Unify linker argument order across platforms #18266
Conversation
Previously extra linker arguments had different behavior on darwin vs other unix platforms. On darwin the arguments passed with -Xlinker would be passed to the linker before the default arguments, where as with the default unix toolchain they would be passed afterwards. There isn't really a great option for which order these should be in. If you want to have a custom rpath that takes precedence over the default rpaths, you want them to be passed before, but if you want to negate a default argument you want them to come after. This change unifies the behavior so at least you always get the same behavior across platforms.
|
@jrose-apple can you take a look? or suggest who would be the right person to review this change? |
|
I'm the right person. All -X options are supposed to go after regular options, not before. Maybe we'll come up with an "-X<foo>-before" variant later, but Darwin is the designed behavior. |
|
(It'd also be reasonable both to explicitly support |
On Darwin they appear to come before now, are you saying I should move those instead? |
|
Aargh, I can't read. On both toolchains they are in the middle but Darwin is close to the start and Unix is close to the end. They should be last-but-just-before-the- (All the other -X options probably need to be checked too, and then get a big comment making sure people don't move them.) |
|
Sounds good I'll move these down, add the comment, fix the tests, and look for other cases for a separate change |
|
I've moved these down for both the unix and darwin toolchains |
|
I don't see any other |
lib/Driver/UnixToolChains.cpp
Outdated
| @@ -325,6 +322,10 @@ toolchains::GenericUnix::constructInvocation(const LinkJobAction &job, | |||
| Arguments.push_back("-v"); | |||
| } | |||
|
|
|||
| // These custom arguments should be right before the object file at the end | |||
| context.Args.AddAllArgs(Arguments, options::OPT_Xlinker); | |||
| context.Args.AddAllArgs(Arguments, options::OPT_linker_option_Group); | |||
There was a problem hiding this comment.
Ah, OPT_linker_option_Group should go before -Xlinker too. That's just a convenience for forwarding arguments.
There was a problem hiding this comment.
Can you add a test for this as well? That would just be an invocation that uses both -L and -Xlinker.
There was a problem hiding this comment.
Added, renamed the tests too since it's now not just about Xlinker
lib/Driver/UnixToolChains.cpp
Outdated
| @@ -325,6 +322,10 @@ toolchains::GenericUnix::constructInvocation(const LinkJobAction &job, | |||
| Arguments.push_back("-v"); | |||
| } | |||
|
|
|||
| // These custom arguments should be right before the object file at the end | |||
There was a problem hiding this comment.
Nitpick: please end comments with a period.
|
We've got -Xcc (processed in the Clang importer sources, not at the driver level), -Xllvm (I guess this doesn't matter too much), and -Xfrontend (only for compiler devs to work with). -Xlinker is the only important one in the driver. |
|
@swift-ci Please smoke test |
|
@swift-ci Please test source compatibility |
|
@jrose-apple I think this one's ready to go 🙏 |
|
Oops, sorry, forgot to assign it to myself so I wouldn't lose track of it. |
|
No worries, thanks! |
Previously extra linker arguments had different behavior on darwin vs
other unix platforms. On darwin the arguments passed with -Xlinker would
be passed to the linker before the default arguments, where as with the
default unix toolchain they would be passed afterwards.
There isn't really a great option for which order these should be in.
If you want to have a custom rpath that takes precedence over the
default rpaths, you want them to be passed before, but if you want to
negate a default argument you want them to come after.
This change unifies the behavior so at least you always get the same
behavior across platforms.
Resolves SR-6508.