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

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added inline comments.


================
Comment at: lld/ELF/Explain.cpp:157
+
+  if (!seen.insert(sym.file).second)
+    return;
----------------
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.


================
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 '"
----------------
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"`?


================
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;
----------------
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)
```


================
Comment at: lld/ELF/Options.td:47
+    Eq<"explain", "Explain why a given file gets linked to the final binary">,
+    MetaVarName<"<path>">;
+
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69607