[PATCH] D57909: [WebAssembly] Don't generate invalid modules when function signatures mismatch

[Rui Ueyama via Phabricator via llvm-commits]

ruiu added inline comments.


================
Comment at: docs/WebAssembly.rst:101-106
+One way in which the WebAssembly linker differs from tradiation native linkers
+is that function signature checking is strict in WebAssembly.  i.e. its not
+possible to call a function with the wrong signature.  Even though this is
+undefined behaviour in C/C++ its not uncommon to find this in real world C/C++
+programs.  For example, a call site in one complication unit which calls a
+function defined in another complication unit but with too many arguments.
----------------
Could you update this comment a bit to reflect the discussion we've had so far? I think that it should mention the following things:

 - The runtime verifier rejects modules if it finds a function type mismatch
 - So there's no point of creating a module by ignoring type mismatch errors
 - But sometimes link failure as a result of a type mismatch is too strict; you want to create an executable even if it contains a few errors.
 - So lld/wasm supports two modes: one is to report an error for type mismatch and exit. The other is to create a stub function for an erroneous function, so that the generated file at least passes the verifier, which effectively turns a type error into a runtime error. The latter is the default mode.


================
Comment at: wasm/Options.td:94
 
+def signature_check_strict : F<"signature-check-strict">,
+                             HelpText<"Error on function signature mismatch">;
----------------
Remove a space before `:`. Indent with two spaces.


================
Comment at: wasm/SymbolTable.cpp:311
 
-  if (Function)
-    checkFunctionType(S, File, &Function->Signature);
+  bool createdNewVariant = false;
+  if (Function && !checkFunctionType(S, File, &Function->Signature))
----------------
created -> Created


================
Comment at: wasm/SymbolTable.cpp:410-411
     Lazy->fetch();
   else
-    checkFunctionType(S, File, Sig);
+    if (!checkFunctionType(S, File, Sig))
+      if (createFunctionVariant(Name, Sig, File, &S))
----------------
`else if` should be on the same line.


================
Comment at: wasm/SymbolTable.cpp:571-572
+
+// Remove any variant symbols that were created due to function signature
+// mismatches.
+void SymbolTable::handleSymbolVariants() {
----------------
It looks like you are trying to reduce the number of stub functions, by generating only one stub for each function type. But is this necessary? Given that no production code will contain a stub, no one cares if a binary with type mismatch contains many duplicated stubs?


Repository:
  rLLD LLVM Linker

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D57909/new/

https://reviews.llvm.org/D57909