Page MenuHomePhabricator

[ELF] Add --warn-backrefs-exclude=<glob>
ClosedPublic

Authored by MaskRay on Apr 5 2020, 1:19 PM.

Details

Summary

D77522 changed --warn-backrefs to not warn for linking sandwich
problems (-ldef1 -lref -ldef2). This removed lots of false positives.

However, glibc still has some problems. libc.a defines some symbols
which are normally in libm.a and libpthread.a, e.g. __isnanl/raise.

For a linking order -lm -lpthread -lc, I have seen:

// different resolutions: GNU ld/gold select libc.a(s_isnan.o) as the definition
backward reference detected: __isnanl in libc.a(printf_fp.o) refers to libm.a(m_isnanl.o)

// different resolutions: GNU ld/gold select libc.a(raise.o) as the definition
backward reference detected: raise in libc.a(abort.o) refers to libpthread.a(pt-raise.o)

To facilitate deployment of --warn-backrefs, add --warn-backrefs-exclude= so that
certain known issues (which may be impractical to fix) can be whitelisted.

Deliberate choices:

  • Not a comma-separated list (--warn-backrefs-exclude=liba.a,libb.a). -Wl, splits the argument at commas, so we cannot use commas. --export-dynamic-symbol is similar.
  • Not in the style of --warn-backrefs='*' --warn-backrefs=-liba.a. We just need exclusion, not inclusion. For easier build system integration, we should avoid order dependency. With the current scheme, we enable --warn-backrefs, and indivial libraries can add --warn-backrefs-exclude=<glob> to their LDFLAGS.

Diff Detail

Event Timeline

MaskRay created this revision.Apr 5 2020, 1:19 PM
MaskRay marked an inline comment as done.Apr 5 2020, 1:22 PM
MaskRay added inline comments.
lld/docs/ld.lld.1
577

I know the wording of --warn-backrefs-exclude is subject to change. So I just left a stub here.

MaskRay updated this revision to Diff 255209.Apr 5 2020, 4:38 PM
MaskRay edited the summary of this revision. (Show Details)

Fix test

With more thoughts, I'll probably abandon this patch and go for D77522, but I'll keep it open to collect some feedback.

MaskRay updated this revision to Diff 255905.Apr 7 2020, 11:42 PM
MaskRay edited the summary of this revision. (Show Details)

With some experiments, I believe this change is still useful (yes, D77522+D77640 are not sufficient for glibc...)

MaskRay updated this revision to Diff 255907.Apr 7 2020, 11:53 PM
MaskRay edited the summary of this revision. (Show Details)

Use Eq<> instead of J<> for consistency with --export-dynamic-symbol

tmsriram added inline comments.
lld/ELF/Options.td
406

Just a drive by observation. Why not --no-warn-backrefs=? Basically the same semantics except --no-warn-backrefs= instead of --warn-backrefs-exclude=

MaskRay marked an inline comment as done.Apr 12 2020, 8:04 AM
MaskRay added inline comments.
lld/ELF/Options.td
406

--no-warn-backrefs is the opposite of --warn-backrefs.

Adding --no-warn-backrefs= may be confusing. A different stem name makes it slightly clearer there is no order dependency between --warn-backrefs-exclude= and --warn-backrefs

Apologies for the delay in responding, just coming back from the Easter break.

  • The user interface for system libraries is often --library or -l, I expect back refs from system libraries to be one of the most common exclusions as these won't be under the users control. Is it worth having --warn-backrefs-exclude mimic that interface. Perhaps even worth --warn-backrefs-exclude-searched that excludes all libraries found via --library. Maybe too blunt for many, but it may be the most common case.
  • The library name might not be obvious if it is a linker script like libc that expands to several libraries. Not sure how to solve this one.
  • library filenames may not be unique in some large projects, may need to add the full path to the library. If this already works maybe worth a test.

These aren't strong opinions, user interfaces aren't something I've been particular good at.

Apologies for the delay in responding, just coming back from the Easter break.

Thanks for making suggestions!

  • The user interface for system libraries is often --library or -l, I expect back refs from system libraries to be one of the most common exclusions as these won't be under the users control. Is it worth having --warn-backrefs-exclude mimic that interface. Perhaps even worth --warn-backrefs-exclude-searched that excludes all libraries found via --library. Maybe too blunt for many, but it may be the most common case.

Excluding all libraries via --library can be both coarse and insufficient.

Too coarse: many build systems use -lfoo instead of path/to/libfoo.a. The backward reference checking feature is unnecessarily removed for such users.

Insufficient: clang driver passes the full path to some runtime libraries, e.g. path/to/clang_rt.safestack-x86_64.a I noticed that library may have a backref problem. More likely it is a problem of our internal (complex) build system, but this may still be a nice example that a system not caring (enough) about GNU linkers can get really difficult to use GNU toolchain at all... I occasionally link programs with gold to cross validate lld. Having a second viable toolchain has a number of advantages. It would be a great loss if projects get really difficult to be linked with GNU linkers.. Making projects overly rely on lld can make it difficult to improve lld as well.
As a positive example, I implemented --no-allow-shlib-undefined to detect compatibility problems. This requires huge efforts cleaning up our internal code base. Thankfully most projects are clean now. Now the backward reference problem is another issue and I noticed that the situation is getting worse but is still fixable.

So really, I created the patch for a selfish purpose but I am hoping it can be general and useful enough and may be adopted by other projects: Android, ChromeOS, Fuchsia (separator; the list on the left are really projects of my company and sometimes their reliance on lld makes it difficult to improve things; as you may have noticed, I got pretty unhappy when Fuchsia people argued against default -z noseparate-code which I think is a clear community favorable choice) FreeBSD, some Linux distributions which use LLVM toolchain.

  • The library name might not be obvious if it is a linker script like libc that expands to several libraries. Not sure how to solve this one.

Yeah, --warn-backrefs-exclude-searched can still be insufficient but a generic pattern base blacklist can solve the problem.

  • library filenames may not be unique in some large projects, may need to add the full path to the library. If this already works maybe worth a test.

These aren't strong opinions, user interfaces aren't something I've been particular good at.

In the tests, the full path --warn-backrefs-exclude=%/t2.o is to check that the full path is considered.

psmith accepted this revision.Apr 16 2020, 1:38 AM

Thanks for the response. If this is aimed more at advanced rather than casual users then I'm happy with the interface. Maybe worth waiting to early next week to see if there are any more comments, people may be on Easter break.

As an aside I think LLD has reached the point where there is quite a diverse user base and Hyrum's law now applies. Pretty much everything we change is going to upset someone depending on it. I think this means that we have to be thinking more about stability and to give people tools to find out where there may be incompatibilities so I think it is good to have features like these.

This revision is now accepted and ready to land.Apr 16 2020, 1:38 AM
This revision was automatically updated to reflect the committed changes.