Skip to content

[lld][WebAssembly] Fix spurious signature mismatch under LTO #136197

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Apr 18, 2025
+47 −1
Merged

[lld][WebAssembly] Fix spurious signature mismatch under LTO #136197

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
45 changes: 45 additions & 0 deletions lld/test/wasm/lto/thinlto-signature-mismatch-unknown.ll
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
; RUN: rm -rf %t && split-file %s %t && cd %t
; RUN: opt -thinlto-bc a.ll -o a.o
; RUN: opt -thinlto-bc b.ll -o b.o
; RUN: llvm-ar rcs b.a b.o
; RUN: opt -thinlto-bc c.ll -o c.o

;; Taking the address of the incorrectly declared @foo should not generate a warning.
; RUN: wasm-ld --fatal-warnings --no-entry --export-all a.o b.a -o a.out \
; RUN: | FileCheck %s --implicit-check-not 'warning' --allow-empty

;; But we should still warn if we call the function with the wrong signature.
; RUN: not wasm-ld --fatal-warnings --no-entry --export-all a.o b.a c.o -o b.out 2>&1 \
; RUN: | FileCheck %s --check-prefix=INVALID

; INVALID: error: function signature mismatch: foo
; INVALID: >>> defined as () -> void
; INVALID: >>> defined as () -> i32

;--- a.ll
target datalayout = "e-m:e-p:32:32-p10:8:8-p20:8:8-i64:64-n32:64-S128-ni:1:10:20"
target triple = "wasm32-unknown-unknown"

@ptr = constant ptr @foo
declare void @foo()

;--- b.ll
target datalayout = "e-m:e-p:32:32-p10:8:8-p20:8:8-i64:64-n32:64-S128-ni:1:10:20"
target triple = "wasm32-unknown-unknown"

define i32 @foo() noinline {
entry:
ret i32 42
}

;--- c.ll
target datalayout = "e-m:e-p:32:32-p10:8:8-p20:8:8-i64:64-n32:64-S128-ni:1:10:20"
target triple = "wasm32-unknown-unknown"

declare void @foo()

define void @invalid() {
entry:
call void @foo()
ret void
}
3 changes: 2 additions & 1 deletion lld/wasm/LTO.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,9 +108,10 @@ BitcodeCompiler::~BitcodeCompiler() = default;

static void undefine(Symbol *s) {
if (auto f = dyn_cast<DefinedFunction>(s))
// If the signature is null, there were no calls from non-bitcode objects.
replaceSymbol<UndefinedFunction>(f, f->getName(), std::nullopt,
std::nullopt, 0, f->getFile(),
f->signature);
f->signature, f->signature != nullptr);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if we should instead make SymbolTable.cpp assume that the null signature is a placeholder? i.e. today we do checkSig = ud->isCalledDirectly; but we could do checkSig = ud->isCalledDirectly && ud->signature;?

I don't feel strongly though and this approach certainly works.

Copy link
Member Author

@BertalanD BertalanD Apr 18, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think that would fix the issue: signatureMatches() already returns true when either function has a null signature.

In the test case, after LTO, the () -> void reference from a.ll is added first. The existing undefined symbol has a null signature, but this one doesn't, so we set existingFunction->signature to () -> void:

llvm-project/lld/wasm/SymbolTable.cpp

Lines 657 to 658 in 1db03ca

if (!existingFunction->signature && sig)
existingFunction->signature = sig;

As isCalledDirectly is set to true, we'll report the signature mismatch when we try to add the () -> i32 definition from b.ll.

I suppose an alternative would be to only set existingFunction->signature if the undefined symbol we replace it with is called directly, but that would mean we never set the signature if the function has no call sites, only its address is taken. That might cause problems later on.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good. LGTM as is.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks!

else if (isa<DefinedData>(s))
replaceSymbol<UndefinedData>(s, s->getName(), 0, s->getFile());
else
Expand Down
Loading