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

[MaskRay]

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.

[MaskRay]

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]

Fix test

[MaskRay]

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

[MaskRay]

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

[MaskRay]

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

[MaskRay]

Fix test

[tmsriram]

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]

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

[MaskRay]

Ping:)

[psmith]

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.

[MaskRay]

In D77512#1982890, @psmith wrote:

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]

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.

lld/ELF/Config.h

 #include "lld/Common/ErrorHandler.h"
 #include "llvm/ADT/MapVector.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/ADT/StringSet.h"
 #include "llvm/BinaryFormat/ELF.h"
 #include "llvm/Support/CachePruning.h"
 #include "llvm/Support/CodeGen.h"
 #include "llvm/Support/Endian.h"
+#include "llvm/Support/GlobPattern.h"
 #include <atomic>
 #include <vector>
 
 namespace lld {
 namespace elf {
 
 class InputFile;
 class InputSectionBase;
struct Configuration {
   bool thinLTOEmitImportsFiles;
   bool thinLTOIndexOnly;
   bool timeTraceEnabled;
   bool tocOptimize;
   bool undefinedVersion;
   bool unique;
   bool useAndroidRelrTags = false;
   bool warnBackrefs;
+  std::vector<llvm::GlobPattern> warnBackrefsExclude;
   bool warnCommon;
   bool warnIfuncTextrel;
   bool warnMissingEntry;
   bool warnSymbolOrdering;
   bool writeAddends;
   bool zCombreloc;
   bool zCopyreloc;
   bool zForceBti;

lld/ELF/Driver.cpp

if (auto *arg = args.getLastArg(OPT_retain_symbols_file)) {
     config->versionDefinitions[VER_NDX_LOCAL].patterns.push_back(
         {"*", /*isExternCpp=*/false, /*hasWildcard=*/true});
     if (Optional<MemoryBufferRef> buffer = readFile(arg->getValue()))
       for (StringRef s : args::getLines(*buffer))
         config->versionDefinitions[VER_NDX_GLOBAL].patterns.push_back(
             {s, /*isExternCpp=*/false, /*hasWildcard=*/false});
   }
 
+  for (opt::Arg *arg : args.filtered(OPT_warn_backrefs_exclude)) {
+    StringRef pattern(arg->getValue());
+    if (Expected<GlobPattern> pat = GlobPattern::create(pattern))
+      config->warnBackrefsExclude.push_back(std::move(*pat));
+    else
+      error(arg->getSpelling() + ": " + toString(pat.takeError()));
+  }
+
   // Parses -dynamic-list and -export-dynamic-symbol. They make some
   // symbols private. Note that -export-dynamic takes precedence over them
   // as it says all symbols should be exported.
   if (!config->exportDynamic) {
     for (auto *arg : args.filtered(OPT_dynamic_list))
       if (Optional<MemoryBufferRef> buffer = readFile(arg->getValue()))
         readDynamicList(*buffer);
 

lld/ELF/Options.td

 def version: F<"version">, HelpText<"Display the version number and exit">;
 
 defm version_script: Eq<"version-script", "Read a version script">;
 
 defm warn_backrefs: B<"warn-backrefs",
     "Warn about backward symbol references to fetch archive members",
     "Do not warn about backward symbol references to fetch archive members (default)">;
 
+defm warn_backrefs_exclude
+    : Eq<"warn-backrefs-exclude",
+         "Glob describing an archive (or an object file within --start-lib) "
+         "which should be ignored for --warn-backrefs.">,
+      MetaVarName<"<glob>">;
+
 defm warn_common: B<"warn-common",
     "Warn about duplicate common symbols",
     "Do not warn about duplicate common symbols (default)">;
 
 defm warn_ifunc_textrel: B<"warn-ifunc-textrel",
     "Warn about using ifunc symbols with text relocations",
     "Do not warn about using ifunc symbols with text relocations (default)">;
 

lld/ELF/Symbols.cpp

 #include "InputSection.h"
 #include "OutputSections.h"
 #include "SyntheticSections.h"
 #include "Target.h"
 #include "Writer.h"
 #include "lld/Common/ErrorHandler.h"
 #include "lld/Common/Strings.h"
 #include "llvm/ADT/STLExtras.h"
+#include "llvm/Support/FileSystem.h"
 #include "llvm/Support/Path.h"
 #include <cstring>
 
 using namespace llvm;
 using namespace llvm::object;
 using namespace llvm::ELF;
 
 namespace lld {
if (isLazy()) {
     //
     //   ld.lld A B --start-group C D --end-group E
     //
     // A forms group 0. B form group 1. C and D (including their member object
     // files) form group 2. E forms group 3. I think that you can see how this
     // group assignment rule simulates the traditional linker's semantics.
     bool backref = config->warnBackrefs && other.file &&
                    file->groupId < other.file->groupId;
+    if (backref) {
+      // Some libraries have known problems and can cause noise. Filter them out
+      // with --warn-backrefs-exclude=.
+      StringRef name =
+          !file->archiveName.empty() ? file->archiveName : file->getName();
+      for (const llvm::GlobPattern &pat : config->warnBackrefsExclude)
+        if (pat.match(name)) {
+          backref = false;
+          break;
+        }
+    }
     fetch();
 
     // We don't report backward references to weak symbols as they can be
     // overridden later.
     //
     // A traditional linker does not error for -ldef1 -lref -ldef2 (linking
     // sandwich), where def2 may or may not be the same as def1. We don't want
     // to warn for this case, so dismiss the warning if we see a subsequent lazy

lld/docs/ld.lld.1

 Verbose mode.
 .It Fl -version-script Ns = Ns Ar file
 Read version script from
 .Ar file .
 .It Fl -warn-backrefs
 Warn about reverse or cyclic dependencies to or between static archives.
 This can be used to ensure linker invocation remains compatible with
 traditional Unix-like linkers.
+.It Fl -warn-backrefs-exclude Ns = Ns Ar glob
+Glob describing an archive (or an object file within --start-lib)
+which should be ignored for
+.Fl -warn-backrefs
 .It Fl -warn-common
 Warn about duplicate common symbols.
 .It Fl -warn-ifunc-textrel
 Warn about using ifunc symbols in conjunction with text relocations.
 Older versions of glibc library (2.28 and earlier) has a bug that causes
 the segment that includes ifunc symbols to be marked as not executable when
 they are relocated.
 As a result, although the program compiles and links

lld/test/ELF/warn-backrefs.s

 # RUN: ld.lld --fatal-warnings %t1.o %t2.a -o /dev/null
 # RUN: ld.lld --fatal-warnings --warn-backrefs %t1.o %t2.a -o /dev/null
 # RUN: ld.lld --fatal-warnings --warn-backrefs %t1.o --start-lib %t2.o --end-lib -o /dev/null
 
 # RUN: echo 'INPUT("%t1.o" "%t2.a")' > %t1.lds
 # RUN: ld.lld --fatal-warnings --warn-backrefs %t1.lds -o /dev/null
 
 ## A backward reference from %t1.o to %t2.a
+## Warn unless the archive is excluded by --warn-backrefs-exclude
 # RUN: ld.lld --fatal-warnings %t2.a %t1.o -o /dev/null
 # RUN: ld.lld --warn-backrefs %t2.a %t1.o -o /dev/null 2>&1 | FileCheck %s
 # RUN: ld.lld --warn-backrefs %t2.a '-(' %t1.o '-)' -o /dev/null 2>&1 | FileCheck %s
+# RUN: ld.lld --warn-backrefs --warn-backrefs-exclude='*3.a' %t2.a %t1.o -o /dev/null 2>&1 | FileCheck %s
+# RUN: ld.lld --fatal-warnings --warn-backrefs --warn-backrefs-exclude='*2.a' %t2.a %t1.o -o /dev/null
+# RUN: ld.lld --fatal-warnings --warn-backrefs --warn-backrefs-exclude '*2.a' \
+# RUN:   --warn-backrefs-exclude not_exist %t2.a %t1.o -o /dev/null
+## Without --warn-backrefs, --warn-backrefs-exclude is ignored.
+# RUN: ld.lld --fatal-warnings --warn-backrefs-exclude=not_exist %t2.a %t1.o -o /dev/null
 
 ## Placing the definition and the backward reference in a group can suppress the warning.
 # RUN: echo 'GROUP("%t2.a" "%t1.o")' > %t2.lds
 # RUN: ld.lld --fatal-warnings --warn-backrefs %t2.lds -o /dev/null
 # RUN: ld.lld --fatal-warnings --warn-backrefs '-(' %t2.a %t1.o '-)' -o /dev/null
 
 ## A backward reference from %t1.o to %t2.a (added by %t3.lds).
 # RUN: echo 'GROUP("%t2.a")' > %t3.lds
 # RUN: ld.lld --warn-backrefs %t3.lds %t1.o -o /dev/null 2>&1 | FileCheck %s
 # RUN: ld.lld --fatal-warnings --warn-backrefs '-(' %t3.lds %t1.o '-)' -o /dev/null
+# RUN: ld.lld --fatal-warnings --warn-backrefs --warn-backrefs-exclude='*2.a' -o /dev/null %t3.lds %t1.o
 ## If a lazy definition appears after the backward reference, don't warn.
 # RUN: ld.lld --fatal-warnings --warn-backrefs %t3.lds %t1.o %t3.lds -o /dev/null
 
 # CHECK: warning: backward reference detected: foo in {{.*}}1.o refers to {{.*}}2.a
 
 ## A backward reference from %t1.o to %t2.o
+## --warn-backrefs-exclude= applies to --start-lib covered object files.
 # RUN: ld.lld --warn-backrefs --start-lib %t2.o --end-lib %t1.o -o /dev/null 2>&1 | \
 # RUN:   FileCheck --check-prefix=OBJECT %s
+# RUN: ld.lld --fatal-warnings --warn-backrefs --warn-backrefs-exclude=%/t2.o --start-lib %/t2.o --end-lib %t1.o -o /dev/null
 ## If a lazy definition appears after the backward reference, don't warn.
 # RUN: ld.lld --fatal-warnings --warn-backrefs --start-lib %t2.o --end-lib %t1.o --start-lib %t2.o --end-lib -o /dev/null
 
 # OBJECT: warning: backward reference detected: foo in {{.*}}1.o refers to {{.*}}2.o
 
 ## Don't warn if the definition and the backward reference are in a group.
 # RUN: echo '.globl bar; bar:' | llvm-mc -filetype=obj -triple=x86_64 - -o %t3.o
 # RUN: echo '.globl foo; foo: call bar' | llvm-mc -filetype=obj -triple=x86_64 - -o %t4.o

 # RUN: ld.lld --fatal-warnings --warn-backrefs %t2.a %t1.o %t2.so %t2.a -o /dev/null
 
 ## In GNU linkers, -u does not make a backward reference.
 # RUN: ld.lld --fatal-warnings --warn-backrefs -u foo %t2.a %t1.o -o /dev/null
 
 ## In GNU gold, --export-dynamic-symbol does not make a backward reference.
 # RUN: ld.lld --fatal-warnings --warn-backrefs --export-dynamic-symbol foo %t2.a %t1.o -o /dev/null
 
+# RUN: not ld.lld --warn-backrefs-exclude='[' 2>&1 | FileCheck --check-prefix=INVALID %s
+# INVALID: error: --warn-backrefs-exclude: invalid glob pattern: [
+
 .globl _start, foo
 _start:
   call foo