[PATCH] D63344: lld/elf: Deduplicate undefined symbol diagnostics

[Nico Weber via Phabricator via llvm-commits]

thakis added a comment.

> How long is the increase?

That's a good question. Intuitively I'd say it shouldn't be all that much: Undefined diags were emitted doing relocation scanning, which I think happens fairly late in the link, and now it happens after it. So the max latency added is the time it takes to scan relocations. I'd expect that relocation scanning time is small compared to total link time. But I'll measure and report back.



================
Comment at: lld/ELF/Relocations.cpp:718
+  std::vector<Loc> Locs;
+  bool IsWarning;
+};
----------------
grimar wrote:
> MaskRay wrote:
> > grimar wrote:
> > > Seems this field can be calculated at any time, does it worth to keep the precalculated value?
> > `IsWarning` is computed on a per-symbol basis:
> > 
> > ```
> > bool CanBeExternal = !Sym.isLocal() && Sym.computeBinding() != STB_LOCAL &&
> >                        Sym.Visibility == STV_DEFAULT;
> > 
> >   bool IsWarning =
> >       (Config->UnresolvedSymbols == UnresolvedPolicy::Warn && CanBeExternal) ||
> >       Config->NoinhibitExec;
> > 
> > ```
> Yes and we have `Symbol * UndefinedDiag::Sym` here, so can calculate it on fly I think.
It's needed as a return value; doing the computation twice seems less good to me?


================
Comment at: lld/ELF/Relocations.cpp:778
+  // collect all "referenced from" lines at the first diagnostic.
+  std::map<Symbol*, size_t> FirstRef;
+  for (size_t i = 0; i < Undefs.size(); ++i) {
----------------
grimar wrote:
> I'd use llvm::DenseMap. It is probably a bit more consistent with the rest LLD ELF code,
> and allows then to use its `lookup` method below:
> 
> ```
> if (Symbol *S = FirstRef.lookup(Undef.Sym)) {
> ...
> }
> ```
Done, I think. (I had to change the value type to UndefinedDiag, and do `if (UndefinedDiag *...` to make it work; that's my best guess at what you meant. Maybe I got it wrong.)


================
Comment at: lld/test/ELF/undef-multi.s:17
+
+# All references to a single undefined symbol count as a single error -- but
+# at most 10 references are printed.
----------------
MaskRay wrote:
> Just a suggestion. It is now common to use `## ` for comments that are not RUN/CHECK lines. 
```
$ git grep -l '^# ' lld/test/ | wc -l
    1388
$ git grep -l '^## ' lld/test/ | wc -l
     201
```

In LLVM, it's 4766/176.

So it's an order of magnitude more common to not do this. Unless there's an effort to rewrite all of llvm's existing tests to this new style and enforce it, using ## for comments seems like it decreases llvm's self-consistency and I suggest everyone adding comments like this to stop doing it.


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

https://reviews.llvm.org/D63344