Page MenuHomePhabricator

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

Authored by MaskRay on Apr 5 2020, 11:12 PM.

Details

Summary

This is an alternative design to D77512.

D45195 added --warn-backrefs to detect

  • A. certain input orders which GNU ld either errors ("undefined reference") or has different resolution semantics
  • B. (byproduct) some latent multiple definition problems (-ldef1 -lref -ldef2) which I call "linking sandwich problems". def2 may or may not be the same as def1.

When an archive appears more than once (-ldef -lref -ldef), lld and GNU
ld may have the same resolution but --warn-backrefs may warn. This is
not uncommon. For example, currently lld itself has such a problem:

liblldCommon.a liblldCOFF.a ... liblldCommon.a
  _ZN3lld10DWARFCache13getDILineInfoEmm in liblldCOFF.a refers to liblldCommon.a(DWARF.cpp.o)
libLLVMSupport.a also appears twice and has a similar warning

glibc has such problems. It is somewhat destined because of its separate
libc/libpthread/... and arbitrary grouping. The situation is getting
improved over time but I have seen:

-lc __isnanl references -lm
-lc _IO_funlockfile references -lpthread

There are also various issues in interaction with other runtime
libraries such as libgcc_eh and libunwind:

-lc __gcc_personality_v0 references -lgcc_eh
-lpthread __gcc_personality_v0 references -lgcc_eh
-lpthread _Unwind_GetCFA references -lunwind

These problems are actually benign. We want --warn-backrefs to focus on
its main task A and defer task B (which is also useful) to a more
specific future feature (see gold --detect-odr-violations).

Instead of warning immediately, we store the message and only report it
if no subsequent lazy definition exists.

The use of the static variable backrefDiags is similar to undefs in
Relocations.cpp

Diff Detail

Event Timeline

MaskRay created this revision.Apr 5 2020, 11:12 PM
MaskRay added a comment.EditedApr 5 2020, 11:20 PM

I will try this in our internal large code base and ask Android/ChromeOS folks for help as well. If this goes well (little or no false positive), I hope we can eventually make --warn-backrefs the default.
Here for "false positive" I mean --warn-backrefs warns but our symbol resolution is actually the same as GNU ld's.

grimar added inline comments.Apr 6 2020, 5:33 AM
lld/ELF/Symbols.cpp
382

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()?

537

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.

MaskRay updated this revision to Diff 255479.Apr 6 2020, 1:43 PM
MaskRay marked 4 inline comments as done.

Address comments

lld/ELF/Symbols.cpp
382

Thanks for pushing back on this :)

I should have stopped being lazy and do it correctly.

537

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.

grimar accepted this revision.Apr 7 2020, 3:55 AM

LGTM, thanks!

lld/ELF/Symbols.h
562

Ussd -> Used

lld/test/ELF/warn-backrefs.s
55

Seems you make this comment to be 2 lines.

This revision is now accepted and ready to land.Apr 7 2020, 3:55 AM
grimar added inline comments.Apr 7 2020, 3:55 AM
lld/test/ELF/warn-backrefs.s
55

*you can make

MaskRay updated this revision to Diff 255714.Apr 7 2020, 10:19 AM
MaskRay marked 3 inline comments as done.

Address comments

This revision was automatically updated to reflect the committed changes.