This is an archive of the discontinued LLVM Phabricator instance.

[LLD] [COFF] Implement a GNU/ELF like -wrap option
ClosedPublic

Authored by mstorsjo on Oct 7 2020, 2:10 PM.

Details

Summary

Add a simple forwarding option in the MinGW frontend, and implement the private -wrap option in the COFF linker.

The feature in lld-link isn't gated by the -lldmingw option, but the option is left as a private, undocumented option primarily used by the MinGW driver.

The implementation is significantly based on the support for --wrap in the ELF linker, but many small nuance details are different between the ELF and COFF linkers, ending up with more than a few implementation differences.

This fixes https://bugs.llvm.org/show_bug.cgi?id=47384.

Diff Detail

Event Timeline

mstorsjo created this revision.Oct 7 2020, 2:10 PM
Herald added a project: Restricted Project. · View Herald TranscriptOct 7 2020, 2:10 PM
mstorsjo requested review of this revision.Oct 7 2020, 2:10 PM
orgads added a comment.Oct 7 2020, 8:46 PM

Thank you very much!

I'll try to test it next week.

lld/COFF/Driver.cpp
2012

-wrap?

2061

ditto

lld/COFF/MinGW.cpp
194

You have using namespace lld::coff. Remove the qualifiers? (repeats below)

lld/test/MinGW/driver.test
285

-wrap foo2?

286

ditto

mstorsjo added inline comments.Oct 7 2020, 10:54 PM
lld/COFF/Driver.cpp
2012

Oh, right, yes, within lld-link, that's the only correct spelling of it.

lld/test/MinGW/driver.test
285

No, GNU ld supports the option with one or two leading dashes, so this intentionally tests both (and either with the parameter as a separate argument, or with concatenated with an equals char). The option on the lld-link level on the other hand, has only one spelling.

mstorsjo updated this revision to Diff 296870.Oct 7 2020, 11:00 PM

Consistently spell the option "-wrap" within comments within the COFF subdirectory.

rnk added a comment.Oct 13 2020, 4:02 PM

I had a naming suggestion, but otherwise it looks neat.

lld/COFF/MinGW.cpp
194
lld/COFF/Symbols.h
141

There are more kinds of IPO than just inlining, so the name isn't quite right. I also would prefer to reverse the sense so that a default value of false is the more prevalent value, and only special wrapped symbols set this to true. I suggest using the name isHiddenFromLTO for that purpose.

mstorsjo added inline comments.
lld/COFF/Symbols.h
141

Sounds like a good suggestion, but this is (including the comment) an exact copy of the corresponding field from the ELF side - and consistency also is nice.

Or should I go with this and then rename accordingly on the ELF side afterwards - WDYT @MaskRay?

MaskRay added inline comments.Oct 13 2020, 9:29 PM
lld/COFF/Symbols.h
141

I agree that canInline is a misnomer. isHiddenFromLTO may be a bit confusing as well: there is an LTO property called VisibleToRegularObj. "hidden" and "Visible" are confusing when used together. FWIW in lib/LTO, this property is called LinkerRedefined.

However, LinkerRedefined is not great for the ELF case because a linker script symbol assignment (e.g. a = 1013;) does not set LinkerRedefined. The ELF port just overrides the LTO definition in a later pass.

The testsuite is simply ported from test/ELF. The way the tests are organized in ELF is actually not great. For the 3 symbols: foo __wrap_foo __real_foo. There are 8 states of their presence. The ELF naming is a bit arbitrary.

The testsuite is simply ported from test/ELF.

It's based on those tests yes, but some amount of modifications, and with a few new things. In particular, I added a negative test to see that we really do fail if referencing __real_func but it isn't available. (During development, while fiddling with different flags, I had one situation where such an error actually could slip through.)

The way the tests are organized in ELF is actually not great. For the 3 symbols: foo __wrap_foo __real_foo. There are 8 states of their presence. The ELF naming is a bit arbitrary.

Any concrete suggestions on what to improve?

lld/COFF/MinGW.cpp
194

Removing it from the WrappedSymbol references.

mstorsjo updated this revision to Diff 298045.Oct 13 2020, 11:16 PM

Removed superfluous lld::coff:: specifiers where possible. Added a testcase for wrapping an imported symbol. (In that case, one can't do the wrapping by replacing the actual Symbol object contents, because that breaks generation of the import thunks.) Didn't rename the canInline field yet, pending consensus on what to rename it to.

rnk accepted this revision.Oct 14 2020, 9:19 AM
rnk added inline comments.
lld/COFF/Symbols.h
141

I think consistency with ELF is better than trying to improve on the naming. We can do a follow-up to rename all the properties to something sensible and consistent.

For example, I like IsUsedByRegularObj better than VisibleFromRegularObj, and I think we can improve on LinkerRedefined, and make that match whatever LLD uses in both ELF and COFF.

This revision is now accepted and ready to land.Oct 14 2020, 9:19 AM
This revision was automatically updated to reflect the committed changes.

Thank you very much!

Can you please send me a static 64-bit build like you did in the old days? ;)

If you can give me instructions how to build it that would be even better :)

This breaks the Windows build:

C:\src\llvm-mint\lld\COFF\Symbols.cpp(26,1): error: static_assert failed due to requirement 'sizeof(lld::coff::SymbolUnion) <= 48' "symbols should be optimized for memory usage"
static_assert(sizeof(SymbolUnion) <= 48,

Reverted in 3d338f681340e2075707eabcf530bcc0c37da80e.

(example bot failure: http://45.33.8.238/win/25918/step_4.txt, I also repro'd locally with the normal CMake build)

MaskRay added inline comments.Oct 15 2020, 10:37 AM
lld/COFF/Symbols.h
141

This needs to be unsigned, instead of uint8_t, to have good packing on MSVC (I think).

rnk added inline comments.Oct 15 2020, 10:40 AM
lld/COFF/Symbols.h
141

Yep, that should be sufficient to fix the error. I'm glad I put the assert in, or we'd never find out about these regressions. :)

mstorsjo added inline comments.Oct 15 2020, 11:22 AM
lld/COFF/Symbols.h
141

Thanks, reapplying it with that fixed soon - and sorry for the breakage.

mstorsjo added a comment.EditedOct 15 2020, 1:25 PM

Thank you very much!

Can you please send me a static 64-bit build like you did in the old days? ;)

If you can give me instructions how to build it that would be even better :)

I primarily cross compile things, so to build it, I'd do something equivalent to this, on linux:

  • Check out https://github.com/mstorsjo/llvm-mingw
  • Build a cross toolchain using LLVM_VERSION=<current_rev> ./build-all.sh ~/llvm-mingw-latest
  • Using the newly built cross toolchain, cross compile LLVM and the rest of the toolchain to run on windows, ./build-cross-tools.sh ~/llvm-mingw-latest ~/llvm-mingw-latest-x86_64 x86_64

If you want the toolchain not to use a dynamically linked libc++, you can either add --disable-shared in build-all.sh on the ./build-libcxx.sh line before the second step, or delete the libc++.dll.a and libunwind.dll.a files from the built toolchain after the second step.

If you've got an old checkout to build in, either export SYNC=1 (to force it to fetch the given revision instead of using whatever was checked out) and CLEAN=1 (to clean the existing build directories before building) or nuke it completely before starting.

If you've only got access to building things on MSYS, the same steps should mostly apply still. If you just run the first step, you'll end up with a working LLD, but it's linked against libstdc++ and winpthreads - and some people report issues still with the LLD threading and winpthreads. So on MSYS, the second step would rebuild all of the tools using the newly built tools, so you have an LLD linked against libc++.