[LLD][ELF] Warn on invalid local symbols

[thevinster]

Summary

Some of our apps are failing to link properly with third-party libraries
because this warning has been converted to an error. In principle, we
shouldn't allow this sort of behavior, but I don't see any reasons for
error-ing since it hasn't caused any real issues for us in the past. For
those who wish to keep it as an error, users may provide -fatal-warnings
to keep the existing behavior.

[thevinster]

@MaskRay This is effectively a revert of https://github.com/llvm/llvm-project/commit/3ac94280245415be66cb1b603367c5f4f6d498e7 (but keeping the message intact). Do you have any suggestions how to get around this specific issue?

[MaskRay]

This is shared object parsing and the performance is less important. If it is important, it may be attempting to remove this error check to have less code in the hot path and just assume that all non-local symbols are not STB_LOCAL. Before we do that, we probably want this to be an error for sufficiently long time.

I wonder whether you can consider binary patching the broken shared objects. I have done similar things for my employer (for symbol versioning, not for this STB_LOCAL instance) as well..
If binary patching doesn't look great, use --noinhibit-exec.

[thevinster]

I wonder whether you can consider binary patching the broken shared objects. I have done similar things for my employer (for symbol versioning, not for this STB_LOCAL instance) as well..
If binary patching doesn't look great, use --noinhibit-exec.

Patching the binary is something we're probably going to try and pursue, but I wanted to get your thoughts first whether you feel strongly against switching this back to a warning. Using --noinhibit-exec relaxes other errors as well and would prefer not to do that to avoid more broken shared objects from creeping. So far, it's just this specific message that I've seen so far.

[MaskRay]

In D155989#4523745, @thevinster wrote:

I wonder whether you can consider binary patching the broken shared objects. I have done similar things for my employer (for symbol versioning, not for this STB_LOCAL instance) as well..
If binary patching doesn't look great, use --noinhibit-exec.

Patching the binary is something we're probably going to try and pursue, but I wanted to get your thoughts first whether you feel strongly against switching this back to a warning. Using --noinhibit-exec relaxes other errors as well and would prefer not to do that to avoid more broken shared objects from creeping. So far, it's just this specific message that I've seen so far.

errorOrWarn for this instance seems to be better, for (a) the reason I stated in a previous comment, and (b) to match the errorOrWarn binding check ObjFile<EDLFT>::postParse (though we may want to remove that diagnostic completely to remove code from a hot patch in the future)...

lld/ELF/InputFiles.cpp

for (size_t i = 0, e = syms.size(); i != e; ++i) {
     const Elf_Sym &sym = syms[i];
 
     // ELF spec requires that all local symbols precede weak or global
     // symbols in each symbol table, and the index of first non-local symbol
     // is stored to sh_info. If a local symbol appears after some non-local
     // symbol, that's a violation of the spec.
     StringRef name = CHECK(sym.getName(stringTable), this);
     if (sym.getBinding() == STB_LOCAL) {
-      errorOrWarn(toString(this) + ": invalid local symbol '" + name +
+      warn(toString(this) + ": invalid local symbol '" + name +
                   "' in global part of symbol table");
       continue;
     }
 
     const uint16_t ver = versyms[i], idx = ver & ~VERSYM_HIDDEN;
     if (sym.isUndefined()) {
       // For unversioned undefined symbols, VER_NDX_GLOBAL makes more sense but
       // as of binutils 2.34, GNU ld produces VER_NDX_LOCAL.

lld/test/ELF/invalid-local-symbol-in-dso.s

 # REQUIRES: x86
 
 # We used to crash on this
-# RUN: not ld.lld %p/Inputs/local-symbol-in-dso.so -o /dev/null 2>&1 | FileCheck %s
-# CHECK: error: {{.*}}local-symbol-in-dso.so: invalid local symbol 'foo' in global part of symbol table
+# RUN: ld.lld %p/Inputs/local-symbol-in-dso.so -o /dev/null 2>&1 | FileCheck %s
+# CHECK: warning: {{.*}}local-symbol-in-dso.so: invalid local symbol 'foo' in global part of symbol table
 
 # RUN: llvm-mc %s -o %t.o -filetype=obj -triple x86_64-pc-linux
 # RUN: not ld.lld %t.o %p/Inputs/local-symbol-in-dso.so -o /dev/null
 
 .globl main
 main:
   movq foo@GOTTPOFF(%rip), %rax

lld/test/ELF/invalid/undefined-local-symbol-in-dso.test

 # REQUIRES: x86
 
 # LLD used to crash when linking against a DSO with an undefined STB_LOCAL
 # symbol in the global part of the dynamic symbol table (i.e. an STB_LOCAL
 # symbol with an index >= the sh_info of the dynamic symbol table section). Such
 # a DSO is very broken, because local symbols should precede all global symbols
 # in the symbol table, and because having a symbol that's both undefined and
-# STB_LOCAL is a nonsensical combination. Nevertheless, we should error on such
+# STB_LOCAL is a nonsensical combination. Nevertheless, we should warn on such
 # input files instead of crashing.
 
 # RUN: yaml2obj %s -o %t.so
-# RUN: not ld.lld %t.so -o /dev/null 2>&1 | FileCheck %s
-# CHECK: error: {{.*}}.so: invalid local symbol 'foo' in global part of symbol table
+# RUN: ld.lld %t.so -o /dev/null 2>&1 | FileCheck %s
+# CHECK: warning: {{.*}}.so: invalid local symbol 'foo' in global part of symbol table
 
 --- !ELF
 FileHeader:
   Class:           ELFCLASS64
   Data:            ELFDATA2LSB
   Type:            ET_DYN
   Machine:         EM_X86_64
 Sections:
   - Name:          .dynsym
     Info:          1
     Type:          SHT_DYNSYM
 DynamicSymbols:
   - Name: foo