[WebAssembly] Make function signature checks into warning

[sbc100]

Summary

Also add --fatal-warnings to match the ELF and COFF backend
behaviour.

[ncw]

Sorry to poke my nose in, I'm just curious as to the rationale on why this shouldn't be a link error? We know for certain at this point that the output Wasm file won't validate, so we have effectively failed to link the inputs together.

[sbc100]

In D42652#991125, @ncw wrote:

Sorry to poke my nose in, I'm just curious as to the rationale on why this shouldn't be a link error? We know for certain at this point that the output Wasm file won't validate, so we have effectively failed to link the inputs together.

Mostly because turning it on breaks so much existing code. For example, musl has several such issues in it, which would mean that most of our tests on the wasm waterfall would stop linking if we enable this today. For what its worth this is completely off by default right now. So unless you explictly ask for it the linker will be silent about these errors.

I think it is a worthy goal to enable this by default one day.

Mostly I was looking for a warning to test the --fatal-warnings flag and these seemed like a good one because I imagine turning on by default as a warning first. AFAICT there are no other warnings in the wasm linker today.. so maybe we don't need this flag at all?

[ncw]

In D42652#991174, @sbc100 wrote:

Mostly because turning it on breaks so much existing code. For example, musl has several such issues in it, which would mean that most of our tests on the wasm waterfall would stop linking if we enable this today. For what its worth this is completely off by default right now. So unless you explictly ask for it the linker will be silent about these errors.

I think it is a worthy goal to enable this by default one day.

Ah, now I see! Thanks. There can't be many broken prototypes in Musl? I'm aware of one, where __libc_start_main is called with an intentionally wrong prototype, but that can be fixed with an arch-specific override. It's still a bit off if it currently works, since the resulting Wasm file won't be runnable if the types don't match.

Perhaps some of the errors are instances of the bug (https://bugs.llvm.org/show_bug.cgi?id=35385), which results in correct declarations getting a messed-up signature mismatch.

[sbc100]

The biggest culprit I've seen so far is the declaration of main() itself. We are talking about special casing this in the frontend since the standard allows it to have two different signatures.

Outside of that, we need to see just how widespread the signature mismatch issue is. If it only effects a few existing projects they it might be feasible the enable this error by default and to fix those projects. If such violations are common we may need to do some more cleverness/mitigations in the linker.

Often times the resulting modules can actually be runable, if the function is called via call_indirect(). In this case the resulting program will fail at run time rather than validation time, and only if that particular function is called. One can imagine a project that includes such code, but in practice never execute it. Indeed this is the case for musl, which calls main, but indirectly in its _start function. As long as we don't actually call its start (i.e if we call main() directly instead) that module is still usable.

[sbc100]

I think I'm going to abandon this change because I'm artificially creating a warning where we have none otherwise. Seems backwards.

[sbc100]

Resurrected this in a slightly different form: https://reviews.llvm.org/D46484

test/wasm/fatal-warnings.ll

+; RUN: llc -filetype=obj %s -o %t.main.o
+; RUN: lld -flavor wasm --check-signatures -o %t.wasm %t.main.o 2>&1 | FileCheck %s
+
+target triple = "wasm32-unknown-unknown-wasm"
+
+define hidden i32 @_start(i32 %arg) local_unnamed_addr {
+entry:
+  ret i32 %arg
+}
+
+; CHECK: warning: function signature mismatch: _start
+
+; RUN: not lld -flavor wasm --check-signatures --fatal-warnings -o %t.wasm %t.main.o 2>&1 | FileCheck %s -check-prefix=CHECK-FATAL
+
+; CHECK-FATAL: error: function signature mismatch: _start

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 lld -flavor wasm --check-signatures -o %t.wasm %t.main.o %t.ret32.o 2>&1 | FileCheck %s
+; RUN: not lld -flavor wasm --check-signatures --fatal-warnings -o %t.wasm %t.main.o %t.ret32.o 2>&1 | FileCheck %s
 
 target triple = "wasm32-unknown-unknown-wasm"
 
 ; 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-NEXT: >>> defined as (I32, I64, I32) -> I32 in {{.*}}.main.o
 ; CHECK-NEXT: >>> defined as (F32) -> I32 in {{.*}}.ret32.o

wasm/Driver.cpp

if (Args.hasArg(OPT_version) || Args.hasArg(OPT_v)) {
     outs() << getLLDVersion() << "\n";
     return;
   }
 
   Config->AllowUndefined = Args.hasArg(OPT_allow_undefined);
   Config->CheckSignatures =
       Args.hasFlag(OPT_check_signatures, OPT_no_check_signatures, false);
   Config->Entry = getEntry(Args, Args.hasArg(OPT_relocatable) ? "" : "_start");
+  errorHandler().FatalWarnings =
+      Args.hasFlag(OPT_fatal_warnings, OPT_no_fatal_warnings, false);
   Config->ImportMemory = Args.hasArg(OPT_import_memory);
   Config->OutputFile = Args.getLastArgValue(OPT_o);
   Config->Relocatable = Args.hasArg(OPT_relocatable);
   Config->SearchPaths = args::getStrings(Args, OPT_L);
   Config->StripAll = Args.hasArg(OPT_strip_all);
   Config->StripDebug = Args.hasArg(OPT_strip_debug);
   errorHandler().Verbose = Args.hasArg(OPT_verbose);
   ThreadsEnabled = Args.hasFlag(OPT_threads, OPT_no_threads, true);

wasm/SymbolTable.cpp

static void checkSymbolTypes(const Symbol &Existing, const InputFile &F,
 
   DEBUG(dbgs() << "checkSymbolTypes: " << Existing.getName() << "\n");
   assert(NewSig);
 
   const WasmSignature &OldSig = Existing.getFunctionType();
   if (*NewSig == OldSig)
     return;
 
-  error("function signature mismatch: " + Existing.getName() +
-        "\n>>> defined as " + toString(OldSig) + " in " +
-        toString(Existing.getFile()) + "\n>>> defined as " + toString(*NewSig) +
-        " in " + F.getName());
+  warn("function signature mismatch: " + Existing.getName() +
 }
 
 static void checkSymbolTypes(const Symbol &Existing, const InputFile &F,
                              Symbol::Kind Kind, const InputChunk *Chunk) {
   const WasmSignature *Sig = nullptr;
   if (auto *F = dyn_cast_or_null<InputFunction>(Chunk))
     Sig = &F->Signature;
   return checkSymbolTypes(Existing, F, Kind, Sig);