This is an archive of the discontinued LLVM Phabricator instance.

Differential D133165

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

Authored by ayzhao on Sep 1 2022, 4:06 PM.

Details

Reviewers
rnk
mstorsjo
Commits
rGb34006dd0a51: [lld][COFF] Add support for overriding weak symbols in LLVM bitcode input
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

Diff Detail

Event Timeline

ayzhao created this revision.Sep 1 2022, 4:06 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 1 2022, 4:06 PM
ayzhao requested review of this revision.Sep 1 2022, 4:06 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 1 2022, 4:06 PM
Herald added a subscriber: llvm-commits. · View Herald Transcript
ayzhao added a reviewer: rnk.Sep 1 2022, 4:07 PM
ayzhao added a subscriber: aeubanks.
Harbormaster completed remote builds in B184708: Diff 457437.Sep 1 2022, 4:31 PM
mstorsjo added a reviewer: mstorsjo.Sep 1 2022, 10:10 PM
ayzhao mentioned this in D121478: [libc++] Add a lightweight overridable assertion handler.Sep 2 2022, 4:06 PM
mstorsjo added a comment.Sep 5 2022, 2:55 AM

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
151

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.

165

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 added a subscriber: hans.Sep 5 2022, 5:53 AM

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 added a comment.Sep 5 2022, 6:17 AM
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 updated this revision to Diff 458189.Sep 6 2022, 9:26 AM
ayzhao marked 3 inline comments as done.

code review comments

Harbormaster completed remote builds in B185236: Diff 458189.Sep 6 2022, 9:46 AM
ayzhao updated this revision to Diff 458199.Sep 6 2022, 9:56 AM

fix test

Harbormaster completed remote builds in B185243: Diff 458199.Sep 6 2022, 10:36 AM
rnk accepted this revision.Sep 6 2022, 12:04 PM
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

This revision is now accepted and ready to land.Sep 6 2022, 12:04 PM
ayzhao updated this revision to Diff 458250.Sep 6 2022, 1:01 PM

fix spacing

Harbormaster completed remote builds in B185276: Diff 458250.Sep 6 2022, 1:24 PM
ayzhao updated this revision to Diff 458780.Sep 8 2022, 10:02 AM

rebase

This revision was landed with ongoing or failed builds.Sep 8 2022, 10:17 AM
Closed by commit rGb34006dd0a51: [lld][COFF] Add support for overriding weak symbols in LLVM bitcode input (authored by ayzhao). · Explain Why
This revision was automatically updated to reflect the committed changes.
ayzhao added a commit: rGb34006dd0a51: [lld][COFF] Add support for overriding weak symbols in LLVM bitcode input.
Harbormaster completed remote builds in B185648: Diff 458780.Sep 8 2022, 10:27 AM

Revision Contents

PathSize
lld/
COFF/
3 lines
3 lines
8 lines
11 lines
test/
COFF/
Inputs/
6 lines
6 lines
30 lines

Diff 458785

lld/COFF/InputFiles.cpp

Show First 20 Lines • Show All 1,064 Lines • ▼ Show 20 Lines if (objSym.isUndefined()) {
if (cast<DefinedRegular>(sym)->data == nullptr) if (cast<DefinedRegular>(sym)->data == nullptr)
cast<DefinedRegular>(sym)->data = &fakeSC->repl; cast<DefinedRegular>(sym)->data = &fakeSC->repl;
} else if (comdat[comdatIndex].second) { } else if (comdat[comdatIndex].second) {
sym = ctx.symtab.addRegular(this, symName, nullptr, fakeSC); sym = ctx.symtab.addRegular(this, symName, nullptr, fakeSC);
} else { } else {
sym = ctx.symtab.addUndefined(symName, this, false); sym = ctx.symtab.addUndefined(symName, this, false);
} }
} else { } else {
sym = ctx.symtab.addRegular(this, symName, nullptr, fakeSC); sym = ctx.symtab.addRegular(this, symName, nullptr, fakeSC, 0,
objSym.isWeak());
} }
symbols.push_back(sym); symbols.push_back(sym);
if (objSym.isUsed()) if (objSym.isUsed())
config->gcroot.push_back(sym); config->gcroot.push_back(sym);
} }
directives = obj->getCOFFLinkerOpts(); directives = obj->getCOFFLinkerOpts();
} }
▲ Show 20 Lines • Show All 119 Lines • Show Last 20 Lines

lld/COFF/SymbolTable.h

Show First 20 Lines • Show All 89 Lines • ▼ Show 20 Linespublic:
Symbol *addUndefined(StringRef name, InputFile *f, bool isWeakAlias); Symbol *addUndefined(StringRef name, InputFile *f, bool isWeakAlias);
void addLazyArchive(ArchiveFile *f, const Archive::Symbol &sym); void addLazyArchive(ArchiveFile *f, const Archive::Symbol &sym);
void addLazyObject(InputFile *f, StringRef n); void addLazyObject(InputFile *f, StringRef n);
void addLazyDLLSymbol(DLLFile *f, DLLFile::Symbol *sym, StringRef n); void addLazyDLLSymbol(DLLFile *f, DLLFile::Symbol *sym, StringRef n);
Symbol *addAbsolute(StringRef n, COFFSymbolRef s); Symbol *addAbsolute(StringRef n, COFFSymbolRef s);
Symbol *addRegular(InputFile *f, StringRef n, Symbol *addRegular(InputFile *f, StringRef n,
const llvm::object::coff_symbol_generic *s = nullptr, 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> std::pair<DefinedRegular *, bool>
addComdat(InputFile *f, StringRef n, addComdat(InputFile *f, StringRef n,
const llvm::object::coff_symbol_generic *s = nullptr); const llvm::object::coff_symbol_generic *s = nullptr);
Symbol *addCommon(InputFile *f, StringRef n, uint64_t size, Symbol *addCommon(InputFile *f, StringRef n, uint64_t size,
const llvm::object::coff_symbol_generic *s = nullptr, const llvm::object::coff_symbol_generic *s = nullptr,
CommonChunk *c = nullptr); CommonChunk *c = nullptr);
Symbol *addImportData(StringRef n, ImportFile *f); Symbol *addImportData(StringRef n, ImportFile *f);
Symbol *addImportThunk(StringRef name, DefinedImportData *s, Symbol *addImportThunk(StringRef name, DefinedImportData *s,
Show All 40 Lines

lld/COFF/SymbolTable.cpp

Show First 20 Lines • Show All 692 Lines • ▼ Show 20 Lines if (wasInserted || isa<Undefined>(s) || s->isLazy())
replaceSymbol<DefinedSynthetic>(s, n, c); replaceSymbol<DefinedSynthetic>(s, n, c);
else if (!isa<DefinedCOFF>(s)) else if (!isa<DefinedCOFF>(s))
reportDuplicate(s, nullptr); reportDuplicate(s, nullptr);
return s; return s;
} }
Symbol *SymbolTable::addRegular(InputFile *f, StringRef n, Symbol *SymbolTable::addRegular(InputFile *f, StringRef n,
const coff_symbol_generic *sym, SectionChunk *c, const coff_symbol_generic *sym, SectionChunk *c,
uint32_t sectionOffset) { uint32_t sectionOffset, bool isWeak) {
auto [s, wasInserted] = insert(n, f); 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, replaceSymbol<DefinedRegular>(s, f, n, /*IsCOMDAT*/ false,
/*IsExternal*/ true, sym, c); /*IsExternal*/ true, sym, c, isWeak);
else else if (!isWeak)
reportDuplicate(s, f, c, sectionOffset); reportDuplicate(s, f, c, sectionOffset);
return s; return s;
} }
std::pair<DefinedRegular *, bool> std::pair<DefinedRegular *, bool>
SymbolTable::addComdat(InputFile *f, StringRef n, SymbolTable::addComdat(InputFile *f, StringRef n,
const coff_symbol_generic *sym) { const coff_symbol_generic *sym) {
auto [s, wasInserted] = insert(n, f); auto [s, wasInserted] = insert(n, f);
▲ Show 20 Lines • Show All 153 Lines • Show Last 20 Lines

lld/COFF/Symbols.h

Show First 20 Lines • Show All 100 Lines • ▼ Show 20 Linesprivate:
void computeName(); void computeName();
protected: protected:
friend SymbolTable; friend SymbolTable;
explicit Symbol(Kind k, StringRef n = "") explicit Symbol(Kind k, StringRef n = "")
: symbolKind(k), isExternal(true), isCOMDAT(false), : symbolKind(k), isExternal(true), isCOMDAT(false),
writtenToSymtab(false), pendingArchiveLoad(false), isGCRoot(false), writtenToSymtab(false), pendingArchiveLoad(false), isGCRoot(false),
isRuntimePseudoReloc(false), deferUndefined(false), canInline(true), 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()) {
assert((!n.empty() || k <= LastDefinedCOFFKind) && assert((!n.empty() || k <= LastDefinedCOFFKind) &&
"If the name is empty, the Symbol must be a DefinedCOFF."); "If the name is empty, the Symbol must be a DefinedCOFF.");
} }
const unsigned symbolKind : 8; const unsigned symbolKind : 8;
unsigned isExternal : 1; unsigned isExternal : 1;
public: public:
Show All 22 Linespublic:
// might be fixed up later. // might be fixed up later.
unsigned deferUndefined : 1; unsigned deferUndefined : 1;
// False if LTO shouldn't inline whatever this symbol points to. If a symbol // 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 // is overwritten after LTO, LTO shouldn't inline the symbol because it
// doesn't know the final contents of the symbol. // doesn't know the final contents of the symbol.
unsigned canInline : 1; 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.
mstorsjoUnsubmitted
Done
ReplyInline Actions

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.

mstorsjo: Would it be good to mention that this only is tracked for bitcode/LTO symbols? For object files…
unsigned isWeak : 1;
protected: protected:
// Symbol name length. Assume symbol lengths fit in a 32-bit integer. // Symbol name length. Assume symbol lengths fit in a 32-bit integer.
uint32_t nameSize; uint32_t nameSize;
const char *nameData; const char *nameData;
}; };
// The base class for any defined symbols, including absolute symbols, // The base class for any defined symbols, including absolute symbols,
// etc. // etc.
class Defined : public Symbol { class Defined : public Symbol {
public: public:
Defined(Kind k, StringRef n) : Symbol(k, n) {} Defined(Kind k, StringRef n) : Symbol(k, n) {}
mstorsjoUnsubmitted
Done
ReplyInline Actions

Nit: This looks like an unintentded indentation change?

mstorsjo: Nit: This looks like an unintentded indentation change?
static bool classof(const Symbol *s) { return s->kind() <= LastDefinedKind; } static bool classof(const Symbol *s) { return s->kind() <= LastDefinedKind; }
// Returns the RVA (relative virtual address) of this symbol. The // Returns the RVA (relative virtual address) of this symbol. The
// writer sets and uses RVAs. // writer sets and uses RVAs.
uint64_t getRVA(); uint64_t getRVA();
// Returns the chunk containing this symbol. Absolute symbols and __ImageBase // Returns the chunk containing this symbol. Absolute symbols and __ImageBase
Show All 27 Lines
}; };
// Regular defined symbols read from object file symbol tables. // Regular defined symbols read from object file symbol tables.
class DefinedRegular : public DefinedCOFF { class DefinedRegular : public DefinedCOFF {
public: public:
DefinedRegular(InputFile *f, StringRef n, bool isCOMDAT, DefinedRegular(InputFile *f, StringRef n, bool isCOMDAT,
bool isExternal = false, bool isExternal = false,
const coff_symbol_generic *s = nullptr, 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) { : DefinedCOFF(DefinedRegularKind, f, n, s), data(c ? &c->repl : nullptr) {
this->isExternal = isExternal; this->isExternal = isExternal;
this->isCOMDAT = isCOMDAT; this->isCOMDAT = isCOMDAT;
this->isWeak = isWeak;
} }
static bool classof(const Symbol *s) { static bool classof(const Symbol *s) {
return s->kind() == DefinedRegularKind; return s->kind() == DefinedRegularKind;
} }
uint64_t getRVA() const { return (*data)->getRVA() + sym->Value; } uint64_t getRVA() const { return (*data)->getRVA() + sym->Value; }
SectionChunk *getChunk() const { return *data; } SectionChunk *getChunk() const { return *data; }
▲ Show 20 Lines • Show All 295 Lines • Show Last 20 Lines

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

  • This file was added.
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

  • This file was added.
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

  • This file was added.
; 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
mstorsjoUnsubmitted
Done
ReplyInline Actions

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

mstorsjo: I think it'd be useful to test that linking without the strong definition also succeeds.
; 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