[WebAssembly] Error on mismatched function signature in final output

[sbc100]

Summary

During symbol resolution, emit warnings for function signature
mismatches. During GC, if any mismatched symbol is marked as live
then generate an error.

This means that we only error out if the mismatch is written to the
final output. i.e. if we would generate an invalid wasm file.

[sbc100]

• rebase

[sbc100]

• rebase

[sbc100]

• cleanup

[jgravelle-google]

wasm/SymbolTable.cpp

134

It feels odd to me that we warn about a mismatch, and then later on error with the same message. Would feel more natural to only print the error, though it's probably useful enough to have the ">>> defind as" context that it's fine as-is.

[sbc100]

Exactly, we don't have the detailed information later on, unless we were to cache it somewhere I guess. Its certainly essential to know where to mismatches are coming from.

[ruiu]

I'm not sure if this is a good thing to do, because my understanding is that -gc-sections shouldn't generally change the linking semantics except it will produce a smaller binary. With this change, you are now easy to write code that links only when you pass -gc-sections. This is perhaps not problematic for wasm because -gc-section is on by default, but still, I feel like -gc-sections shouldn't do anything more than reducing the output size. What do you think?

lld/trunk/wasm/SymbolTable.cpp

125 ↗

(On Diff #152203)

msg -> Msg

[sbc100]

In D48394#1144482, @ruiu wrote:

I'm not sure if this is a good thing to do, because my understanding is that -gc-sections shouldn't generally change the linking semantics except it will produce a smaller binary. With this change, you are now easy to write code that links only when you pass -gc-sections. This is perhaps not problematic for wasm because -gc-section is on by default, but still, I feel like -gc-sections shouldn't do anything more than reducing the output size. What do you think?

Hmm.. I tend to agree. I'm working on some changes that should hopefully make these error less common, and then we should be able to turn it for all functions (even those that get GC'd)

[sbc100]

This change was reverted in rL335355

test/wasm/Inputs/ret32.ll

 target triple = "wasm32-unknown-unknown"
 
 ; Function Attrs: norecurse nounwind readnone
-define i32 @ret32(float %arg) #0 {
+define hidden i32 @ret32(float %arg) #0 {
 entry:
     ret i32 0
      ; ptrtoint (i32 (float)* @ret32 to i32)
 }

test/wasm/Inputs/ret64.ll

 target triple = "wasm32-unknown-unknown"
 
-define i64 @ret64(double %arg) local_unnamed_addr #0 {
+define hidden i64 @ret64(double %arg) local_unnamed_addr #0 {
 entry:
     ret i64 1
 }

test/wasm/fatal-warnings.ll

 ; RUN: llc -filetype=obj %s -o %t.main.o
 ; RUN: llc -filetype=obj %p/Inputs/ret32.ll -o %t.ret32.o
 ; RUN: lld -flavor wasm -o %t.wasm %t.main.o %t.ret32.o 2>&1 | FileCheck %s -check-prefix=CHECK-WARN
 ; RUN: not lld -flavor wasm --fatal-warnings -o %t.wasm %t.main.o %t.ret32.o 2>&1 | FileCheck %s -check-prefix=CHECK-FATAL
 
 ; CHECK-WARN: warning: function signature mismatch: ret32
 ; CHECK-FATAL: error: function signature mismatch: ret32
 
 target triple = "wasm32-unknown-unknown"
 
 define hidden void @_start() local_unnamed_addr #0 {
 entry:
+  ret void
+}
+
+define hidden void @_call_ret32() local_unnamed_addr #0 {
+entry:
   %call = tail call i32 @ret32(i32 1, i64 2, i32 3) #2
   ret void
 }
 
 declare i32 @ret32(i32, i64, i32) local_unnamed_addr #1

test/wasm/load-undefined.test

 ; Verify that the -u / --undefined option is able to pull in symbols from
 ; an archive, and doesn't error when uses to pull in a symbol already loaded.
 ;
 ; RUN: llc -filetype=obj %S/Inputs/ret64.ll -o %t.o
 ; RUN: llc -filetype=obj %S/Inputs/ret32.ll -o %t2.o
 ; RUN: llc -filetype=obj %S/Inputs/start.ll -o %t.start.o
 ; RUN: llvm-ar rcs %t2.a %t2.o
-; RUN: wasm-ld %t.start.o %t2.a %t.o -o %t.wasm -u ret32 --undefined ret64
+; RUN: wasm-ld %t.start.o --no-gc-sections %t2.a %t.o -o %t.wasm -u ret32 --undefined ret64
 ; RUN: obj2yaml %t.wasm | FileCheck %s
+; RUN: wasm-ld %t.start.o --no-gc-sections %t2.a %t.o -o %t2.wasm
+; RUN: obj2yaml %t2.wasm | FileCheck %s -check-prefix=NO-LOAD
 
-; CHECK:        - Type:            EXPORT
-; CHECK-NEXT:     Exports:
-; CHECK-NEXT:       - Name:            memory
-; CHECK-NEXT:         Kind:            MEMORY
-; CHECK-NEXT:         Index:           0
-; CHECK-NEXT:       - Name:            __heap_base
-; CHECK-NEXT:         Kind:            GLOBAL
-; CHECK-NEXT:         Index:           1
-; CHECK-NEXT:       - Name:            __data_end
-; CHECK-NEXT:         Kind:            GLOBAL
-; CHECK-NEXT:         Index:           2
-; CHECK-NEXT:       - Name:            _start
-; CHECK-NEXT:         Kind:            FUNCTION
-; CHECK-NEXT:         Index:           1
-; CHECK-NEXT:       - Name:            ret32
-; CHECK-NEXT:         Kind:            FUNCTION
-; CHECK-NEXT:         Index:           2
-; CHECK-NEXT:       - Name:            ret64
-; CHECK-NEXT:         Kind:            FUNCTION
-; CHECK-NEXT:         Index:           3
-; CHECK-NEXT:   - Type:
+; CHECK:         Name:            name
+; CHECK-NEXT:    FunctionNames:   
+; CHECK-NEXT:      - Index:           0
+; CHECK-NEXT:        Name:            __wasm_call_ctors
+; CHECK-NEXT:      - Index:           1
+; CHECK-NEXT:        Name:            _start
+; CHECK-NEXT:      - Index:           2
+; CHECK-NEXT:        Name:            ret32
+; CHECK-NEXT:      - Index:           3
+; CHECK-NEXT:        Name:            ret64
+; CHECK-NEXT: ...
 
+; NO-LOAD:         Name:            name
+; NO-LOAD-NEXT:    FunctionNames:   
+; NO-LOAD-NEXT:      - Index:           0
+; NO-LOAD-NEXT:        Name:            __wasm_call_ctors
+; NO-LOAD-NEXT:      - Index:           1
+; NO-LOAD-NEXT:        Name:            _start
+; NO-LOAD-NEXT:      - Index:           2
+; NO-LOAD-NEXT:        Name:            ret64
+; NO-LOAD-NEXT: ...
 
 ; Verify that referencing a symbol that doesn't exist won't work
 ; RUN: not wasm-ld %t.start.o -o %t.wasm -u symboldoesnotexist 2>&1 | FileCheck -check-prefix=CHECK-UNDEFINED1 %s
 ; CHECK-UNDEFINED1: error: undefined symbol: symboldoesnotexist
 
 ; RUN: not wasm-ld %t.start.o -o %t.wasm --undefined symboldoesnotexist --allow-undefined 2>&1 | FileCheck -check-prefix=CHECK-UNDEFINED2 %s
 ; CHECK-UNDEFINED2: symbol forced with --undefined not found: symboldoesnotexist

test/wasm/signature-mismatch-weak.ll

 ; RUN: llc -filetype=obj %p/Inputs/weak-symbol1.ll -o %t.weak.o
 ; RUN: llc -filetype=obj %p/Inputs/strong-symbol.ll -o %t.strong.o
 ; RUN: llc -filetype=obj %s -o %t.o
-; RUN: wasm-ld -o %t.wasm %t.o %t.strong.o %t.weak.o 2>&1 | FileCheck %s
+; RUN: not wasm-ld -o %t.wasm %t.o %t.strong.o %t.weak.o 2>&1 | FileCheck %s
 
 target triple = "wasm32-unknown-unknown"
 
 declare i32 @weakFn() local_unnamed_addr
 
 define void @_start() local_unnamed_addr {
 entry:
   %call = call i32 @weakFn()
   ret void
 }
 
 ; CHECK: warning: function signature mismatch: weakFn
 ; CHECK-NEXT: >>> defined as () -> I32 in {{.*}}signature-mismatch-weak.ll.tmp.o
 ; CHECK-NEXT: >>> defined as () -> I64 in {{.*}}signature-mismatch-weak.ll.tmp.strong.o
+; CHECK: error: function signature mismatch: weakFn

test/wasm/signature-mismatch.ll

 ; RUN: llc -filetype=obj %p/Inputs/ret32.ll -o %t.ret32.o
 ; RUN: llc -filetype=obj %s -o %t.main.o
-; RUN: not wasm-ld --fatal-warnings -o %t.wasm %t.main.o %t.ret32.o 2>&1 | FileCheck %s
+; RUN: not wasm-ld -o %t.wasm %t.main.o %t.ret32.o 2>&1 | FileCheck %s
+; RUN: not wasm-ld --no-gc-sections -o %t.wasm %t.main.o %t.ret32.o 2>&1 | FileCheck %s -check-prefix=NO-GC
 ; Run the test again by with the object files in the other order to verify
 ; the check works when the undefined symbol is resolved by an existing defined
 ; one.
-; RUN: not wasm-ld --fatal-warnings -o %t.wasm %t.ret32.o %t.main.o 2>&1 | FileCheck %s -check-prefix=REVERSE
+; RUN: not wasm-ld -o %t.wasm %t.ret32.o %t.main.o 2>&1 | FileCheck %s -check-prefix=REVERSE
 
 target triple = "wasm32-unknown-unknown"
 
 ; Function Attrs: nounwind
 define hidden void @_start() local_unnamed_addr #0 {
 entry:
   %call = tail call i32 @ret32(i32 1, i64 2, i32 3) #2
   ret void
 }
 
 declare i32 @ret32(i32, i64, i32) local_unnamed_addr #1
 
-; CHECK: error: function signature mismatch: ret32
+; CHECK: warning: function signature mismatch: ret32
 ; CHECK-NEXT: >>> defined as (I32, I64, I32) -> I32 in {{.*}}.main.o
 ; CHECK-NEXT: >>> defined as (F32) -> I32 in {{.*}}.ret32.o
+; CHECK: error: function signature mismatch: ret32
 
-; REVERSE: error: function signature mismatch: ret32
+; NO-GC: error: function signature mismatch: ret32
+; NO-GC-NEXT: >>> defined as (I32, I64, I32) -> I32 in {{.*}}.main.o
+; NO-GC-NEXT: >>> defined as (F32) -> I32 in {{.*}}.ret32.o
+
+; REVERSE: warning: function signature mismatch: ret32
 ; REVERSE-NEXT: >>> defined as (F32) -> I32 in {{.*}}.ret32.o
 ; REVERSE-NEXT: >>> defined as (I32, I64, I32) -> I32 in {{.*}}.main.o
+; REVERSE: error: function signature mismatch: ret32

wasm/Driver.cpp

void LinkerDriver::link(ArrayRef<const char *> ArgsArr) {
 
   if (EntrySym)
     EntrySym->setHidden(false);
 
   if (errorCount())
     return;
 
   // Do size optimizations: garbage collection
+  if (Config->GcSections)
     markLive();
 
   // Write the result to the file.
   writeResult();
 }

wasm/MarkLive.cpp

 #define DEBUG_TYPE "lld"
 
 using namespace llvm;
 using namespace llvm::wasm;
 using namespace lld;
 using namespace lld::wasm;
 
 void lld::wasm::markLive() {
-  if (!Config->GcSections)
-    return;
-
   LLVM_DEBUG(dbgs() << "markLive\n");
   SmallVector<InputChunk *, 256> Q;
 
   auto Enqueue = [&](Symbol *Sym) {
     if (!Sym || Sym->isLive())
       return;
     LLVM_DEBUG(dbgs() << "markLive: " << Sym->getName() << "\n");
     Sym->markLive();
+    if (Sym->SignatureMismatch)
+      error("function signature mismatch: " + Sym->getName());
     if (InputChunk *Chunk = Sym->getChunk())
       Q.push_back(Chunk);
   };
 
   // Add GC root symbols.
   if (!Config->Entry.empty())
     Enqueue(Symtab->find(Config->Entry));
   Enqueue(WasmSym::CallCtors);

wasm/SymbolTable.cpp

 static void reportTypeError(const Symbol *Existing, const InputFile *File,
                             llvm::wasm::WasmSymbolType Type) {
   error("symbol type mismatch: " + toString(*Existing) + "\n>>> defined as " +
         toString(Existing->getWasmType()) + " in " +
         toString(Existing->getFile()) + "\n>>> defined as " + toString(Type) +
         " in " + toString(File));
 }
 
-static void checkFunctionType(const Symbol *Existing, const InputFile *File,
+static void checkFunctionType(Symbol *Existing, const InputFile *File,
                               const WasmSignature *NewSig) {
   auto ExistingFunction = dyn_cast<FunctionSymbol>(Existing);
   if (!ExistingFunction) {
     reportTypeError(Existing, File, WASM_SYMBOL_TYPE_FUNCTION);
     return;
   }
 
+  // Don't generate more than one warning per symbol.
+  if (Existing->SignatureMismatch)
+    return;
+
   const WasmSignature *OldSig = ExistingFunction->getFunctionType();
   if (OldSig && NewSig && *NewSig != *OldSig) {
-    warn("function signature mismatch: " + Existing->getName() +
+    std::string msg = ("function signature mismatch: " + Existing->getName() +
                        "\n>>> defined as " + toString(*OldSig) + " in " +
                        toString(Existing->getFile()) + "\n>>> defined as " +
-         toString(*NewSig) + " in " + toString(File));
+                       toString(*NewSig) + " in " + toString(File))
+                          .str();
+    // A function signature mismatch is only really problem if the mismatched
+    // symbol is included in the final output, and gc-sections can remove the
+    // offending uses.  Therefore we delay reporting this as an error when
+    // section GC is enabled.
+    if (Config->GcSections) {
+      Existing->SignatureMismatch = true;
+      warn(msg);
+    } else {
+      error(msg);
+    }
   }
 }
 
 // Check the type of new symbol matches that of the symbol is replacing.
 // For functions this can also involve verifying that the signatures match.
 static void checkGlobalType(const Symbol *Existing, const InputFile *File,
                             const WasmGlobalType *NewType) {
   if (!isa<GlobalSymbol>(Existing)) {

wasm/Symbols.h

public:
   // relocatable output.
   uint32_t getOutputSymbolIndex() const;
   void setOutputSymbolIndex(uint32_t Index);
 
   WasmSymbolType getWasmType() const;
 
   // True if this symbol was referenced by a regular (non-bitcode) object.
   unsigned IsUsedInRegularObj : 1;
+  unsigned SignatureMismatch : 1;
 
 protected:
   Symbol(StringRef Name, Kind K, uint32_t Flags, InputFile *F)
-      : IsUsedInRegularObj(false), Name(Name), SymbolKind(K), Flags(Flags),
-        File(F), Referenced(!Config->GcSections) {}
+      : IsUsedInRegularObj(false), SignatureMismatch(false), Name(Name),
+        SymbolKind(K), Flags(Flags), File(F), Referenced(!Config->GcSections) {}
 
   StringRef Name;
   Kind SymbolKind;
   uint32_t Flags;
   InputFile *File;
   uint32_t OutputSymbolIndex = INVALID_INDEX;
   bool Referenced;
 };
static_assert(alignof(T) <= alignof(SymbolUnion),
                 "SymbolUnion not aligned enough");
   assert(static_cast<Symbol *>(static_cast<T *>(nullptr)) == nullptr &&
          "Not a Symbol");
 
   Symbol SymCopy = *S;
 
   T *S2 = new (S) T(std::forward<ArgT>(Arg)...);
   S2->IsUsedInRegularObj = SymCopy.IsUsedInRegularObj;
+  S2->SignatureMismatch = SymCopy.SignatureMismatch;
   return S2;
 }
 
 } // namespace wasm
 
 // Returns a symbol name for an error message.
 std::string toString(const wasm::Symbol &Sym);
 std::string toString(wasm::Symbol::Kind Kind);
 
 } // namespace lld
 
 #endif