[lld][WebAssembly] Fix regression in function signature checking #78831
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@llvm/pr-subscribers-lld Author: Sam Clegg (sbc100) ChangesFollowup to #78658, which caused a regression in emscripten. When a lazy symbol is added, which resolved and existing undefined symbol, we don't need/want to replace the undefined symbol with the lazy one. Instead we called extract, which replaces the undefined symbol with the defined one. The fact that we were first replacing the undefined symbol with a lazy one before extracting the archive member doesn't normally matter but, in the case of the function symbol, replacing the undefined symbol with a lazy symbol means that Note that this is consistent with both the ELF code in Full diff: https://github.com/llvm/llvm-project/pull/78831.diff 3 Files Affected:
diff --git a/lld/test/wasm/signature-mismatch.s b/lld/test/wasm/signature-mismatch.s
index 5d305efca24649..7dc1b8ced3530d 100644
--- a/lld/test/wasm/signature-mismatch.s
+++ b/lld/test/wasm/signature-mismatch.s
@@ -8,6 +8,11 @@
# RUN: wasm-ld -r -o %t.reloc.o %t.main.o %t.ret32.o %t.call.o 2>&1 | FileCheck %s -check-prefix=WARN
# RUN: obj2yaml %t.reloc.o | FileCheck %s -check-prefix=RELOC
+# RUN: rm -f %t.a
+# RUN: ar crS %t.a %t.ret32.o %t.call.o
+# RUN: wasm-ld --export=call_ret32 --export=ret32 -o %t2.wasm %t.main.o %t.a 2>&1 | FileCheck %s -check-prefix=ARCHIVE
+# RUN: obj2yaml %t2.wasm | FileCheck %s -check-prefix=YAML
+
# RUN: not wasm-ld --fatal-warnings -o %t.wasm %t.main.o %t.ret32.o %t.call.o 2>&1 | FileCheck %s -check-prefix=ERROR
.functype ret32 (i32, i64, i32) -> (i32)
@@ -42,6 +47,10 @@ ret32_address_main:
# WARN-NEXT: >>> defined as (i32, i64, i32) -> i32 in {{.*}}.main.o
# WARN-NEXT: >>> defined as (f32) -> i32 in {{.*}}.ret32.o
+# ARCHIVE: warning: function signature mismatch: ret32
+# ARCHIVE-NEXT: >>> defined as (i32, i64, i32) -> i32 in {{.*}}.main.o
+# ARCHIVE-NEXT: >>> defined as (f32) -> i32 in {{.*}}.ret32.o
+
# ERROR: error: function signature mismatch: ret32
# ERROR-NEXT: >>> defined as (i32, i64, i32) -> i32 in {{.*}}.main.o
# ERROR-NEXT: >>> defined as (f32) -> i32 in {{.*}}.ret32.o
@@ -56,7 +65,7 @@ ret32_address_main:
# YAML: - Type: CUSTOM
# YAML-NEXT: Name: name
-# YAML-NEXT: FunctionNames:
+# YAML-NEXT: FunctionNames:
# YAML-NEXT: - Index: 0
# YAML-NEXT: Name: 'signature_mismatch:ret32'
# YAML-NEXT: - Index: 1
diff --git a/lld/wasm/SymbolTable.cpp b/lld/wasm/SymbolTable.cpp
index c98aa3ee3a7a32..c000d5d3b97dc0 100644
--- a/lld/wasm/SymbolTable.cpp
+++ b/lld/wasm/SymbolTable.cpp
@@ -749,7 +749,7 @@ void SymbolTable::addLazy(StringRef name, InputFile *file) {
std::tie(s, wasInserted) = insertName(name);
if (wasInserted) {
- replaceSymbol<LazySymbol>(s, name, 0, file);
+ replaceSymbol<LazySymbol>(s, name, file);
return;
}
@@ -767,14 +767,14 @@ void SymbolTable::addLazy(StringRef name, InputFile *file) {
oldSig = f->signature;
LLVM_DEBUG(dbgs() << "replacing existing weak undefined symbol\n");
auto newSym =
- replaceSymbol<LazySymbol>(s, name, WASM_SYMBOL_BINDING_WEAK, file);
+ replaceSymbol<LazySymbol>(s, name, file, WASM_SYMBOL_BINDING_WEAK);
newSym->signature = oldSig;
return;
}
LLVM_DEBUG(dbgs() << "replacing existing undefined\n");
const InputFile *oldFile = s->getFile();
- replaceSymbol<LazySymbol>(s, name, 0, file)->extract();
+ LazySymbol(name, file).extract();
if (!config->whyExtract.empty())
ctx.whyExtractRecords.emplace_back(toString(oldFile), s->getFile(), *s);
}
diff --git a/lld/wasm/Symbols.h b/lld/wasm/Symbols.h
index de52c92d34e78b..9908f23f38695b 100644
--- a/lld/wasm/Symbols.h
+++ b/lld/wasm/Symbols.h
@@ -497,7 +497,7 @@ class UndefinedTag : public TagSymbol {
// symbols into consideration.
class LazySymbol : public Symbol {
public:
- LazySymbol(StringRef name, uint32_t flags, InputFile *file)
+ LazySymbol(StringRef name, InputFile *file, uint8_t flags = 0)
: Symbol(name, LazyKind, flags, file) {}
static bool classof(const Symbol *s) { return s->kind() == LazyKind; }
|
|
@llvm/pr-subscribers-lld-wasm Author: Sam Clegg (sbc100) ChangesFollowup to #78658, which caused a regression in emscripten. When a lazy symbol is added, which resolved and existing undefined symbol, we don't need/want to replace the undefined symbol with the lazy one. Instead we called extract, which replaces the undefined symbol with the defined one. The fact that we were first replacing the undefined symbol with a lazy one before extracting the archive member doesn't normally matter but, in the case of the function symbol, replacing the undefined symbol with a lazy symbol means that Note that this is consistent with both the ELF code in Full diff: https://github.com/llvm/llvm-project/pull/78831.diff 3 Files Affected:
diff --git a/lld/test/wasm/signature-mismatch.s b/lld/test/wasm/signature-mismatch.s
index 5d305efca246496..7dc1b8ced3530d5 100644
--- a/lld/test/wasm/signature-mismatch.s
+++ b/lld/test/wasm/signature-mismatch.s
@@ -8,6 +8,11 @@
# RUN: wasm-ld -r -o %t.reloc.o %t.main.o %t.ret32.o %t.call.o 2>&1 | FileCheck %s -check-prefix=WARN
# RUN: obj2yaml %t.reloc.o | FileCheck %s -check-prefix=RELOC
+# RUN: rm -f %t.a
+# RUN: ar crS %t.a %t.ret32.o %t.call.o
+# RUN: wasm-ld --export=call_ret32 --export=ret32 -o %t2.wasm %t.main.o %t.a 2>&1 | FileCheck %s -check-prefix=ARCHIVE
+# RUN: obj2yaml %t2.wasm | FileCheck %s -check-prefix=YAML
+
# RUN: not wasm-ld --fatal-warnings -o %t.wasm %t.main.o %t.ret32.o %t.call.o 2>&1 | FileCheck %s -check-prefix=ERROR
.functype ret32 (i32, i64, i32) -> (i32)
@@ -42,6 +47,10 @@ ret32_address_main:
# WARN-NEXT: >>> defined as (i32, i64, i32) -> i32 in {{.*}}.main.o
# WARN-NEXT: >>> defined as (f32) -> i32 in {{.*}}.ret32.o
+# ARCHIVE: warning: function signature mismatch: ret32
+# ARCHIVE-NEXT: >>> defined as (i32, i64, i32) -> i32 in {{.*}}.main.o
+# ARCHIVE-NEXT: >>> defined as (f32) -> i32 in {{.*}}.ret32.o
+
# ERROR: error: function signature mismatch: ret32
# ERROR-NEXT: >>> defined as (i32, i64, i32) -> i32 in {{.*}}.main.o
# ERROR-NEXT: >>> defined as (f32) -> i32 in {{.*}}.ret32.o
@@ -56,7 +65,7 @@ ret32_address_main:
# YAML: - Type: CUSTOM
# YAML-NEXT: Name: name
-# YAML-NEXT: FunctionNames:
+# YAML-NEXT: FunctionNames:
# YAML-NEXT: - Index: 0
# YAML-NEXT: Name: 'signature_mismatch:ret32'
# YAML-NEXT: - Index: 1
diff --git a/lld/wasm/SymbolTable.cpp b/lld/wasm/SymbolTable.cpp
index c98aa3ee3a7a320..c000d5d3b97dc0d 100644
--- a/lld/wasm/SymbolTable.cpp
+++ b/lld/wasm/SymbolTable.cpp
@@ -749,7 +749,7 @@ void SymbolTable::addLazy(StringRef name, InputFile *file) {
std::tie(s, wasInserted) = insertName(name);
if (wasInserted) {
- replaceSymbol<LazySymbol>(s, name, 0, file);
+ replaceSymbol<LazySymbol>(s, name, file);
return;
}
@@ -767,14 +767,14 @@ void SymbolTable::addLazy(StringRef name, InputFile *file) {
oldSig = f->signature;
LLVM_DEBUG(dbgs() << "replacing existing weak undefined symbol\n");
auto newSym =
- replaceSymbol<LazySymbol>(s, name, WASM_SYMBOL_BINDING_WEAK, file);
+ replaceSymbol<LazySymbol>(s, name, file, WASM_SYMBOL_BINDING_WEAK);
newSym->signature = oldSig;
return;
}
LLVM_DEBUG(dbgs() << "replacing existing undefined\n");
const InputFile *oldFile = s->getFile();
- replaceSymbol<LazySymbol>(s, name, 0, file)->extract();
+ LazySymbol(name, file).extract();
if (!config->whyExtract.empty())
ctx.whyExtractRecords.emplace_back(toString(oldFile), s->getFile(), *s);
}
diff --git a/lld/wasm/Symbols.h b/lld/wasm/Symbols.h
index de52c92d34e78b0..9908f23f38695b8 100644
--- a/lld/wasm/Symbols.h
+++ b/lld/wasm/Symbols.h
@@ -497,7 +497,7 @@ class UndefinedTag : public TagSymbol {
// symbols into consideration.
class LazySymbol : public Symbol {
public:
- LazySymbol(StringRef name, uint32_t flags, InputFile *file)
+ LazySymbol(StringRef name, InputFile *file, uint8_t flags = 0)
: Symbol(name, LazyKind, flags, file) {}
static bool classof(const Symbol *s) { return s->kind() == LazyKind; }
|
Followup to llvm#78658, which caused a regression in emscripten. When a lazy symbol is added, which resolved and existing undefined symbol, we don't need/want to replace the undefined symbol with the lazy one. Instead we called extract, which replaces the undefined symbol with the defined one. The fact that we were first replacing the undefined symbol with a lazy one before extracting the archive member doesn't normally matter but, in the case of the function symbol, replacing the undefined symbol with a lazy symbol means that `addDefinedFunction` sees the existing symbol as lazy and simply replaces it. Note that this is consistent with both the ELF code in `Symbol::resolve(const LazySymbol &other)` and the wasm code prior to llvm#78658, neither of which replace the existing symbol with the lazy one in this case.
dschuff
reviewed
Jan 22, 2024
| @@ -8,6 +8,11 @@ | |||
| # RUN: wasm-ld -r -o %t.reloc.o %t.main.o %t.ret32.o %t.call.o 2>&1 | FileCheck %s -check-prefix=WARN | |||
| # RUN: obj2yaml %t.reloc.o | FileCheck %s -check-prefix=RELOC | |||
|
|
|||
| # RUN: rm -f %t.a | |||
| # RUN: ar crS %t.a %t.ret32.o %t.call.o | |||
There was a problem hiding this comment.
should this be llvm-ar? I wasn't sure this would work on e.g. Windows where there's no "system" ar, and the (few) other uses of 'ar' in tests use llvm-ar.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Followup to #78658, which caused a regression in emscripten.
When a lazy symbol is added, which resolved and existing undefined symbol, we don't need/want to replace the undefined symbol with the lazy one. Instead we called extract, which replaces the undefined symbol with the defined one.
The fact that we were first replacing the undefined symbol with a lazy one before extracting the archive member doesn't normally matter but, in the case of the function symbol, replacing the undefined symbol with a lazy symbol means that
addDefinedFunctionsees the existing symbol as lazy and simply replaces it.Note that this is consistent with both the ELF code in
Symbol::resolve(const LazySymbol &other)and the wasm code prior to#78658, neither of which replace the existing symbol with the lazy one
in this case.