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

Fri Nov 1 03:50:28 PDT 2019

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

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

> Thanks for the update, I'm happy with the explain output format.
>
> One anomaly that I'm not sure is worth dealing with is the linkerscript GROUP command as used in libc.so
>
>   GROUP ( /lib/libc.so.6 /usr/lib/libc_nonshared.a  AS_NEEDED ( /lib/ld-linux-armhf.so.3 ) )
>
>
> I think that these will just end up as input files on the command line. Although they are non-obvious and implicitly included due to the linker script.

In most cases I believe linker scripts are explicitly added to the command line by users, and that wouldn't cause too much confusion as they should know what they are doing. One exception is Linux's libc.so which is not a DSO but actually a linker script, but maybe that's the only implicit use case?

================
Comment at: lld/ELF/Explain.cpp:157
+
+  if (!seen.insert(sym.file).second)
+    return;
----------------
MaskRay wrote:
> ruiu wrote:
> > ruiu wrote:
> > > MaskRay wrote:
> > > > We can make edges a superset of specialEdges, then we can just delete `seen` and use `edges.try_emplace(...)` here.
> > > We probably could but looks like it also makes sense to keep them distinctive sets, so I'm leaning towards not doing that.
> > It's not a strong preference, but error is perhaps better than printing it out as a non-error message? When the control reaches here, something is wrong from the user's perspective.
> `error` inhibits producing an output. I have a feeling that retaining the output may be slightly more useful.
> 
> Sometimes the user can specify multiple --explain. The user may still need the output though some of the --explain values are not existent.
I guess that's actually a good thing, no? I want users to use this option only interactively just by appending an --explain to an otherwise complete command line, and if a command line option passed by a user is not correct, that is, well, I think an error.

================
Comment at: lld/ELF/Explain.cpp:168
+
+  if (auto *sym = dyn_cast_or_null<Defined>(symtab->find(name))) {
+    outs() << "Explain: Symbol '" << name << "' is defined in file '"
----------------
MaskRay wrote:
> peter.smith wrote:
> > Global symbols defined by a comdat group may cause some confusion as there will be multiple object files that define the symbol, but only one group that is selected that will match here. Not a lot that we can do here.  Perhaps if we knew that a symbol was defined in a group then we could print that out. "Explain: Symbol foo is defined in COMDAT group g, selected from from file foo.o". Perhaps wait and see if anyone does get confused.
> Explaining more about COMDAT groups looks good to me. We essentially discarded all but one definitions, lost some information in the transformation, and the best we can do here is to select the prevailing definition. The user is hinted from the message that the archive order may matter.
> 
> A COMDAT group is a SHT_GROUP section with the ELF group section flag GRP_COMDAT. Its name is almost always ".group". Shall we say `the COMDAT group with signature "g"`?
Currently it is not easy to know whether a section was in a comdat group or not. Fortunately, I don't think that's too confusing, at least we don't tell a lie to users. If we report that there's some dependency from one file to another, that dependency does exist in the source code, though other files may have similar dependencies to the same file. So I don't think we need to do something special for comdats.

================
Comment at: lld/ELF/Explain.cpp:175
+  error("--explain: no such file or symbol: '" + name +
+        "'. Use --verbose option to see a list of input files.");
+  return nullptr;
----------------
MaskRay wrote:
> Is `--trace` better? --verbose suggests the raw archive names while --trace does not, and the --trace output does not include unused files.
> 
> ```
> # --verbose
> ld.lld: a.a
> ld.lld: b.o
> ld.lld: a.a(a.o)
> ```
> 
> ```
> # --trace
> b.o
> a.a(a.o)
> ```
Thank you for the suggestion. I replaced --verbose with --trace.

================
Comment at: lld/ELF/Options.td:47
+    Eq<"explain", "Explain why a given file gets linked to the final binary">,
+    MetaVarName<"<path>">;
+
----------------
MaskRay wrote:
> peter.smith wrote:
> > A suggestion:
> > 
> > ```
> > Explain why a given object file gets linked in to the final binary. Objects can be specified by full path, or by a global symbol that they define. Objects defined in archives are specified by full/path/to/library(object) such as library.a(object.o).
> > ```
> Unfortunately this (lld::elf::InputFile::archiveName) is not the full path. Here are some examples:
> 
> ```
> ld.lld a.a => a.a
> ld.lld -L. -la => ./liba.a
> ld.lld /tmp/c/a.a => /tmp/c/a.a
> ```
> 
> So we may need path normalization here. Some users may resolve the path and feed that to lld.
Added that text to the man page, thanks!

================
Comment at: lld/ELF/Options.td:47
+    Eq<"explain", "Explain why a given file gets linked to the final binary">,
+    MetaVarName<"<path>">;
+
----------------
ruiu wrote:
> MaskRay wrote:
> > peter.smith wrote:
> > > A suggestion:
> > > 
> > > ```
> > > Explain why a given object file gets linked in to the final binary. Objects can be specified by full path, or by a global symbol that they define. Objects defined in archives are specified by full/path/to/library(object) such as library.a(object.o).
> > > ```
> > Unfortunately this (lld::elf::InputFile::archiveName) is not the full path. Here are some examples:
> > 
> > ```
> > ld.lld a.a => a.a
> > ld.lld -L. -la => ./liba.a
> > ld.lld /tmp/c/a.a => /tmp/c/a.a
> > ```
> > 
> > So we may need path normalization here. Some users may resolve the path and feed that to lld.
> Added that text to the man page, thanks!
OK, I made a change to code so that we normalize pathnames before comparison.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69607