If all references to a DSO happen to be weak, --gc-sections eliminates
the DSO from DT_NEEDED. If that happens, we also need to eliminate
shared symbols created from the DSO. Otehrwise, they become dangling
references that point to non-exsitent DSO.
Details
- Reviewers
• espindola grimar andrewrk pcc - Commits
- rG61376d9bed0a: Bring r329960 back.
rG039d24877870: Do not keep shared symbols created from garbage-collected eliminated DSOs.
rL330788: Bring r329960 back.
rLLD330788: Bring r329960 back.
rLLD329960: Do not keep shared symbols created from garbage-collected eliminated DSOs.
rL329960: Do not keep shared symbols created from garbage-collected eliminated DSOs.
Diff Detail
- Repository
- rL LLVM
Event Timeline
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.
From a quick test it looks like this fixes PR28335 but only if --gc-sections is passed. Maybe we should be doing this unconditionally?
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? |
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. |
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 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!
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.
lld/trunk/ELF/Driver.cpp | ||
---|---|---|
1070 | Does it work to replace these with absolute symbols with value 0? |
Does it work to replace these with absolute symbols with value 0?