[lld][COFF] Add support for overriding weak symbols in LLVM bitcode input

[ayzhao]

Summary

LLVM bitcode contains support for weak symbols, so we can add support
for overriding weak symbols in the output COFF even though COFF doesn't
have inherent support for weak symbols.

The motivation for this patch is that Chromium is trying to use libc++'s
assertion handler mechanism, which relies on weak symbols [0], but we're
unable to perform a ThinLTO build on Windows due to this problem [1].

[0]: https://reviews.llvm.org/D121478
[1]: https://crrev.com/c/3863576

[mstorsjo]

Thanks for your effort on this! This looks mostly good to me. It's a shame that weak symbols from bitcode has to be handled differently than regular weak symbols from object files.

FWIW, I've got a set of testcases for weak symbol interactions in mingw, and a couple of them used to fail with LTO before; with this patch in place, all except one pass.

The one that still fails does this:

__attribute__((weak)) void func(void);
void doCall(void) {
    if (func)
        func();
}

If this is linked without any definition of func, it still fails with an undefined reference. (In the regular object file form, there's a weak undefined named func, which points at an absolute symbol (with a autogenerated unique name) with the value zero. If there's no other definition of func, the symbol resolves as the absolute address zero.)

lld/COFF/Symbols.h

148

Would it be good to mention that this only is tracked for bitcode/LTO symbols? For object files, weak symbols are handled in a different way.

161

Nit: This looks like an unintentded indentation change?

lld/test/COFF/weak-override.ll

8

I think it'd be useful to test that linking without the strong definition also succeeds.

[hans]

Very nice!

I'm still curious how our non-ThinLTO builds worked. I assume there is some nuance to "COFF doesn't support weak symbols"?

[mstorsjo]

In D133165#3770117, @hans wrote:

I'm still curious how our non-ThinLTO builds worked. I assume there is some nuance to "COFF doesn't support weak symbols"?

COFF does have weak aliases, and at least within the mingw/GNU ecosystem, there's ways that __attribute__((weak)) is mapped to weak aliases, which mostly works to emulate ELF style weak symbols. Not sure if this is usable when building in MSVC mode or not - that could be what you've been using.

[ayzhao]

code review comments

[ayzhao]

fix test

[rnk]

In D133165#3770165, @mstorsjo wrote:

In D133165#3770117, @hans wrote:

I'm still curious how our non-ThinLTO builds worked. I assume there is some nuance to "COFF doesn't support weak symbols"?

COFF does have weak aliases, and at least within the mingw/GNU ecosystem, there's ways that __attribute__((weak)) is mapped to weak aliases, which mostly works to emulate ELF style weak symbols. Not sure if this is usable when building in MSVC mode or not - that could be what you've been using.

Yes, and I think it would be quite awkward for us to try to model bitcode weak symbols with weak aliases. This new flag seems like a better way, thanks!

lgtm

[ayzhao]

fix spacing

[ayzhao]

rebase

lld/COFF/InputFiles.cpp

if (objSym.isUndefined()) {
         if (cast<DefinedRegular>(sym)->data == nullptr)
           cast<DefinedRegular>(sym)->data = &fakeSC->repl;
       } else if (comdat[comdatIndex].second) {
         sym = ctx.symtab.addRegular(this, symName, nullptr, fakeSC);
       } else {
         sym = ctx.symtab.addUndefined(symName, this, false);
       }
     } else {
-      sym = ctx.symtab.addRegular(this, symName, nullptr, fakeSC);
+      sym = ctx.symtab.addRegular(this, symName, nullptr, fakeSC, 0,
+                                  objSym.isWeak());
     }
     symbols.push_back(sym);
     if (objSym.isUsed())
       config->gcroot.push_back(sym);
   }
   directives = obj->getCOFFLinkerOpts();
 }
 

lld/COFF/SymbolTable.h

public:
 
   Symbol *addUndefined(StringRef name, InputFile *f, bool isWeakAlias);
   void addLazyArchive(ArchiveFile *f, const Archive::Symbol &sym);
   void addLazyObject(InputFile *f, StringRef n);
   void addLazyDLLSymbol(DLLFile *f, DLLFile::Symbol *sym, StringRef n);
   Symbol *addAbsolute(StringRef n, COFFSymbolRef s);
   Symbol *addRegular(InputFile *f, StringRef n,
                      const llvm::object::coff_symbol_generic *s = nullptr,
-                     SectionChunk *c = nullptr, uint32_t sectionOffset = 0);
+                     SectionChunk *c = nullptr, uint32_t sectionOffset = 0,
+                     bool isWeak = false);
   std::pair<DefinedRegular *, bool>
   addComdat(InputFile *f, StringRef n,
             const llvm::object::coff_symbol_generic *s = nullptr);
   Symbol *addCommon(InputFile *f, StringRef n, uint64_t size,
                     const llvm::object::coff_symbol_generic *s = nullptr,
                     CommonChunk *c = nullptr);
   Symbol *addImportData(StringRef n, ImportFile *f);
   Symbol *addImportThunk(StringRef name, DefinedImportData *s,

lld/COFF/SymbolTable.cpp

if (wasInserted || isa<Undefined>(s) || s->isLazy())
     replaceSymbol<DefinedSynthetic>(s, n, c);
   else if (!isa<DefinedCOFF>(s))
     reportDuplicate(s, nullptr);
   return s;
 }
 
 Symbol *SymbolTable::addRegular(InputFile *f, StringRef n,
                                 const coff_symbol_generic *sym, SectionChunk *c,
-                                uint32_t sectionOffset) {
+                                uint32_t sectionOffset, bool isWeak) {
   auto [s, wasInserted] = insert(n, f);
-  if (wasInserted || !isa<DefinedRegular>(s))
+  if (wasInserted || !isa<DefinedRegular>(s) || s->isWeak)
     replaceSymbol<DefinedRegular>(s, f, n, /*IsCOMDAT*/ false,
-                                  /*IsExternal*/ true, sym, c);
-  else
+                                  /*IsExternal*/ true, sym, c, isWeak);
+  else if (!isWeak)
     reportDuplicate(s, f, c, sectionOffset);
   return s;
 }
 
 std::pair<DefinedRegular *, bool>
 SymbolTable::addComdat(InputFile *f, StringRef n,
                        const coff_symbol_generic *sym) {
   auto [s, wasInserted] = insert(n, f);

lld/COFF/Symbols.h

private:
   void computeName();
 
 protected:
   friend SymbolTable;
   explicit Symbol(Kind k, StringRef n = "")
       : symbolKind(k), isExternal(true), isCOMDAT(false),
         writtenToSymtab(false), pendingArchiveLoad(false), isGCRoot(false),
         isRuntimePseudoReloc(false), deferUndefined(false), canInline(true),
-        nameSize(n.size()), nameData(n.empty() ? nullptr : n.data()) {}
+        isWeak(false), nameSize(n.size()),
+        nameData(n.empty() ? nullptr : n.data()) {}
 
   const unsigned symbolKind : 8;
   unsigned isExternal : 1;
 
 public:
   // This bit is used by the \c DefinedRegular subclass.
   unsigned isCOMDAT : 1;
 
public:
   // might be fixed up later.
   unsigned deferUndefined : 1;
 
   // False if LTO shouldn't inline whatever this symbol points to. If a symbol
   // is overwritten after LTO, LTO shouldn't inline the symbol because it
   // doesn't know the final contents of the symbol.
   unsigned canInline : 1;
 
+  // True if the symbol is weak. This is only tracked for bitcode/LTO symbols.
+  // This information isn't written to the output; rather, it's used for
+  // managing weak symbol overrides.
+  unsigned isWeak : 1;
 protected:
   // Symbol name length. Assume symbol lengths fit in a 32-bit integer.
   uint32_t nameSize;
 
   const char *nameData;
 };
 
 // The base class for any defined symbols, including absolute symbols,
 // etc.
 class Defined : public Symbol {
 public:
   Defined(Kind k, StringRef n) : Symbol(k, n) {}
 
   static bool classof(const Symbol *s) { return s->kind() <= LastDefinedKind; }
 
   // Returns the RVA (relative virtual address) of this symbol. The
   // writer sets and uses RVAs.
   uint64_t getRVA();
 
   // Returns the chunk containing this symbol. Absolute symbols and __ImageBase

 };
 
 // Regular defined symbols read from object file symbol tables.
 class DefinedRegular : public DefinedCOFF {
 public:
   DefinedRegular(InputFile *f, StringRef n, bool isCOMDAT,
                  bool isExternal = false,
                  const coff_symbol_generic *s = nullptr,
-                 SectionChunk *c = nullptr)
+                 SectionChunk *c = nullptr,
+                 bool isWeak = false)
       : DefinedCOFF(DefinedRegularKind, f, n, s), data(c ? &c->repl : nullptr) {
     this->isExternal = isExternal;
     this->isCOMDAT = isCOMDAT;
+    this->isWeak = isWeak;
   }
 
   static bool classof(const Symbol *s) {
     return s->kind() == DefinedRegularKind;
   }
 
   uint64_t getRVA() const { return (*data)->getRVA() + sym->Value; }
   SectionChunk *getChunk() const { return *data; }

lld/test/COFF/Inputs/strong-def.ll

+target datalayout = "e-m:w-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-pc-windows-msvc"
+
+define dso_local noundef i32 @foo() local_unnamed_addr {
+  ret i32 5678
+}

lld/test/COFF/Inputs/weak-def.ll

+target datalayout = "e-m:w-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-pc-windows-msvc"
+
+define weak dso_local noundef i32 @foo() local_unnamed_addr {
+  ret i32 1234
+}

lld/test/COFF/weak-override.ll

+; REQUIRES: x86
+; RUN: llvm-as -o %t-weak.obj %S/Inputs/weak-def.ll
+; RUN: llvm-as -o %t-strong.obj %S/Inputs/strong-def.ll
+; RUN: llvm-as -o %t.obj %s
+; RUN: lld-link /dll /out:%t-weak-first.dll %t.obj %t-weak.obj %t-strong.obj
+; RUN: lld-link /dll /out:%t-strong-first.dll %t.obj %t-strong.obj %t-weak.obj
+; RUN: lld-link /dll /out:%t-weak-only.dll %t.obj %t-weak.obj
+; RUN: llvm-objdump -d %t-weak-first.dll | FileCheck --check-prefix=CHECK-STRONG %s
+; RUN: llvm-objdump -d %t-strong-first.dll | FileCheck --check-prefix=CHECK-STRONG %s
+; RUN: llvm-objdump -d %t-weak-only.dll | FileCheck --check-prefix=CHECK-WEAK %s
+
+target datalayout = "e-m:w-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-pc-windows-msvc"
+
+declare noundef i32 @foo() local_unnamed_addr
+
+define dllexport i32 @bar() local_unnamed_addr {
+  %1 = tail call noundef i32 @foo()
+  ret i32 %1
+}
+
+define void @_DllMainCRTStartup() {
+entry:
+  ret void
+}
+
+; CHECK-STRONG: movl $5678, %eax
+; CHECK-STRONG-NOT: movl $1234, %eax
+; CHECK-WEAK: movl $1234, %eax
+; CHECK-WEAK-NOT: movl $5678, %eax