This is an archive of the discontinued LLVM Phabricator instance.

Do not keep shared symbols to garbage-collected eliminated DSOs.
ClosedPublic

Authored by ruiu on Apr 11 2018, 2:30 PM.

Details

Event Timeline

ruiu created this revision.Apr 11 2018, 2:30 PM
pcc added a subscriber: pcc.Apr 11 2018, 2:44 PM

Does this also fix PR28335?

ruiu added a comment.Apr 11 2018, 2:50 PM

I"m not sure if I understand https://bugs.llvm.org/show_bug.cgi?id=28335 correctly, but it sounds like it fixes that bug as well. The intention of this change is, when a DSO is eliminated because of the garbage collection, all traces of the DSO should be removed as if the DSO weren't present in the command line from the beginning.

andrewrk accepted this revision.Apr 11 2018, 3:00 PM
andrewrk added a subscriber: andrewrk.
This revision is now accepted and ready to land.Apr 11 2018, 3:00 PM
pcc added a comment.Apr 11 2018, 3:10 PM

From a quick test it looks like this fixes PR28335 but only if --gc-sections is passed. Maybe we should be doing this unconditionally?

ruiu updated this revision to Diff 142083.Apr 11 2018, 3:32 PM
  • replace dangling shared symbols even if --gc-sections is not specified
pcc added inline comments.Apr 11 2018, 3:39 PM
lld/ELF/Driver.cpp
1065 ↗(On Diff #142083)

nit: non-existent

lld/test/ELF/as-needed-weak.s
11 ↗(On Diff #142083)

Won't this pass without the change?

Probably a better way to test this would be to check that foo is relocated properly.

ruiu updated this revision to Diff 142100.Apr 11 2018, 4:54 PM
  • fix a test and confirmed that the test fails without this patch
pcc accepted this revision.Apr 11 2018, 5:35 PM

LGTM

espindola added inline comments.Apr 11 2018, 5:50 PM
lld/ELF/Driver.cpp
1069 ↗(On Diff #142100)

Is this doing the right thing if the symbol in the shared library is weak? Is it necessary? Doesn't IsNeeded already include all the required information?

ruiu added inline comments.Apr 11 2018, 5:57 PM
lld/ELF/Driver.cpp
1069 ↗(On Diff #142100)

I believe this does the right thing for weak symbols in shared libraries, because if it is in use, IsNeeded will never be false. Otherwise, the symbol should be handled as if it weren't present from the beginning, so replacing it with an (unresolved) weak undefined seems like the right thing to do.

We could technically ignore shared symbols with a dead file everywhere, but I think purging such symbols all at once before processing further is less error-prone.

espindola accepted this revision.Apr 12 2018, 2:35 PM

LGTM with a small change.

lld/ELF/Driver.cpp
1069 ↗(On Diff #142100)

So the isWeak is really redundant. Please simplify this if to

if (!S->getFile<ELFT>().IsNeeded)

This revision was automatically updated to reflect the committed changes.
rnk added a subscriber: rnk.Apr 16 2018, 3:45 PM

This is causing LLD to drop __cxa_finalize from the symbol tables of some Chromium binaries, and that causes them to crash on shutdown. See the test failures here:
https://ci.chromium.org/buildbot/chromium.clang/ToTLinux/2152

I'm going to revert this for now. I found that re-linking just libipc_mojom_shared.so and libmojo_mojom_bindings_shared.so with LLD after this change causes ipc_tests to crash reliably on shutdown.

In D45536#1069314, @rnk wrote:

This is causing LLD to drop __cxa_finalize from the symbol tables of some Chromium binaries, and that causes them to crash on shutdown. See the test failures here:
https://ci.chromium.org/buildbot/chromium.clang/ToTLinux/2152

I'm going to revert this for now. I found that re-linking just libipc_mojom_shared.so and libmojo_mojom_bindings_shared.so with LLD after this change causes ipc_tests to crash reliably on shutdown.

Could you please put a --reproduce package of those libraries in a bug report? Thanks!

rnk added a comment.Apr 16 2018, 5:39 PM

Could you please put a --reproduce package of those libraries in a bug report? Thanks!

I made a reproducer here: https://drive.google.com/file/d/1LuMUQQK1F1AT5xJnGonBdeODqilqX5Fp/view?usp=sharing

This change caused __cxa_finalize to be removed from the dynamic symbol table, which ultimately lead to a crash in __do_global_dtors_aux during shutdown.

pcc added inline comments.Apr 16 2018, 5:53 PM
lld/trunk/ELF/Driver.cpp
1070 ↗(On Diff #142270)

Does it work to replace these with absolute symbols with value 0?

I am finally trying to reduce what went wrong with this patch.