[lld][WebAssembly] Where possible handle signature mismatches via an adaptor function

[sbc100]

Summary

This is similar to what we do at the bitcode level with the
WebAssemblyFixFunctionBitcasts pass but implemented at the object
file level.

Previously when we had caller and callee disagree about the function
signature we generated a warning replaced the call with a call to
a dummy function that contained only unreachable.

After this change we still generate the warning but will also then
try to generate an adapter function that will add or remove arguments
such that call can still possible succeed.

[sbc100]

.

[ruiu]

I'm not sure if there's a standard for the wasm object files and how the linker works, but if there's any, could you add a pointer to the document to the comment?

lld/wasm/SymbolTable.cpp

592

This function seems to enables to call an external function with a less number of arguments. I wonder what is the use case of this -- as long as you have a correct header file for functions, compilers can tell users that the number of parameters doesn't match.

[sbc100]

The primary documentation we have is at : https://github.com/WebAssembly/tool-conventions/blob/master/Linking.md

lld/wasm/SymbolTable.cpp

592

Yes, normally this shouldn't happen, but sadly there are some cases in C when it does.

The motivating case here is crt1.c calling the main function from _start. We want to be able support both 3 argument and 2 arguments forms. With native linking you can simply link these two together it will kind of "just work". With wasm, before this change we end up generating a linker warning and _start ends up calling stub function, so the program doesn't work.

This change fixes that use case.

[ruiu]

lld/wasm/SymbolTable.cpp

592

I'm curious if it makes sense to limit this functionality only to "main" if there's no other use cases, so that we don't accidentally link a wrong program as if it were a correct one. What do you think?

[sbc100]

lld/wasm/SymbolTable.cpp

592

We have seen other examples of code that does this. I remember another case in the mucl c library, although I think most of them are actual bugs that should be fixed in the code. One example is cmake which tests for the presence of a function by compiling a simple test file containing a call the the function, but the function always declared with no args. e.g. it tests for printf by compiling and linking a call to a no-arg version of printf.

We already successfully link such broken programs which is why cmake works today (via replaceWithUnreachable). This change just takes it one step further and allows some such programs to also execute.

With this change we still do give a warning (and that be be turned into an error) so we are still encouraging developers to fix their code. But we want to be permissive here to allow as much legacy code as possible o be compiled and run.

[ruiu]

lld/wasm/SymbolTable.cpp

592

Thank you for the detailed explanation. That helped me a lot. Could you add that here as a comment?

[sbc100]

• add docs

[sbc100]

• rebase

[sbc100]

friendly ping..

[sbc100]

Add @sunfish who suggested maybe we don't this might be necessary since the issue with the zero argument main is already handled at compile time.

I still think this change is somewhat nicer, and it means we can drop the magic handling of main from the compiler.

[ruiu]

Do you want to land this or abandon? I think I'm fine either relying on a compiler magic or landing this patch (and remove the compiler hack), but I'm not very happy about landing this and leaving the compiler hack, so I guess you need to make your mind.

[sbc100]

In D68751#1709224, @ruiu wrote:

Do you want to land this or abandon? I think I'm fine either relying on a compiler magic or landing this patch (and remove the compiler hack), but I'm not very happy about landing this and leaving the compiler hack, so I guess you need to make your mind.

That sounds reasonable. We can take some time to consider which option is better and either abandon this change to land it along with the removal of the compiler hack.

[sbc100]

I'm going at abandon this for now.

lld/test/wasm/lto/signature-mismatch.ll

 ; RUN: llc -filetype=obj -o %t.o %s
 ; RUN: llvm-as %S/Inputs/archive.ll -o %t1.o
-; RUN: not wasm-ld --fatal-warnings %t.o %t1.o -o %t.wasm 2>&1 | FileCheck %s
+; RUN: wasm-ld %t.o %t1.o -o %t.wasm 2>&1 | FileCheck %s
 
 ; Test that functions defined in bitcode correctly report signature
 ; mistmaches with existing undefined sybmols in normal objects.
 
 target triple = "wasm32-unknown-unknown"
 
 ; f is defined to take no argument in archive.ll which is compiled to bitcode
 declare void @f(i32);
 
 define void @_start() {
   call void @f(i32 0)
   ret void
 }
 
-; CHECK: error: function signature mismatch: f
+; CHECK: warning: function signature mismatch: f
 ; CHECK: >>> defined as (i32) -> void in {{.*}}signature-mismatch.ll.tmp1.o
 ; CHECK: >>> defined as () -> void in lto.tmp

lld/wasm/InputChunks.h

public:
 
   StringRef getName() const override { return name; }
   StringRef getDebugName() const override { return debugName; }
   uint32_t getComdat() const override { return UINT32_MAX; }
 
   void setBody(ArrayRef<uint8_t> body_) { body = body_; }
 
 protected:
-  ArrayRef<uint8_t> data() const override { return body; }
+  ArrayRef<uint8_t> data() const override {
+    assert(!body.empty());
+    return body;
+  }
 
   StringRef name;
   StringRef debugName;
   ArrayRef<uint8_t> body;
 };
 
 // Represents a single Wasm Section within an input file.
 class InputSection : public InputChunk {

lld/wasm/MarkLive.cpp

void MarkLive::run() {
 
   if (config->isPic)
     enqueue(WasmSym::callCtors);
 
   if (config->sharedMemory && !config->shared)
     enqueue(WasmSym::initMemory);
 
   mark();
+
+  // For adaptor functions, if that adaptor is live then the target of that
+  // adaptor needs to be too.   Normally relocations take care of this but
+  // the adaptors are synthetic and don't carry relocation information.
+  for (auto pair : symtab->adaptorFunctions)
+    if (pair.first->isLive())
+      enqueue(pair.second);
+
+  mark();
 }
 
 void MarkLive::mark() {
   // Follow relocations to mark all reachable chunks.
   while (!queue.empty()) {
     InputChunk *c = queue.pop_back_val();
 
     for (const WasmRelocation reloc : c->getRelocations()) {

lld/wasm/SymbolTable.h

public:
   DefinedGlobal *addSyntheticGlobal(StringRef name, uint32_t flags,
                                     InputGlobal *global);
   DefinedFunction *addSyntheticFunction(StringRef name, uint32_t flags,
                                         InputFunction *function);
   DefinedData *addOptionalDataSymbol(StringRef name, uint32_t value = 0);
 
   void handleSymbolVariants();
   void handleWeakUndefines();
+  void createAdaptorFunctions();
 
   std::vector<ObjFile *> objectFiles;
   std::vector<SharedFile *> sharedFiles;
   std::vector<BitcodeFile *> bitcodeFiles;
   std::vector<InputFunction *> syntheticFunctions;
   std::vector<InputGlobal *> syntheticGlobals;
 
+  std::vector<std::pair<DefinedFunction *, FunctionSymbol *>> adaptorFunctions;
+
 private:
   std::pair<Symbol *, bool> insert(StringRef name, const InputFile *file);
   std::pair<Symbol *, bool> insertName(StringRef name);
 
   bool getFunctionVariant(Symbol* sym, const WasmSignature *sig,
                           const InputFile *file, Symbol **out);
-  InputFunction *replaceWithUnreachable(Symbol *sym, const WasmSignature &sig,
-                                        StringRef debugName);
+  InputFunction *replaceWithUnreachable(Symbol *sym, StringRef debugName);
+  bool replaceWithAdaptorFunction(FunctionSymbol *sym, FunctionSymbol *target);
 
   // Maps symbol names to index into the symVector.  -1 means that symbols
   // is to not yet in the vector but it should have tracing enabled if it is
   // ever added.
   llvm::DenseMap<llvm::CachedHashStringRef, int> symMap;
   std::vector<Symbol *> symVector;
 
   // For certain symbols types, e.g. function symbols, we allow for muliple

lld/wasm/SymbolTable.cpp

void SymbolTable::wrap(Symbol *sym, Symbol *real, Symbol *wrap) {
   origIdx = wrapIdx;
 }
 
 static const uint8_t unreachableFn[] = {
     0x03 /* ULEB length */, 0x00 /* ULEB num locals */,
     0x00 /* opcode unreachable */, 0x0b /* opcode end */
 };
 
+// Replace the given symbol body with an adaptor function which adds or removes
+// arguments in order to call a function with a different signature.
+// This can only handle adding or removing arguments, if any of the argument
+// types don't match the adaptor creation will fail.
+// See handleSymbolVariants add replaceWithUnreachable.
+bool SymbolTable::replaceWithAdaptorFunction(FunctionSymbol *sym,
+                                             FunctionSymbol *target) {
+  LLVM_DEBUG(dbgs() << "replaceWithAdaptorFunction: " << sym->getName() << "\n");
+  const WasmSignature &srcSig = *sym->getSignature();
+  const WasmSignature &targetSig = *target->getSignature();
+
+  if (srcSig.Returns != targetSig.Returns) {
+    LLVM_DEBUG(dbgs() << "return type mismatch\n");
+    return false;
+  }
+
+  for (size_t i = 0; i < srcSig.Params.size() && i < targetSig.Params.size();
+       i++) {
+    if (srcSig.Params[i] != targetSig.Params[i]) {
+      LLVM_DEBUG(dbgs() << "param type mismatch\n");
+      return false;
+    }
+  }
+
+  StringRef name = saver.save("adaptor:" + sym->getName());
+  auto *func = make<SyntheticFunction>(*sym->getSignature(), name, name);
+  syntheticFunctions.emplace_back(func);
+  auto defined = replaceSymbol<DefinedFunction>(sym, sym->getName(), sym->getFlags(), nullptr,
+                                                func);
+  adaptorFunctions.emplace_back(defined, target);
+  return true;
+}
+
+void SymbolTable::createAdaptorFunctions() {
+  for (auto pair : adaptorFunctions) {
+    DefinedFunction* sym = pair.first;
+    FunctionSymbol* target = pair.second;
+    const WasmSignature &srcSig = *sym->getSignature();
+    const WasmSignature &targetSig = *target->getSignature();
+    std::string bodyContent;
+    {
+      raw_string_ostream os(bodyContent);
+      writeUleb128(os, 0, "num locals");
+      for (size_t i = 0; i < targetSig.Params.size(); i++) {
+        if (i < srcSig.Params.size()) {
+          writeU8(os, WASM_OPCODE_LOCAL_GET, "local.get");
+          writeSleb128(os, i, "param index");
+        } else {
+          switch (targetSig.Params[i]) {
+          case ValType::I32:
+            writeU8(os, WASM_OPCODE_I32_CONST, "i32.const");
+            break;
+          case ValType::I64:
+            writeU8(os, WASM_OPCODE_I32_CONST, "i64.const");
+            break;
+          case ValType::F32:
+            writeU8(os, WASM_OPCODE_I32_CONST, "f32.const");
+            break;
+          case ValType::F64:
+            writeU8(os, WASM_OPCODE_I32_CONST, "f64.const");
+            break;
+          default:
+            llvm_unreachable("unhandled param type");
+          }
+          writeSleb128(os, 0, "zero");
+        }
+      }
+      writeU8(os, WASM_OPCODE_CALL, "CALL");
+      writeUleb128(os, target->getFunctionIndex(), "function index");
+      writeU8(os, WASM_OPCODE_END, "END");
+    }
+
+    std::string functionBody;
+    {
+      raw_string_ostream os(functionBody);
+      writeUleb128(os, bodyContent.size(), "function size");
+      os << bodyContent;
+    }
+    ArrayRef<uint8_t> body = arrayRefFromStringRef(saver.save(functionBody));
+    cast<SyntheticFunction>(sym->function)->setBody(body);
+  }
+}
+
 // Replace the given symbol body with an unreachable function.
 // This is used by handleWeakUndefines in order to generate a callable
 // equivalent of an undefined function and also handleSymbolVariants for
 // undefined functions that don't match the signature of the definition.
 InputFunction *SymbolTable::replaceWithUnreachable(Symbol *sym,
-                                                   const WasmSignature &sig,
                                                    StringRef debugName) {
-  auto *func = make<SyntheticFunction>(sig, sym->getName(), debugName);
+  auto *func =
+      make<SyntheticFunction>(*sym->getSignature(), sym->getName(), debugName);
   func->setBody(unreachableFn);
   syntheticFunctions.emplace_back(func);
   replaceSymbol<DefinedFunction>(sym, sym->getName(), sym->getFlags(), nullptr,
                                  func);
   return func;
 }
 
 // For weak undefined functions, there may be "call" instructions that reference
 // the symbol. In this case, we need to synthesise a dummy/stub function that
 // will abort at runtime, so that relocations can still provided an operand to
 // the call instruction that passes Wasm validation.
 void SymbolTable::handleWeakUndefines() {
   for (Symbol *sym : getSymbols()) {
     if (!sym->isUndefWeak())
       continue;
 
-    const WasmSignature *sig = sym->getSignature();
-    if (!sig) {
+    if (!sym->getSignature()) {
       // It is possible for undefined functions not to have a signature (eg. if
       // added via "--undefined"), but weak undefined ones do have a signature.
       // Lazy symbols may not be functions and therefore Sig can still be null
       // in some circumstantce.
       assert(!isa<FunctionSymbol>(sym));
       continue;
     }
 
     // Add a synthetic dummy for weak undefined functions.  These dummies will
     // be GC'd if not used as the target of any "call" instructions.
     StringRef debugName = saver.save("undefined:" + toString(*sym));
-    InputFunction* func = replaceWithUnreachable(sym, *sig, debugName);
+    InputFunction* func = replaceWithUnreachable(sym, debugName);
     // Ensure it compares equal to the null pointer, and so that table relocs
     // don't pull in the stub body (only call-operand relocs should do that).
     func->setTableIndex(0);
     // Hide our dummy to prevent export.
     sym->setHidden(true);
   }
 }
 
 static void reportFunctionSignatureMismatch(StringRef symName,
                                             FunctionSymbol *a,
                                             FunctionSymbol *b, bool isError) {
   std::string msg = ("function signature mismatch: " + symName +
                      "\n>>> defined as " + toString(*a->signature) + " in " +
                      toString(a->getFile()) + "\n>>> defined as " +
                      toString(*b->signature) + " in " + toString(b->getFile()))
                         .str();
   if (isError)
     error(msg);
   else
     warn(msg);
 }
 
-// Remove any variant symbols that were created due to function signature
+// Resolve any "variant" symbols that were created due to function signature
 // mismatches.
+// These variants are created when different compilation units disagree about
+// the signature of a function.  Normally this represents a bug in the program
+// but there are several use cases that rely on the linker being able to
+// successfully link such programs.  Two motivating examples:
+//
+// 1) When _start (crt1.o) calls main we want to be able to support both
+//    0-argument, 2-argument and 3-argument forms.
+// 2) When cmake tests for the existence of a function it compiles a tiny
+//    program containing a reference the function, but does not use the
+//    functions correct signature.
+//
+// When we encounter mismatches we take two approaches to resolving them:
+//
+// 1) Attempt to create a wrapper function that either drops arguments or
+//    adds extern arguments containing zeros.  This only works if the return
+//    types match that the argument that they both share also match.
+// 2) If we fail to create the wrapper then we create a dummy function with the
+//    correct signature that will trap at runtime.
+//
+// In either case we issue a warning since the resulting program is unlikely to
+// be correct.
 void SymbolTable::handleSymbolVariants() {
   for (auto pair : symVariants) {
     // Push the initial symbol onto the list of variants.
     StringRef symName = pair.first.val();
     std::vector<Symbol *> &variants = pair.second;
 
 #ifndef NDEBUG
     LLVM_DEBUG(dbgs() << "symbol with (" << variants.size()
if (!defined) {
                                       cast<FunctionSymbol>(variants[1]), true);
       return;
     }
 
     for (auto *symbol : variants) {
       if (symbol != defined) {
         auto *f = cast<FunctionSymbol>(symbol);
         reportFunctionSignatureMismatch(symName, f, defined, false);
+        // First attempt to replace the callee with an adaptor function.  If
+        // that fails fall back to creating a dummy function that will trap at
+        // runtime.
+        if (!replaceWithAdaptorFunction(f, defined)) {
           StringRef debugName = saver.save("unreachable:" + toString(*f));
-        replaceWithUnreachable(f, *f->signature, debugName);
+          replaceWithUnreachable(f, debugName);
+        }
       }
     }
   }
 }
 
 } // namespace wasm
 } // namespace lld

lld/wasm/Writer.cpp

void Writer::run() {
   }
 
   log("-- scanRelocations");
   scanRelocations();
   log("-- assignIndexes");
   assignIndexes();
   log("-- calculateInitFunctions");
   calculateInitFunctions();
+  log("-- createAdaptorFunctions");
+  symtab->createAdaptorFunctions();
 
   if (!config->relocatable) {
     // Create linker synthesized functions
     if (config->sharedMemory)
       createInitMemoryFunction();
     if (config->isPic)
       createApplyRelocationsFunction();
     createCallCtorsFunction();