[lld] [lld][WebAssembly] Fix regression in function signature checking (PR #78831)

[Sam Clegg via llvm-commits]

https://github.com/sbc100 created https://github.com/llvm/llvm-project/pull/78831

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 `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
 #78658, neither of which replace the existing symbol with the lazy one
in this case.

>From ae1a9d765dc15d1793cade0fda1e271a7de98858 Mon Sep 17 00:00:00 2001
From: Sam Clegg <sbc at chromium.org>
Date: Sat, 20 Jan 2024 05:05:22 +0000
Subject: [PATCH] [lld][WebAssembly] Fix regression in function signature
 checking

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 `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
 #78658, neither of which replace the existing symbol with the lazy one
in this case.
---
 lld/test/wasm/signature-mismatch.s | 11 ++++++++++-
 lld/wasm/SymbolTable.cpp           |  6 +++---
 lld/wasm/Symbols.h                 |  2 +-
 3 files changed, 14 insertions(+), 5 deletions(-)

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; }