Page MenuHomePhabricator

[LLD] Improve reporting unresolved symbols in shared libraries
ClosedPublic

Authored by ikudrin on May 6 2021, 7:36 AM.

Details

Summary

Currently, when reporting unresolved symbols in shared libraries, if an undefined symbol is firstly seen in a regular object file that shadows the reference for the same symbol in a shared object. As a result, the error for the unresolved symbol in the shared library is not reported. If referencing sections in regular object files are discarded because of --gc-sections, no reports about such symbols are generated, and the linker finishes successfully, generating an output image that fails on the run.

The patch fixes the issue by keeping symbols, which should be checked, for each shared library separately.

Diff Detail

Event Timeline

ikudrin created this revision.May 6 2021, 7:36 AM
ikudrin requested review of this revision.May 6 2021, 7:36 AM

The current implementation checks the file where the symbol was seen for the first time. That might be a file that does not meet the requirements for reporting, e.g. it might have some DT_NEEDED missed or it might be a relocatable file that also references the symbol. As a result, the actual issue is not detected, the error is not reported, and an incorrect output file is generated whereas it should not.

"it might have some DT_NEEDED missed" -> We don't recursively load DT_NEEDED so this is expected. This patch doesn't address the issue as well. The description may give a false impression that the issue is tackled.

I think the issue is entirely: an undefined symbol in a regular object file shadows an undefined symbol in a shared object, and that regular object file doesn't get diagnosed (-z defs) because --gc-sections discards the referencing sections.
You can amend the description to mention this. Are there other types of missing --no-allow-shlib-undefined diagnostics?

lld/test/ELF/unresolved-in-dso.ll
1 ↗(On Diff #343402)

Can this be placed in allow-shlib-undefined.s or named similar to that file?

14 ↗(On Diff #343402)

You can place %t.o and %ta.o in the same object file.

MaskRay added inline comments.May 6 2021, 9:29 AM
lld/test/ELF/unresolved-in-dso.ll
15 ↗(On Diff #343402)

Perhaps cp %tb.so and add another test that we can now report more than one .so for one undefined symbol.

MaskRay added inline comments.May 6 2021, 12:09 PM
lld/ELF/InputFiles.h
388

Perhaps simply this to --no-allow-shlib-undefined

ikudrin updated this revision to Diff 343634.May 7 2021, 3:51 AM
ikudrin marked 3 inline comments as done.
ikudrin retitled this revision from [LLD] Improve reporting of unresolved symbols in shared libraries to [LLD] Improve reporting unresolved symbols in shared libraries.
ikudrin edited the summary of this revision. (Show Details)
  • Updated a comment for SharedFile::requiredSymbols
  • Moved the test into allow-shlib-undefined.s
  • Added a test to check that the error is reported for each shared library where the symbol is referenced

The current implementation checks the file where the symbol was seen for the first time. That might be a file that does not meet the requirements for reporting, e.g. it might have some DT_NEEDED missed or it might be a relocatable file that also references the symbol. As a result, the actual issue is not detected, the error is not reported, and an incorrect output file is generated whereas it should not.

"it might have some DT_NEEDED missed" -> We don't recursively load DT_NEEDED so this is expected. This patch doesn't address the issue as well. The description may give a false impression that the issue is tackled.

I meant that, as we do not check such files, they may prevent us to detect the issue in files that come after them, just like if a symbol is first seen in a regular object file. The case is roughly the same as with object files, so I did not add a distinct test for that. Probably, it would be more clear to remove that from the description.

I think the issue is entirely: an undefined symbol in a regular object file shadows an undefined symbol in a shared object, and that regular object file doesn't get diagnosed (-z defs) because --gc-sections discards the referencing sections.
You can amend the description to mention this.

Thanks for the suggestions. I'll update the description.

Are there other types of missing --no-allow-shlib-undefined diagnostics?

Haven't noticed yet.

lld/test/ELF/unresolved-in-dso.ll
14 ↗(On Diff #343402)

As the test is moved into allow-shlib-undefined.s, that reference should be kept in a separate object file to not tamper with the existing tests.

MaskRay accepted this revision.May 7 2021, 9:28 AM

Looks great!

lld/ELF/InputFiles.h
388

Drop and alike? There is no other usage.

This revision is now accepted and ready to land.May 7 2021, 9:28 AM
ikudrin added inline comments.May 7 2021, 7:26 PM
lld/ELF/InputFiles.h
388

It's about --unresolved_symbols=ignore-in-object-files and --unresolved_symbols=report-all. The implementation is the same, but the switches are still different.

MaskRay added inline comments.May 10 2021, 3:10 PM
lld/ELF/InputFiles.h
388

To the best of my knowledge, no project uses ignore-in-object-files or report-all. Intentionally we just translate them to --no-allow-shlib-undefined. I do slightly prefer dropping and alike, but am fine if you want to keep it.