Driver: loosen restrictions for -force-autolink-symbol
#26614
Conversation
On targets which support COMDAT (PE/COFF, ELF), use that to relax the constraints on the emission of `-force-autolink-symbol` so that it is possible to use that with `-incremental`.
|
CC: @jckarter @rjmccall @jrose-apple MachO does not support COMDAT, which means that we cannot have the linker coalesce and preserve a single definition - if there is a way to emulate that, it would be useful. |
|
@swift-ci please test |
Er. I don't know how you think MachO supports C++ without a basic ability to coalesce definitions. Weak linkage is a totally adequate facility for 99% of what languages need from symbol coalescing; all of COMDAT's complexity goes into supporting the relatively marginal use case of needing coordinated coalescing across multiple symbols. Does this require coordination across symbols? |
|
@rjmccall - hmm, the only coordination that is required is that a definition is preserved, nothing else. If there is a way to ensure that the symbol is preserved, then, I think that this is something that I can generalise to all targets. I'll do a follow up change to enable this for Darwin as well. (A quick glance at LangRef indicates |
|
This may have caused a regression on the Windows build: https://dev.azure.com/compnerd/windows-swift/_build/results?buildId=6214&view=logs&j=2d2b3007-3c5c-5840-9bb0-2b1ea49925f3&t=2fdae087-0aee-589c-2ab0-2b8fc539478d&l=228 |
| // UNSUPPORTED: OS=watchos | ||
| // UNSUPPORTED: OS=ios | ||
|
|
||
| // AUTOLINK_FORCE_LOAD-NOT: error: '-autolink-force-load' is not supported with '-incremental' |
There was a problem hiding this comment.
For negative checks, it's best not to include the exact text of a diagnostic, because we tweak that sometimes.
| @@ -0,0 +1,8 @@ | |||
| // RUN: not %swiftc_driver -incremental -autolink-force-load %s 2>&1 | %FileCheck -check-prefix=AUTOLINK_FORCE_LOAD %s | |||
| // RUN: not %swiftc_driver -autolink-force-load -incremental %s 2>&1 | %FileCheck -check-prefix=AUTOLINK_FORCE_LOAD %s | |||
There was a problem hiding this comment.
There's no real reason to put this in a separate test file; since you're just testing the driver behavior, you can use -### and an explicit triple to check for the error cases.
There was a problem hiding this comment.
Hmm, yes, this is driver, not IRGen. My mistake. Either way, #26635 will remove the file.
|
|
||
| if (!IRGen.Opts.ForceLoadSymbolName.empty() && | ||
| isFirstObjectFileInModule(*this)) { | ||
| (Triple.supportsCOMDAT() || isFirstObjectFileInModule(*this))) { |
There was a problem hiding this comment.
Didn't we add this whole thing in the first place because we were using a COMMON global variable on Mach-O and that didn't work on Windows? Can't we go back to that on non-COMDAT platforms? (See #15647.)
|
#15647 also explains why we can't use |
|
We could presumably use a used, weak_odr, hidden symbol if common isn't good enough for some reason. EDIT: oh, no, not if the symbol needs to be visible outside the linkage unit. |
3 participants
On targets which support COMDAT (PE/COFF, ELF), use that to relax the
constraints on the emission of
-force-autolink-symbolso that it ispossible to use that with
-incremental.Replace this paragraph with a description of your changes and rationale. Provide links to external references/discussions if appropriate.
Resolves SR-NNNN.