[PATCH] D69607: Add a feature to explain why some file gets included to the linker's output

[Rui Ueyama via Phabricator via llvm-commits]

ruiu marked 4 inline comments as done.
ruiu added a comment.

In D69607#1726792 <https://reviews.llvm.org/D69607#1726792>, @peter.smith wrote:

> I agree that having the most common case built in is valuable. Unless there is an analysis program that is shipped with the linker I suspect that the majority of users won't want to write their own algorithm.
>  Looking at the example output:
>
>   '(--entry option)' uses '_start'
>   '/usr/lib/x86_64-linux-gnu/crt1.o' uses 'main'
>   'lld.cpp.o' uses 'lld::elf::link(llvm::ArrayRef<char const*>)'
>   'lib/liblldELF.a(Driver.cpp.o)' uses 'llvm::object::Archive::create()'
>   'lib/libLLVMObject.a(Archive.cpp.o)' uses 'llvm::Expected<bool>::operator bool()'
>   'lib/libLLVMPasses.a(PassBuilder.cpp.o)' uses 'llvm::InlinerPass::~InlinerPass()'
>   'lib/libLLVMipo.a(Inliner.cpp.o)'
>
>
> I'd be tempted to start with the last object in the chain that is unconditionally present on the command line. For example _start, crt1.o, etc. will always be present and most likely not visible to the user as they have been added by the compiler driver. In this case I'd expect lld.cpp.o to be the first entry in the diagnostic.
>
> Although it is implicit, it may be worth stating the dependency as:
>
>   'lld.cpp.o' uses 'lld::elf::link(llvm::ArrayRef<char const*>)' defined in 'lib/liblldELF.a(Driver.cpp.o)'
>
>
> It would make it a bit more verbose, but everything is on one line.


How about the new one? I think I can reverse the order by making the message in a passive tense, but it seems that this order is a bit easier to read.



================
Comment at: lld/ELF/Explain.cpp:91
+  if (!target) {
+    error("--explain: file not found: " + filename);
+    return;
----------------
grimar wrote:
> Perhaps worth to show a list of files available for --explain to user on this step?
> Or have another way to get a list of files that can be passed here. May be explain without arguments could show something.
Added a hint as to how to get a list of input files.


================
Comment at: lld/ELF/Explain.cpp:95
+
+  // Collect root objects.
+  for (InputFile *f : objectFiles)
----------------
pcc wrote:
> Hmm, object files aren't really gc roots, so this could give misleading output (e.g. `ld a.o b.o --explain a` with a.o containing `a: ret` and b.o containing `b: call a` with only b in dynsym won't report b.o). Also, it misses archives included with `--whole-archive`.
> 
> Shouldn't this be more similar to MarkLive.cpp? In other words, include dynsym and don't treat object files as roots.
I think object files are actually roots in this features sense if you do not pass `-gc-sections`, so we need to handle two different cases, `-no-gc-sections` and `-gc-sections`. How is this new code?


================
Comment at: lld/ELF/Explain.cpp:125
+
+      if (sec->areRelocsRela) {
+        for (const typename ELFT::Rela &rel : sec->template relas<ELFT>())
----------------
pcc wrote:
> Shouldn't it also look at `dependentSections`?
Done.


================
Comment at: lld/ELF/Explain.cpp:167
+  auto *sym = dyn_cast_or_null<Defined>(symtab->find(symName));
+  if (!sym || sym->isWeak())
+    return;
----------------
MaskRay wrote:
> pcc wrote:
> > I don't think the `isWeak` part here is correct.
> `-u`/`-e` can fetch a weak lazy definition. This may cause a bogus `file unreachable` error.
Yeah, this was in a wrong place. I moved this to `enqueue`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69607