[PATCH] D77522: [ELF] --warn-backrefs: don't warn for linking sandwich problems

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added inline comments.


================
Comment at: lld/ELF/Symbols.cpp:381
+    warn(it.second);
+  backrefDiags.clear();
+}
----------------
grimar wrote:
> > backrefDiags.clear();
> 
> Should it be called before populating the `backrefDiags` and not after? I guess it might be
> possible when using LLD as a library to return !0 error code after `backrefDiags` was filled, but before reporting and cleaning.
> 
> We initialize a few things like this in 
> 
> ```
> bool link(ArrayRef<const char *> args, bool canExitEarly, raw_ostream &stdoutOS,
>           raw_ostream &stderrOS)
> ```
> 
> We have the known mess with the global variables. Should we at least stop adding them at random places and stick to initializing
> new ones in the single place, like in the `link()`?
Thanks for pushing back on this :)

I should have stopped being lazy and do it correctly.


================
Comment at: lld/ELF/Symbols.cpp:530
+           toString(other.file) + " refers to " + toString(file))
+              .str());
+    }
----------------
grimar wrote:
> Looking on this, my first impression was that probably since you now have the `reportBackrefs` method,
> you could move the creation and printing of the warning there probably somehow.
> (i.e. leave `backrefDiags.try_emplace` here, but change the map value from `std::string` to something else probably).
> This should having excessive string allocations.
Thanks for the tip. I am not very concerned about the string allocation because every lazy input file there can be at most one such symbol. It looks like changing the signature of the DenseMap will beautify the code, as well as addressing the concern. So I will fix it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77522