Page MenuHomePhabricator

[ELF] resolveUndefined: ignore undefined symbols in SharedFile for Undefined and SharedSymbol
ClosedPublic

Authored by MaskRay on Jul 3 2019, 8:07 AM.

Details

Summary

If %t1.o has a weak reference on foo, and %t2.so has a non-weak
reference on foo: ld.lld %t1.o %t2.so -o %t

We incorrectly set the binding of the undefined foo to STB_GLOBAL.
Fix this by ignoring undefined symbols in a SharedFile for Undefined and
SharedSymbol.

This fixes the binding of pthread_once when the program links against
both librt.so and libpthread.so

a.o: STB_WEAK reference to pthread_once
librt.so: STB_GLOBAL reference to pthread_once    # should be ignored
libstdc++.so: STB_WEAK reference to pthread_once  # should be ignored
libgcc_s.so.1: STB_WEAK reference to pthread_once # should be ignored

The STB_GLOBAL pthread_once issue (not fixed by D63974) can cause a link error when the result
DSO is used to link another DSO with -z defs if -lpthread is not specified. (libstdc++.so.6 not having a dependency on libpthread.so is a really nasty hack...)

We happened to create a weak undef before D63974 because libgcc_s.so.1
was linked the last and it changed the binding again to weak.

Diff Detail

Repository
rL LLVM

Event Timeline

MaskRay created this revision.Jul 3 2019, 8:07 AM
Herald added a project: Restricted Project. · View Herald Transcript
MaskRay updated this revision to Diff 207803.Jul 3 2019, 8:12 AM
MaskRay edited the summary of this revision. (Show Details)

Reword

MaskRay edited the summary of this revision. (Show Details)Jul 3 2019, 8:15 AM
peter.smith added inline comments.Jul 3 2019, 8:29 AM
ELF/Symbols.cpp
413 ↗(On Diff #207803)

Wouldn't this prevent a non-weak reference from a shared library from fetching an archive member? Or am I missing something else that permits that. Apologies I've not got much time left today.

MaskRay updated this revision to Diff 207812.Jul 3 2019, 9:15 AM
MaskRay edited the summary of this revision. (Show Details)

Check an undef ref from a SharedFile can fetch an archive.

This was not tested before.

MaskRay marked an inline comment as done.Jul 3 2019, 9:17 AM
MaskRay added inline comments.
ELF/Symbols.cpp
413 ↗(On Diff #207803)

No worries!

Thank you for noticing this. It turns out we do not have a test to check a shared reference can fetch an archive. I thus added the test to test/ELF/archive-fetch.s

Moved the logic below the if (isLazy()) {...} branch.

MaskRay updated this revision to Diff 207818.Jul 3 2019, 9:33 AM
MaskRay retitled this revision from [ELF] resolveUndefined: ignore undefined symbols in SharedFile to [ELF] resolveUndefined: ignore undefined symbols in SharedFile for Undefined and SharedSymbol.
MaskRay edited the summary of this revision. (Show Details)

Simpler example

MaskRay updated this revision to Diff 207974.Jul 3 2019, 10:48 PM
MaskRay edited the summary of this revision. (Show Details)

Simplify description

MaskRay edited the summary of this revision. (Show Details)Jul 3 2019, 10:55 PM
grimar added a comment.Jul 4 2019, 1:27 AM

LG, I have no other comments except a minor suggestion below.

ELF/Symbols.cpp
481 ↗(On Diff #207974)

Perhaps an early return would look a bit better here:

if (isLazy()) {
...
return;
}

// Undefined symbols in a SharedFile do not change the binding.
if (isa_and_nonnull<SharedFile>(Other.File))
  return;

...

Thanks for the update and the extra test. Functionally I think this looks good.

MaskRay updated this revision to Diff 207990.Jul 4 2019, 1:52 AM

use early return

ruiu accepted this revision.Jul 4 2019, 3:21 AM

LGTM

ELF/Symbols.cpp
485 ↗(On Diff #207990)

Little off-topic, but I'd probably just use dyn_cast instead of this new predicate so that code readers don't have to memorize one more predicate.

This revision is now accepted and ready to land.Jul 4 2019, 3:21 AM
MaskRay marked an inline comment as done.Jul 4 2019, 3:25 AM
MaskRay added inline comments.
ELF/Symbols.cpp
485 ↗(On Diff #207990)

Do you mean dyn_cast_or_null?

dyn_cast cannot be used because it would assert on a nullptr.

This revision was automatically updated to reflect the committed changes.
ruiu added inline comments.Jul 4 2019, 3:39 AM
ELF/Symbols.cpp
485 ↗(On Diff #207990)

Yes, sorry, I meant dyn_cast_or_null. If you prefer isa_and_nonnull, I'm fine, though.