[ELF] Demote lazy symbols relative to a discarded section to Undefined

[MaskRay]

Summary

Fixes PR45594.

In ObjFile<ELFT>::initializeSymbols(), for a defined symbol relative to
a discarded section (due to section group rules), it may have been
inserted as a lazy symbol. We need to demote it to an Undefined to
enable the discarded section error happened in a later pass.

Add LazyObjFile::fetched (if true) and ArchiveFile::parsed (if false) to represent that there is an
ongoing lazy symbol fetch and we should replace the current lazy symbol
with an Undefined, instead of calling Symbol::resolve (the lazy symbol
was added by an unrelated archive/lazy object).

As a side result, one small issue in start-lib-comdat.s is now fixed.
The hack motivating D51892 will be unsupported: if
.gnu.linkonce.t.__i686.get_pc_thunk.bx in an archive is referenced
by another section, this will likely be errored unless the function is
also defined in a regular object file.
(Bringing back rL330869 would error undefined symbol instead of the
more relevant discarded section.)

Note, glibc i386's crti.o still works (PR31215), because
.gnu.linkonce.t.__x86.get_pc_thunk.bx is in crti.o (one of the first
regular object files in a linker command line).

[MaskRay]

lld/ELF/InputFiles.cpp

1592

s/has been processed/is being fetched/

[grimar]

lld/ELF/InputFiles.cpp

1124

I am thinking about adding some flag like isFetched or alike to InputFile instead of using symbols.empty() this and in other places.
(Or perhaps only for ArchiveFile/LazyObjFile`)
It should look cleaner and avoid hacks like push_back(nullptr). What do you think?

1594

You could use std::swap(file->symbols, symbols) instead I think.

lld/test/ELF/comdat-discarded-lazy.s

12

I think it should mention somewhere why it is ordered before/after.
I.e. that symbols are normally ordered by name in the symbol table.

19

Perhaps AA->BEFORE, ZZ->AFTER

[MaskRay]

lld/ELF/InputFiles.cpp

1124

Thanks. I agree that the usage is subtle. Added a member to ArchiveFile and LazyObjFile, respectively.

lld/test/ELF/comdat-discarded-lazy.s

19

It is not clear what the subject and object are when I see BEFORE or AFTER. I will just stick with AA ZZ

[MaskRay]

Address comments

[ruiu]

I wonder in what condition this error occurs. In particular, can you trigger this with standard-compliant object files?

[MaskRay]

In D79300#2033170, @ruiu wrote:

I wonder in what condition this error occurs. In particular, can you trigger this with standard-compliant object files?

Compiled code may not cause such object files because their use of section groups has some patterns. It is not common to see more than one global symbol in a section group.

As @rprichard described in https://bugs.llvm.org//show_bug.cgi?id=45594 , this issue happened with some handwritten assembly files.

[psmith]

lld/ELF/InputFiles.cpp

1124

It took me a while to understand what was going on here. Some suggestions

ArchiveFile::parsed or !LazyObjFile::fetched means that the file containing this object has not finished processing, i.e. this symbol is a result of a lazy symbol fetch. We should demote the lazy symbol to an Undefined so that any relocations to it will trigger a discarded section error.

lld/test/ELF/comdat-discarded-lazy.s

13

Can we mention that the symbols are ordered lexically. Something like

The test relies on the symbol table order of llvm-mc (lexical), which will need ...

[MaskRay]

Update comments

[psmith]

Thanks for the updates, I don't have any more comments. Happy if George is happy.

[ruiu]

There might be a better way of doing this. As far as I know, directly referring a non-group-leader symbol is not allowed, so the problem we have here is to find an object file that violates the spec. Do you think you can directly implement that logic? I mean, when a symbol is being resolved, we can check if a symbol is directly referring a group internal symbol, and if that's the case, we can emit an error as soon as such a bad symbol use is found.

[MaskRay]

In D79300#2035662, @ruiu wrote:

There might be a better way of doing this. As far as I know, directly referring a non-group-leader symbol is not allowed, so the problem we have here is to find an object file that violates the spec. Do you think you can directly implement that logic? I mean, when a symbol is being resolved, we can check if a symbol is directly referring a group internal symbol, and if that's the case, we can emit an error as soon as such a bad symbol use is found.

This approach is the best I can find.

I have thought about restoring rL330869. It can give us an error, but not a specific error like "relocations refer to a symbol in a discarded section". So it is inferior.

Our current symbol resolution errs that some symbols are incorrectly considered Lazy while should be Undefined. This patch fixes the bug. More precisely, some symbols which should be considered undefined according to the spec:

A symbol table entry with STB_GLOBAL or STB_WEAK binding that is defined relative to one of a group's sections, and that is contained in a symbol table section that is not part of the group, must be converted to an undefined symbol (its section index must be changed to SHN_UNDEF) if the group members are discarded. References to this symbol table entry from outside the group are allowed.

lld/ELF/InputFiles.cpp

1124

Thanks the suggestion! Adapted.

[MaskRay]

😀

[MaskRay]

Ping.. I think we should do this.

[psmith]

LGTM, I don't have a better idea.

As I understand it, I don't think that we can detect this case when reading the object. As these symbols are global they could be referred to from outside the group, even another object file (references to a global from outside a group are permitted in ELF). If we scan the relocations in the object we can detect references to the discarded symbol from the object, but not the other objects. We'd be able to detect all relocations to local symbols.

lld/ELF/InputFiles.h

LazyObjFile(MemoryBufferRef m, StringRef archiveName,
     this->archiveName = std::string(archiveName);
   }
 
   static bool classof(const InputFile *f) { return f->kind() == LazyObjKind; }
 
   template <class ELFT> void parse();
   void fetch();
 
+  bool fetched = false;
+
 private:
   uint64_t offsetInArchive;
 };
 
 // An ArchiveFile object represents a .a file.
 class ArchiveFile : public InputFile {
 public:
   explicit ArchiveFile(std::unique_ptr<Archive> &&file);
   static bool classof(const InputFile *f) { return f->kind() == ArchiveKind; }
   void parse();
 
   // Pulls out an object file that contains a definition for Sym and
   // returns it. If the same file was instantiated before, this
   // function does nothing (so we don't instantiate the same file
   // more than once.)
   void fetch(const Archive::Symbol &sym);
 
   size_t getMemberCount() const;
   size_t getFetchedMemberCount() const { return seen.size(); }
 
+  bool parsed = false;
+
 private:
   std::unique_ptr<Archive> file;
   llvm::DenseSet<uint64_t> seen;
 };
 
 class BitcodeFile : public InputFile {
 public:
   BitcodeFile(MemoryBufferRef m, StringRef archiveName,

lld/ELF/InputFiles.cpp

for (size_t i = 0, end = eSyms.size(); i != end; ++i) {
     }
 
     // If a defined symbol is in a discarded section, handle it as if it
     // were an undefined symbol. Such symbol doesn't comply with the
     // standard, but in practice, a .eh_frame often directly refer
     // COMDAT member sections, and if a comdat group is discarded, some
     // defined symbol in a .eh_frame becomes dangling symbols.
     if (sec == &InputSection::discarded) {
-      this->symbols[i]->resolve(
-          Undefined{this, name, binding, stOther, type, secIdx});
+      Undefined und{this, name, binding, stOther, type, secIdx};
+      Symbol *sym = this->symbols[i];
+      // !ArchiveFile::parsed or LazyObjFile::fetched means that the file
+      // containing this object has not finished processing, i.e. this symbol is
+      // a result of a lazy symbol fetch. We should demote the lazy symbol to an
+      // Undefined so that any relocations outside of the group to it will
+      // trigger a discarded section error.
+      if ((sym->symbolKind == Symbol::LazyArchiveKind &&
+           !cast<ArchiveFile>(sym->file)->parsed) ||
+          (sym->symbolKind == Symbol::LazyObjectKind &&
+           cast<LazyObjFile>(sym->file)->fetched))
+        sym->replace(und);
+      else
+        sym->resolve(und);
       continue;
     }
 
     // Handle global defined symbols.
     if (binding == STB_GLOBAL || binding == STB_WEAK ||
         binding == STB_GNU_UNIQUE) {
       this->symbols[i]->resolve(
           Defined{this, name, binding, stOther, type, value, size, sec});
       continue;
     }
 
     fatal(toString(this) + ": unexpected binding: " + Twine((int)binding));
   }
 }
 
 ArchiveFile::ArchiveFile(std::unique_ptr<Archive> &&file)
     : InputFile(ArchiveKind, file->getMemoryBufferRef()),
       file(std::move(file)) {}
 
 void ArchiveFile::parse() {
   for (const Archive::Symbol &sym : file->symbols())
     symtab->addSymbol(LazyArchive{*this, sym});
+
+  // Inform a future invocation of ObjFile<ELFT>::initializeSymbols() that this
+  // archive has been processed.
+  parsed = true;
 }
 
 // Returns a buffer pointing to a member file containing a given symbol.
 void ArchiveFile::fetch(const Archive::Symbol &sym) {
   Archive::Child c =
       CHECK(sym.getMember(), toString(this) +
                                  ": could not get the member for symbol " +
                                  toELFString(sym));
InputFile *createObjectFile(MemoryBufferRef mb, StringRef archiveName,
   case ELF64BEKind:
     return make<ObjFile<ELF64BE>>(mb, archiveName);
   default:
     llvm_unreachable("getELFKind");
   }
 }
 
 void LazyObjFile::fetch() {
-  if (mb.getBuffer().empty())
+  if (fetched)
     return;
+  fetched = true;
 
   InputFile *file = createObjectFile(mb, archiveName, offsetInArchive);
   file->groupId = groupId;
 
-  mb = {};
-
   // Copy symbol vector so that the new InputFile doesn't have to
   // insert the same defined symbols to the symbol table again.
   file->symbols = std::move(symbols);
 
   parseFile(file);
 }
 
 template <class ELFT> void LazyObjFile::parse() {
   using Elf_Sym = typename ELFT::Sym;
 
   // A lazy object file wraps either a bitcode file or an ELF file.
   if (isBitcode(this->mb)) {
     std::unique_ptr<lto::InputFile> obj =
         CHECK(lto::InputFile::create(this->mb), this);
     for (const lto::InputFile::Symbol &sym : obj->symbols()) {

lld/test/ELF/comdat-discarded-lazy.s

+# REQUIRES: x86
+## Test that lazy symbols in a section group can be demoted to Undefined,
+## so that we can report a "discarded section" error.
+
+# RUN: llvm-mc -filetype=obj -triple=x86_64 %s -o %t.o
+# RUN: echo '.globl f1, foo; f1: call foo; \
+# RUN:   .section .text.foo,"axG",@progbits,foo,comdat; foo:' | \
+# RUN:   llvm-mc -filetype=obj -triple=x86_64 - -o %t1.o
+
+## Test the case when the symbol causing a "discarded section" is ordered
+## *before* the symbol fetching the lazy object.
+## The test relies on the symbol table order of llvm-mc (lexical), which will
+## need adjustment if llvm-mc changes its behavior.
+# RUN: echo '.globl f2, aa; f2: call aa; \
+# RUN:   .section .text.foo,"axG",@progbits,foo,comdat; aa:' | \
+# RUN:   llvm-mc -filetype=obj -triple=x86_64 - -o %taa.o
+# RUN: llvm-nm -p %taa.o | FileCheck --check-prefix=AA-NM %s
+# RUN: not ld.lld %t.o --start-lib %t1.o %taa.o --end-lib -o /dev/null 2>&1 | FileCheck --check-prefix=AA %s
+# RUN: rm -f %taa.a && llvm-ar rc %taa.a %taa.o
+# RUN: not ld.lld %t.o --start-lib %t1.o --end-lib %taa.a -o /dev/null 2>&1 | FileCheck --check-prefix=AA %s
+
+# AA-NM: aa
+# AA-NM: f2
+
+# AA:      error: relocation refers to a symbol in a discarded section: aa
+# AA-NEXT: >>> defined in {{.*}}aa.o
+# AA-NEXT: >>> section group signature: foo
+# AA-NEXT: >>> prevailing definition is in {{.*}}1.o
+# AA-NEXT: >>> referenced by {{.*}}aa.o:(.text+0x1)
+
+## Test the case when the symbol causing a "discarded section" is ordered
+## *after* the symbol fetching the lazy object.
+# RUN: echo '.globl f2, zz; f2: call zz; \
+# RUN:   .section .text.foo,"axG",@progbits,foo,comdat; zz:' | \
+# RUN:   llvm-mc -filetype=obj -triple=x86_64 - -o %tzz.o
+# RUN: llvm-nm -p %tzz.o | FileCheck --check-prefix=ZZ-NM %s
+# RUN: not ld.lld %t.o --start-lib %t1.o %tzz.o --end-lib -o /dev/null 2>&1 | FileCheck --check-prefix=ZZ %s
+# RUN: rm -f %tzz.a && llvm-ar rc %tzz.a %tzz.o
+# RUN: not ld.lld %t.o --start-lib %t1.o --end-lib %tzz.a -o /dev/null 2>&1 | FileCheck --check-prefix=ZZ %s
+
+# ZZ-NM: f2
+# ZZ-NM: zz
+
+# ZZ:      error: relocation refers to a symbol in a discarded section: zz
+# ZZ-NEXT: >>> defined in {{.*}}zz.o
+# ZZ-NEXT: >>> section group signature: foo
+# ZZ-NEXT: >>> prevailing definition is in {{.*}}1.o
+# ZZ-NEXT: >>> referenced by {{.*}}zz.o:(.text+0x1)
+
+## Don't error if the symbol which would cause "discarded section"
+## was inserted before %tzz.o
+# RUN: echo '.globl zz; zz:' | llvm-mc -filetype=obj -triple=x86_64 - -o %tdef.o
+# RUN: ld.lld %t.o --start-lib %t1.o %tdef.o %tzz.o --end-lib -o /dev/null
+# RUN: rm -f %tdef.a && llvm-ar rc %tdef.a %tdef.o
+# RUN: ld.lld %t.o --start-lib %t1.o %tdef.a %tzz.o --end-lib -o /dev/null
+
+.globl _start
+_start:
+  call f1
+  call f2

lld/test/ELF/i386-linkonce.s

 // REQUIRES: x86
 // RUN: llvm-mc -filetype=obj -triple=i386-linux-gnu %s -o %t.o
 // RUN: llvm-mc -filetype=obj -triple=i386-linux-gnu %p/Inputs/i386-linkonce.s -o %t2.o
 // RUN: llvm-ar rcs %t2.a %t2.o
-// RUN: ld.lld %t.o %t2.a -o %t
+// RUN: not ld.lld %t.o %t2.a -o /dev/null 2>&1 | FileCheck %s
+
+// CHECK: error: relocation refers to a symbol in a discarded section: __i686.get_pc_thunk.bx
 
     .globl _start
 _start:
     call _strchr1

lld/test/ELF/start-lib-comdat.s

 // REQUIRES: x86
 
 // RUN: llvm-mc -filetype=obj -triple=x86_64-pc-linux %s -o %t1.o
 // RUN: llvm-mc -filetype=obj -triple=x86_64-pc-linux \
 // RUN:   %p/Inputs/start-lib-comdat.s -o %t2.o
 // RUN: ld.lld -shared -o %t3 %t1.o --start-lib %t2.o --end-lib
 // RUN: llvm-readobj --symbols %t3 | FileCheck %s
 // RUN: ld.lld -shared -o %t3 --start-lib %t2.o --end-lib %t1.o
-// RUN: llvm-readobj --symbols %t3 | FileCheck %s
+// RUN: llvm-readobj --symbols %t3 | FileCheck /dev/null --implicit-check-not='Name: zed'
 
 // CHECK:      Name: zed
 // CHECK-NEXT: Value:
 // CHECK-NEXT: Size:
 // CHECK-NEXT: Binding: Global
 // CHECK-NEXT: Type:
 // CHECK-NEXT: Other:
 // CHECK-NEXT: Section: Undefined
 
         call bar@plt
 // The other file also has a section in the zed comdat, but it defines the
 // symbol zed. That means that we will have a lazy symbol zed, but when adding
 // the actual file zed will be undefined.
         .section        .sec,"aG",@progbits,zed,comdat