This is an archive of the discontinued LLVM Phabricator instance.

[LLD][ELF] --wrap: __real_foo references should trigger archive extraction for foo
ClosedPublic

Authored by bd1976llvm on Oct 13 2022, 11:00 AM.

Details

Summary

A reference to __real_foo should trigger archive extraction of the input file that defines foo, otherwise a link using --wrap=foo might fail to link with an undefined reference to foo.
This matches bfd linker behaviour.

Replaces https://reviews.llvm.org/D135737.

Diff Detail

Event Timeline

bd1976llvm created this revision.Oct 13 2022, 11:00 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 13 2022, 11:00 AM

This makes sense to me, and I tested it internally and confirmed it doesn't change anything for us, but I'd prefer @MaskRay to take a look as well.

lld/test/ELF/wrap-extract-real.ll
20

Why does this become weak? (It's global for the assembly test case, which is what I'd expect.)

MaskRay accepted this revision.Oct 14 2022, 3:00 PM

The new semantics look good.

lld/ELF/Driver.cpp
2249

If __real_ is referenced, pull in the symbol if it is lazy

lld/test/ELF/wrap-extract-real.s
18

1 => [[#]]

This revision is now accepted and ready to land.Oct 14 2022, 3:00 PM
bd1976llvm added inline comments.Oct 17 2022, 4:53 PM
lld/test/ELF/wrap-extract-real.ll
20

Good question! It seems to be a bug. Weak binding is assigned by this piece of code:

llvm\lib\LTO\LTO.cpp:

// For symbols re-defined with linker -wrap and -defsym options,
// set the linkage to weak to inhibit IPO. The linkage will be
// restored by the linker.
if (Res.LinkerRedefined)
  GV->setLinkage(GlobalValue::WeakAnyLinkage);

However, LLD doesn't seem to have fulfilled its side of the bargain alluded to in this comment.

I'll look at this more tomorrow.

smeenai added inline comments.Oct 17 2022, 5:06 PM
lld/test/ELF/wrap-extract-real.ll
20

Ah, I remember this now; that's https://github.com/llvm/llvm-project/issues/51346. It's independent of your change though, so you can certainly land this without needing to fix that.

MaskRay added inline comments.Oct 17 2022, 5:10 PM
lld/test/ELF/wrap-extract-real.ll
20

So this is related to a WeakAnyLinkage workaround D33621. It'd be difficult to fix properly but it's good to re-think how to handle it.

bd1976llvm marked 5 inline comments as done.Oct 18 2022, 4:45 AM
bd1976llvm added inline comments.
lld/test/ELF/wrap-extract-real.ll
20

Thanks.

20

Interesting. Thanks.

bd1976llvm marked an inline comment as done.

Addressed review comments.

Herald added a project: Restricted Project. · View Herald TranscriptOct 18 2022, 4:54 AM