[PATCH] D57385: [ELF] Support --{,no-}allow-shlib-undefined

[Rui Ueyama via Phabricator via llvm-commits]

ruiu added a comment.

ld.bfd traces all DSOs using the same logic as the runtime to try to resolve all undefined symbols in all DSOs. The linker even reads from `/etc/ld.so.conf` to see how DSOs would be resolved at runtime. That's I believe the original concept of `--no-allow-shlib-undefines`, as you can find any DSO symbols that can fail to be resolved at runtime. But that's apparently too much for us.

ld.gold instead does what you implemented in this patch. For a DSO X, if we know all DSOs that X depends on, then we emit a warning if there is an undefined symbol in X that cannot be resolved using any DSO. That perhaps achieves the right balance between being useful and being not too smart.

So I think this patch essentially looks good. The only concern is that the patch doesn't contain any comment about the design decision as described above.



================
Comment at: ELF/InputFiles.h:335
   std::vector<const Elf_Verdef *> Verdefs;
+  std::vector<StringRef> Needed;
   std::string SoName;
----------------
We already have `IsNeeded` bit, so `Needed` would be confusing. `DtNeeded` is better.


================
Comment at: ELF/Writer.cpp:1689
   for (Symbol *Sym : Symtab->getSymbols()) {
+    auto *File = dyn_cast_or_null<SharedFile<ELFT>>(Sym->File);
+    if (!Config->AllowShlibUndefined && File && Sym->isUndefined() &&
----------------
This code is added without what this is supposed to do, so it would be hard to understand in the future.

Is this the best place to add this error check? Can you run another for loop to separate this new logic from another?


================
Comment at: ELF/Writer.cpp:1694
+      bool WasInserted;
+      std::tie(It, WasInserted) = AllNeededIsKnown.try_emplace(File, false);
+      if (WasInserted) {
----------------
The way you check whether we've seen all needed libraries seems a bit too complicated. I think that can be implemented much simpler code. At least you shouldn't mix two independent checks, undefined symbol check and needed lib check in a single loop. The latter can be done in a separate loop.


Repository:
  rLLD LLVM Linker

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

https://reviews.llvm.org/D57385