This is an archive of the discontinued LLVM Phabricator instance.

[ELF] Move IsNeeded logic from SymbolTable::addShared to MarkLive, and check IsUsedInRegularObj
ClosedPublic

Authored by MaskRay on Dec 19 2018, 2:17 PM.

Details

Summary

In glibc, libc.so is a linker script with an as-needed dependency on ld-linux-x86-64.so.2

GROUP ( /lib/x86_64-linux-gnu/libc.so.6 /usr/lib/x86_64-linux-gnu/libc_nonshared.a  AS_NEEDED ( /lib/x86_64-linux-gnu/ld-linux-x86-64.so.2 ) )

ld-linux-x86-64.so.2 (as-needed) defines some symbols which resolve undefined references in libc.so.6, it will therefore be added as a DT_NEEDED entry, which isn't necessary.

The test case as-needed-not-in-regular.s emulates the libc.so scenario, where ld.bfd and gold don't add DT_NEEDED for a.so

The relevant code in gold/resolve.cc:

// If we have a non-WEAK reference from a regular object to a
// dynamic object, mark the dynamic object as needed.
if (to->is_from_dynobj() && to->in_reg() && !to->is_undef_binding_weak())
  to->object()->set_is_needed();

in_reg() appears to do something similar to IsUsedInRegularObj.

This patch makes lld do the similar thing, but moves the check from
addShared to a later stage MarkLive where all symbols are scanned.

Event Timeline

MaskRay created this revision.Dec 19 2018, 2:17 PM
ruiu added inline comments.Dec 19 2018, 2:33 PM
ELF/SymbolTable.cpp
508–509

This part of the logic seems a bit unnecessarily complicated (though I believe I wrote this.) Let's simplify this first.

MaskRay added inline comments.Dec 19 2018, 2:38 PM
ELF/SymbolTable.cpp
508–509

seems a bit unnecessarily complicated

It is. It took me some time to understand if !Config->GcSections is correct here and later I think it is. I'll also do some experiments to see if the logic can be simplified.

ruiu added inline comments.Dec 19 2018, 3:50 PM
ELF/SymbolTable.cpp
510–511

Thinking about this code, I'm no longer sure if this code makes sense. If IsNeeded is set correctly for --gc-sections, we can just remove this if and let the later pass to set an appropriate bit to IsNeeded, maybe?

MaskRay marked an inline comment as done.Dec 19 2018, 3:56 PM
MaskRay added inline comments.
ELF/SymbolTable.cpp
510–511

This if handles the --no-gc-sections case. No later passes set the IsNeeded bit.
For the --gc-sections case, a later pass (MarkLive.cpp:resolveReloc) sets the IsNeeded bit.

grimar added a subscriber: grimar.Dec 20 2018, 12:16 AM
grimar added inline comments.
test/ELF/as-needed-not-in-regular.s
3

A minor suggestion. I am trying to add a short description to each test
case I am writing. I.e. about what the purpose, what we do and what we expect.
It's sometimes really useful because commit log is not always helpful.
Could you add such comment?

MaskRay marked an inline comment as done.Dec 20 2018, 12:16 PM
MaskRay added inline comments.
test/ELF/as-needed-not-in-regular.s
3

I've already added one " The symbol a is not referenced by a regular object." but I can extend it. Thanks for the reminder.

MaskRay updated this revision to Diff 179124.Dec 20 2018, 12:26 PM
MaskRay removed a subscriber: grimar.

Elaborate a comment

ruiu added inline comments.Dec 20 2018, 12:37 PM
ELF/SymbolTable.cpp
510–511

I still want to make this simpler... At least I don't think this is correct per se, as you need to address the case in which a shared symbol is added first and then an undefined symbol is added later. In that case you need to set IsNeeded too.

I wonder if a DSO is needed if and only if there is a shared symbol for the DSO whose isWeak() and IsUsedRegularObj are both true. If that's the case, we can remove the code from here, and add another loop after the symbol resolution to visit all symbols to set IsNeeded bit for DSOs.

MaskRay updated this revision to Diff 179157.Dec 20 2018, 2:18 PM
MaskRay retitled this revision from [ELF] SymbolTable::addShared: don't set IsNeeded unless IsUsedInRegularObj to [ELF] Move IsNeeded logic from SymbolTable::addShared to MarkLive, and check IsUsedInRegularObj.
MaskRay edited the summary of this revision. (Show Details)

Move logic to MarkLive

MaskRay marked an inline comment as done.Dec 20 2018, 2:21 PM
MaskRay marked 4 inline comments as done.
ruiu accepted this revision.Dec 20 2018, 2:33 PM

Looks great! LGTM

This revision is now accepted and ready to land.Dec 20 2018, 2:33 PM
MaskRay updated this revision to Diff 179164.Dec 20 2018, 2:46 PM

Add another test as-needed-in-regular.s

ruiu added a comment.Dec 20 2018, 2:48 PM

Please commit.

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptSep 7 2022, 12:14 AM