[Driver] Work around lld 13+ issue with --gc-sections for ELF by adding -z nostart-stop-gc #60544
Conversation
|
@3405691582, would you try this fix on OpenBSD? |
|
This doesn't seem to solve the issue with |
Hmm, do you build with the early swift driver? That reminds me, I'll need to patch that too. |
No, I haven't built with that in a little while (I probably should remedy that, though). |
Oh, I just realized, that failing command, does it add |
|
Oh, duh, got it: I just checked and the compiler build doesn't link this library with the Swift compiler, it uses a clang++ command for the final link step. As such, this pull won't help with that library, which was never my aim anyway, and you will need to keep using whatever workaround you had for that library before or pass this lld flag for that library separately. |
…ng -z nostart-stop-gc
|
@compnerd, would you review? |
|
@swift-ci please test |
|
Hmm, did we already do this in the Swift driver? If not, we should ensure that we get it in there as well. |
|
I submitted it there too, swiftlang/swift-driver#1153, feel free to review that one too. |
|
Does this cover the case where you don't pick lld but it's the inferred linker? Isn't that the case on android with new NDKs? I still feel like we need to get to the bottom of the section preservation 😞 |
Depends what you mean, if you mean inferred by the Swift compiler as the default linker for that platform, then yes. However, as I noted on the issue I opened, I don't know how to handle the scenario where someone changes the system ld to link to lld instead, as opposed to the old binutils ld it usually points at on linux, so I don't bother with that.
It is inferred by the Swift compiler, so that should work with this pull.
I'm all for someone looking into that and us trying other approaches, this is just the only one that works so far, as demonstrated in the test run data I posted in #60406. I asked for feedback on alternatives there, and didn't get any. |
|
@keith, please merge if you are okay with this approach for now, so we have a fix in the compiler for this lld issue with stripping ELF binaries. You can always investigate #60357 more later if you like- I don't care enough about this problem to dig deeper on those other possible fixes- and swap this out for an updated version of that pull later on, that hopefully fixes those additional tests that fail with that In the meantime, I'd like to get this merged in trunk and 5.7 and re-enable your SPM pulls for dead-stripping, hopefully before 5.7 releases. We can always change this fix later if you find a better way. |
|
@keith, what do you think? If it's too late to get this into 5.7 before it releases, I'm in no hurry and can wait on this. |
|
I don't know if it's too late for 5.7 or not, I'd say open the PR and hopefully the release manager can take a look |
|
Seems like a good fix until we can track down the core issue |
It should work with lld now that the lld flag '-z nostart-stop-gc' was added with swiftlang/swift#60544 and swiftlang/swift-driver#1153.
…ng -z nostart-stop-gc (swiftlang#60544) Fixes swiftlang#60406 Ran the linux x86_64 compiler validation suite on this pull with `-use-ld=lld -Xlinker --gc-sections`, then without stripping as in the linked issue, and only saw the 20 unrelated failures just like @drodriguez had.
It should work with lld now that the lld flag '-z nostart-stop-gc' was added with swiftlang/swift#60544 and swiftlang/swift-driver#1153.
…by adding -z nostart-stop-gc (swiftlang#60544)
…ng -z nostart-stop-gc (swiftlang#60544) Fixes swiftlang#60406 Ran the linux x86_64 compiler validation suite on this pull with `-use-ld=lld -Xlinker --gc-sections`, then without stripping as in the linked issue, and only saw the 20 unrelated failures just like @drodriguez had.
Fixes #60406
Ran the linux x86_64 compiler validation suite on this pull with
-use-ld=lld -Xlinker --gc-sections, then without stripping as in the linked issue, and only saw the 20 unrelated failures just like @drodriguez had.I would have added this test to Driver/linker.swift, but I see those tests have been disabled for more than two years now, #32818. @nate-chandler, seems like those should be re-enabled by now?