[PATCH] D108006: [lld][ELF] Add --no-search-static-libs-for-shlib-undefined flag

[Pirama Arumuga Nainar via Phabricator via llvm-commits]

pirama added a comment.

In D108006#2943030 <https://reviews.llvm.org/D108006#2943030>, @MaskRay wrote:

> It's important to have symmetry.  a) `ld.lld def.a ref.so` b) `ld.lld ref.so def.a`.
> The patch addresses a) but not b).

I'll update to handle the case.

> (
> I have noticed missing test coverage. The following diff doesn't cause a test failure.
>
>   --- i/lld/ELF/Symbols.cpp
>   +++ w/lld/ELF/Symbols.cpp
>   @@ -744,3 +744,4 @@ template <class LazyT> void Symbol::resolveLazy(const LazyT &other) {
>    
>   -  other.fetch();
>   +  if (!isShared())
>   +    other.fetch();
>    }

Test coverage is also missing for the lines I changed in `resolveUndefined`.  Will improve test coverage here as well.

> ---
>
> I am trying to understand the rationale behind this symbol resolution rule.
> On one side, it resembles Windows link.exe and Mach-O ld64: AFAIK they don't use undefined symbols in DLL/dylib for resolution.
> (
>
>   # On macOS
>   echo '.globl _main; _main: call bb' > a.s
>   echo '.globl bb; bb: call cc' > b.s
>   echo '.globl cc; cc: nop' > c.s
>   clang --target=x86_64-apple-darwin -c a.s b.s c.s
>   rm -f c.a && ar rc c.a c.o
>    
>   clang -dynamiclib c.o -o c.dylib
>   clang -dynamiclib b.o c.dylib -o b.dylib
>   clang a.o b.dylib c.a
>   # a.out doesn't have cc
>
> )
>
> On the other side, I strive to think of some scenarios where this rule is both sensible and mandatory, but cannot think of one.
> The closest thing I can think of:
>
>   echo '.globl _start; _start: call bb' > a.s
>   echo '.globl bb; bb: call cc' > b.s
>   echo '.globl cc; cc: nop' > c.s
>   cc -c a.s b.s c.s
>   rm -f c.a && ar rc c.a c.o
>   ld -shared -z defs c.o -o c.so
>   ld -shared -z defs b.o ./c.so -o b.so
>   
>   ld -rpath=. a.o b.so c.a
>   # a.out defines cc. ld.lld, ld.bfd, and gold have the same behavior.
>
> c.so and c.a are built from the same set of object files.
> b.so is linked with -Bsymbolic and depends on c.so
>
> For `ld ... b.so c.a`, if ld extracts c.a to satisfy b.so, the c.a definition may conflict (ODR violation) with c.so.
> Keeping c.a unextracted is probably the intention.
>
> I'd say the bug is that c.so and c.a cannot be used together.
> The user should not do both things: (1) let b.so link against c.so (2) let the executable link against c.a .
> Using `--no-undefined` (`-z defs`) for shared objects is not an exuse.
>
> If the executable really needs to use c.a, the shared objects cannot use c.so.

If `c.so` is a utility library, then one client's (`a.out`'s) usage of `c` dictates how another library (`b.so`) _and_ the users of that library are built: any user of `b.so` would now want to link with `c.so/c.a` as well.

In Android build, ODR violations are limited by disallowing static linking of libraries with global state.  The breaking cases for this usage are with utility functions that are pooled together into a single library for historic reasons.

> Currently it is difficult to make `--no-undefined` happy because we don't support an option listing all allow undefined symbols.
> (If we want to add such an option, the option can be modeled after --just-symbols.)




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108006