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

[Rui Ueyama via Phabricator via llvm-commits]

ruiu added a comment.

I agree that that's perhaps better to omit an error message as `>> and 532 more references` if it is too large, but I don't know how large it can be. Can this be hundreds of lines?



================
Comment at: lld/ELF/Relocations.cpp:720
+};
+static std::vector<UndefinedDiag>& Undefs() {
+  // Function-local static to not require a global initializer.
----------------
nit: add a blank line

In lld we honestly don't care too much about eliminating global initializers, so it can probably be just a global variable, but you need to erase the contents before the first use so that multiple runs of lld::elf::main in the same process works as expected.


================
Comment at: lld/ELF/Relocations.cpp:784
+
+  for (const auto& Undef : Undefs()) {
+    if (!Undef.Locs.empty())
----------------
Replace `auto` with a concrete type.


================
Comment at: lld/ELF/Relocations.cpp:814-815
+  UndefinedDiag Undef = { &Sym, {{&Sec, Offset}}, /*IsWarning=*/false };
+  if ((Config->UnresolvedSymbols == UnresolvedPolicy::Warn && CanBeExternal) ||
+      Config->NoinhibitExec)
+    Undef.IsWarning = true;
----------------
You can define `bool IsWarning` instead of constructing an entire `Undef` here


================
Comment at: lld/ELF/Relocations.cpp:818
+
+  {
+    static std::mutex Mu;
----------------
Is this really called by a multithreaded code?


================
Comment at: lld/ELF/Relocations.cpp:821
+    std::lock_guard<std::mutex> Lock(Mu);
+    Undefs().push_back(Undef);
   }
----------------
Then you can construct an UndefinedDiag as an unnamed struct

  Undefs().push_back({&Sym, {{&Sec, Offset}}, IsWarning});


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

https://reviews.llvm.org/D63344