[PATCH] D59275: [ELF] Do not emit weak-undef symbols in .dynsym under -pie --no-dynamic-linker.

[Siva Chandra via Phabricator via llvm-commits]

sivachandra marked an inline comment as done.
sivachandra added inline comments.


================
Comment at: lld/ELF/Symbols.cpp:270-271
     return false;
+  if (isUndefWeak() && Config->Pie && Config->DynamicLinker.empty())
+    return false;
   if (!isDefined())
----------------
ruiu wrote:
> sivachandra wrote:
> > ruiu wrote:
> > > pcc wrote:
> > > > sivachandra wrote:
> > > > > ruiu wrote:
> > > > > > sivachandra wrote:
> > > > > > > ruiu wrote:
> > > > > > > > This really needs comment. Also, why do you have to check if `DynamicLinker` was not set? In lld, `--dynamic-linker` is used only for setting the name of the dynamic linker. Shouldn't this be `--static`?
> > > > > > > Reading pcc's comment from yesterday, it seemed to me that aligning with what other linkers do would be nice. So, ld.bfd for example, does not just really use "-pie -static" to prevent weak-undef symbols from showing up in .dynsym. It does so if --no-dynamic-linker is specified. Hence, the check for the dynamic linker name as above.
> > > > > > What we really should do is to the thing that makes the most sense. Aligning behavior with other linkers makes sense in many cases, but we don't necessarily blindly copy all corner-case behaviors. To me, the logic that "if an executable is not dynamically linked, there's no point of exporting weak symbols" makes the most sense -- thus, checking the value of `--dynamic-linker` looks weird.
> > > > > To avoid more round trips: how does diff 2 look? It is as per what you are expecting and that is what I started with. 
> > > > `-static` is an alias for `-Bstatic` (i.e. don't search for `.a` files for `-l`). It doesn't necessarily imply that the executable is statically linked because the user could have either passed a `.so` file directly or enabled `-Bdynamic` at some point.
> > > > 
> > > > A more accurate test for whether the executable is dynamically linked is whether `SharedFiles` is non-empty. This is also part of the test that we use when deciding whether to emit a `.dynsym`: http://llvm-cs.pcc.me.uk/tools/lld/ELF/Driver.cpp#1498
> > > > 
> > > > Would it make sense to use something similar here?
> > > Ah, yeah, checking SharedFiles makes sense to me. Thank you for pointing that out.
> > Few of the existing tests will start requiring that we pass a shared object on the lld command line Is this OK?
> Can you elaborate? Did you mean that some tests started failing with this change?
Look at the change to weak-undef.s test that I have now added to this change. There are couple of tests also failing for the same reason, but I have not added them to this change yet.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59275