[PATCH] D115041: [ELF] Do not report undefined weak references in shared libraries

[Igor Kudrin via Phabricator via llvm-commits]

ikudrin added inline comments.


================
Comment at: lld/ELF/InputFiles.cpp:1539
       s->exportDynamic = true;
-      if (s->isUndefined() && !s->isWeak() &&
+      if (s->isUndefined() && sym.getBinding() == STB_GLOBAL &&
           config->unresolvedSymbolsInShlib != UnresolvedPolicy::Ignore)
----------------
MaskRay wrote:
> MaskRay wrote:
> > I just noticed that we did not (before this patch) have test coverage for `&& !s->isWeak()`. 
> > 
> > If the binding check is tested here, Writer.cpp:1978 binding check may be deleted.
> `!sym.isWeak()` would be better if we consider STB_GNU_UNIQUE.
> If the binding check is tested here, Writer.cpp:1978 binding check may be deleted.

We probably cannot delete that check.

If we link `exec` that references symbols in `a.so` that in turn references symbols in `b.so`, and we include `b.so` in the link with `--as-needed`, the symbols defined in `b.so` will be demoted to undefined weak. Now, if we removed the check for `!sym->isWeak()` in `Writer<ELFT>::finalizeSections()`, that would produce false errors. We could avoid that by improving the check for `allNeededIsKnown` to consider shared libraries with `isNeeded == false` as unknown, but that would disable diagnostics for `a.so` entirely.


================
Comment at: lld/test/ELF/allow-shlib-undefined-weak.s:1
+REQUIRES: x86
+
----------------
MaskRay wrote:
> While the suffix name is .s, it is customary to use `#` for RUN/CHECK lines to not render them as instructions which may look weird in some editors.
It was supposed to be `.test`. Just forgot to rename.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D115041/new/

https://reviews.llvm.org/D115041