[WebAssembly] Relax signature checking for undefined functions that are not called directly

[sbc100]

Summary

When function signatures don't match and the undefined function is not
called directly (i.e. only has its address taken) we don't issue a
warning or create a runtime thunk for the undefined function.

Instead in this case we simply use the defined version of the function.
This is possible since checking signatures of dynamic calls happens
at runtime so any invalid usage will still result in a runtime error.

This is needed to allow C++ programs to link without generating
warnings. Its not uncommon in C++ for vtables to be populated by
function address whee the signature of the function is not known in the
compilation unit. In this case clang delcares the method as void(void)
and relies on the vtable caller casting the data back to the correct
signature.

Fixes: https://bugs.llvm.org/show_bug.cgi?id=40412

[sbc100]

• comments

[sunfish]

I don't know the lld internals well, but the basic approach here sounds right.

[sbc100]

reserve -> resize

[sbc100]

• comments

[sbc100]

@ruiu PTAL

[sbc100]

ping

[sbc100]

@ruiu thanks for all the reviews of late. Any chance you could take a quick look at this one today too? Otherwise perhaps @dschuff can lg?

[dschuff]

Otherwise LGTM

lld/wasm/InputFiles.cpp

280 ↗

(On Diff #200945)

Since this is a validation of external input, should we use report_fatal_error or some equivalent instead of assert?

337 ↗

(On Diff #200945)

exclued?

[sbc100]

• feedback

lld/wasm/InputFiles.cpp

280 ↗

(On Diff #200945)

This should already have been validated by libObject so I think this is ok .g

[ruiu]

lld/trunk/wasm/InputFiles.h

78

I'm not sure if I understand the code correctly. It seems like a bit indicating whether a symbol is called directly or not should logically belong to each Symbol object rather than a side table inside a file object, no? Let's say one file contains an undefined symbol, the other file contains a defined symbol, and the undefined symbol is called directly. If the linker merges the two symbols, the _defined_ symbol is called directly, isn't it?

[sbc100]

lld/trunk/wasm/InputFiles.h

78

This information is tracked in the symbol, but we only need to track it for undefined functions.

This data structure is used only temporarily so that the symbols can be constructed correctly. I've made followup that should make this more obvious: https://reviews.llvm.org/D62548.

lld/trunk/test/wasm/signature-mismatch-unknown.ll

+; RUN: llc -filetype=obj %p/Inputs/ret32.ll -o %t.ret32.o
+; RUN: llc -filetype=obj %s -o %t.main.o
+; RUN: wasm-ld --fatal-warnings -o %t.wasm %t.ret32.o %t.main.o
+; RUN: wasm-ld --fatal-warnings -o %t.wasm %t.main.o %t.ret32.o
+
+target triple = "wasm32-unknown-unknown"
+
+; Function declartion with incorrect signature.
+declare dso_local void @ret32()
+
+; Simply taking the address of the function should *not* generate the
+; the signature mismatch warning.
+@ptr = dso_local global i8* bitcast (void ()* @ret32 to i8*), align 8
+
+define hidden void @_start() local_unnamed_addr {
+  %addr = load i32 ()*, i32 ()** bitcast (i8** @ptr to i32 ()**), align 8
+  call i32 %addr()
+  ret void
+}

lld/trunk/wasm/Driver.cpp

 // This data structure is instantiated for each -wrap option.
 struct WrappedSymbol {
   Symbol *Sym;
   Symbol *Real;
   Symbol *Wrap;
 };
 
 static Symbol *addUndefined(StringRef Name) {
-  return Symtab->addUndefinedFunction(Name, "", "", 0, nullptr, nullptr);
+  return Symtab->addUndefinedFunction(Name, "", "", 0, nullptr, nullptr, false);
 }
 
 // Handles -wrap option.
 //
 // This function instantiates wrapper symbols. At this point, they seem
 // like they are not being used at all, so we explicitly set some flags so
 // that LTO won't eliminate them.
 static std::vector<WrappedSymbol> addWrappedSymbols(opt::InputArgList &Args) {

lld/trunk/wasm/InputFiles.h

public:
   MutableArrayRef<Symbol *> getMutableSymbols() { return Symbols; }
 
 protected:
   InputFile(Kind K, MemoryBufferRef M) : MB(M), FileKind(K) {}
   MemoryBufferRef MB;
 
   // List of all symbols referenced or defined by this file.
   std::vector<Symbol *> Symbols;
+  // Bool for each symbol, true if called directly.  This allows us to implement
+  // a weaker form of signature checking where undefined functions that are not
+  // called directly (i.e. only address taken) don't have to match the defined
+  // function's signature.  We cannot do this for directly called functions
+  // because those signatures are checked at validation times.
+  // See https://bugs.llvm.org/show_bug.cgi?id=40412
+  std::vector<bool> SymbolIsCalledDirectly;
 
 private:
   const Kind FileKind;
 };
 
 // .a file (ar archive)
 class ArchiveFile : public InputFile {
 public:
public:
   FunctionSymbol *getFunctionSymbol(uint32_t Index) const;
   DataSymbol *getDataSymbol(uint32_t Index) const;
   GlobalSymbol *getGlobalSymbol(uint32_t Index) const;
   SectionSymbol *getSectionSymbol(uint32_t Index) const;
   EventSymbol *getEventSymbol(uint32_t Index) const;
 
 private:
   Symbol *createDefined(const WasmSymbol &Sym);
-  Symbol *createUndefined(const WasmSymbol &Sym);
+  Symbol *createUndefined(const WasmSymbol &Sym, bool IsCalledDirectly);
 
   bool isExcludedByComdat(InputChunk *Chunk) const;
 
   std::unique_ptr<WasmObjectFile> WasmObj;
 };
 
 // .so file.
 class SharedFile : public InputFile {

lld/trunk/wasm/InputFiles.cpp

for (const WasmElemSegment &Seg : WasmObj->elements()) {
     uint32_t Offset = Seg.Offset.Value.Int32;
     for (uint32_t Index = 0; Index < Seg.Functions.size(); Index++) {
 
       uint32_t FunctionIndex = Seg.Functions[Index];
       TableEntries[FunctionIndex] = Offset + Index;
     }
   }
 
-  // Find the code and data sections.  Wasm objects can have at most one code
-  // and one data section.
   uint32_t SectionIndex = 0;
+  SymbolIsCalledDirectly.resize(WasmObj->getNumberOfSymbols(), false);
   for (const SectionRef &Sec : WasmObj->sections()) {
     const WasmSection &Section = WasmObj->getWasmSection(Sec);
+    // Wasm objects can have at most one code and one data section.
     if (Section.Type == WASM_SEC_CODE) {
+      assert(!CodeSection);
       CodeSection = &Section;
     } else if (Section.Type == WASM_SEC_DATA) {
+      assert(!DataSection);
       DataSection = &Section;
     } else if (Section.Type == WASM_SEC_CUSTOM) {
       CustomSections.emplace_back(make<InputSection>(Section, this));
       CustomSections.back()->setRelocations(Section.Relocations);
       CustomSectionsByIndex[SectionIndex] = CustomSections.back();
     }
     SectionIndex++;
+    // Scans relocations to dermine determine if a function symbol is called
+    // directly
+    for (const WasmRelocation &Reloc : Section.Relocations)
+      if (Reloc.Type == R_WASM_FUNCTION_INDEX_LEB)
+        SymbolIsCalledDirectly[Reloc.Index] = true;
   }
 
   TypeMap.resize(getWasmObj()->types().size());
   TypeIsUsed.resize(getWasmObj()->types().size(), false);
 
   ArrayRef<StringRef> Comdats = WasmObj->linkingData().Comdats;
   for (unsigned I = 0; I < Comdats.size(); ++I)
     if (IgnoreComdats)
void ObjFile::parse(bool IgnoreComdats) {
   // Populate `Events`.
   for (const WasmEvent &E : WasmObj->events())
     Events.emplace_back(make<InputEvent>(Types[E.Type.SigIndex], E, this));
 
   // Populate `Symbols` based on the WasmSymbols in the object.
   Symbols.reserve(WasmObj->getNumberOfSymbols());
   for (const SymbolRef &Sym : WasmObj->symbols()) {
     const WasmSymbol &WasmSym = WasmObj->getWasmSymbol(Sym.getRawDataRefImpl());
-    if (Symbol *Sym = createDefined(WasmSym))
-      Symbols.push_back(Sym);
-    else
-      Symbols.push_back(createUndefined(WasmSym));
+    if (WasmSym.isDefined()) {
+      // createDefined may fail if the symbol is comdat excluded in which case
+      // we fall back to creating an undefined symbol
+      if (Symbol *D = createDefined(WasmSym)) {
+        Symbols.push_back(D);
+        continue;
+      }
+    }
+    size_t Idx = Symbols.size();
+    Symbols.push_back(createUndefined(WasmSym, SymbolIsCalledDirectly[Idx]));
   }
 }
 
 bool ObjFile::isExcludedByComdat(InputChunk *Chunk) const {
   uint32_t C = Chunk->getComdat();
   if (C == UINT32_MAX)
     return false;
   return !KeptComdats[C];
SectionSymbol *ObjFile::getSectionSymbol(uint32_t Index) const {
   return cast<SectionSymbol>(Symbols[Index]);
 }
 
 DataSymbol *ObjFile::getDataSymbol(uint32_t Index) const {
   return cast<DataSymbol>(Symbols[Index]);
 }
 
 Symbol *ObjFile::createDefined(const WasmSymbol &Sym) {
-  if (!Sym.isDefined())
-    return nullptr;
-
   StringRef Name = Sym.Info.Name;
   uint32_t Flags = Sym.Info.Flags;
 
   switch (Sym.Info.Kind) {
   case WASM_SYMBOL_TYPE_FUNCTION: {
     InputFunction *Func =
         Functions[Sym.Info.ElementIndex - WasmObj->getNumImportedFunctions()];
     if (isExcludedByComdat(Func)) {
case WASM_SYMBOL_TYPE_EVENT: {
     if (Sym.isBindingLocal())
       return make<DefinedEvent>(Name, Flags, this, Event);
     return Symtab->addDefinedEvent(Name, Flags, this, Event);
   }
   }
   llvm_unreachable("unknown symbol kind");
 }
 
-Symbol *ObjFile::createUndefined(const WasmSymbol &Sym) {
+Symbol *ObjFile::createUndefined(const WasmSymbol &Sym, bool IsCalledDirectly) {
   StringRef Name = Sym.Info.Name;
   uint32_t Flags = Sym.Info.Flags;
 
   switch (Sym.Info.Kind) {
   case WASM_SYMBOL_TYPE_FUNCTION:
     return Symtab->addUndefinedFunction(Name, Sym.Info.ImportName,
                                         Sym.Info.ImportModule, Flags, this,
-                                        Sym.Signature);
+                                        Sym.Signature, IsCalledDirectly);
   case WASM_SYMBOL_TYPE_DATA:
     return Symtab->addUndefinedData(Name, Flags, this);
   case WASM_SYMBOL_TYPE_GLOBAL:
     return Symtab->addUndefinedGlobal(Name, Sym.Info.ImportName,
                                       Sym.Info.ImportModule, Flags, this,
                                       Sym.GlobalType);
   case WASM_SYMBOL_TYPE_SECTION:
     llvm_unreachable("section symbols cannot be undefined");
static Symbol *createBitcodeSymbol(const std::vector<bool> &KeptComdats,
   Flags |= mapVisibility(ObjSym.getVisibility());
 
   int C = ObjSym.getComdatIndex();
   bool ExcludedByComdat = C != -1 && !KeptComdats[C];
 
   if (ObjSym.isUndefined() || ExcludedByComdat) {
     if (ObjSym.isExecutable())
       return Symtab->addUndefinedFunction(Name, Name, DefaultModule, Flags, &F,
-                                          nullptr);
+                                          nullptr, true);
     return Symtab->addUndefinedData(Name, Flags, &F);
   }
 
   if (ObjSym.isExecutable())
     return Symtab->addDefinedFunction(Name, Flags, &F, nullptr);
   return Symtab->addDefinedData(Name, Flags, &F, nullptr, 0, 0);
 }
 

lld/trunk/wasm/SymbolTable.h

Symbol *addDefinedData(StringRef Name, uint32_t Flags, InputFile *File,
                          uint32_t Size);
   Symbol *addDefinedGlobal(StringRef Name, uint32_t Flags, InputFile *File,
                            InputGlobal *G);
   Symbol *addDefinedEvent(StringRef Name, uint32_t Flags, InputFile *File,
                           InputEvent *E);
 
   Symbol *addUndefinedFunction(StringRef Name, StringRef ImportName,
                                StringRef ImportModule, uint32_t Flags,
-                               InputFile *File, const WasmSignature *Signature);
+                               InputFile *File, const WasmSignature *Signature,
+                               bool IsCalledDirectly);
   Symbol *addUndefinedData(StringRef Name, uint32_t Flags, InputFile *File);
   Symbol *addUndefinedGlobal(StringRef Name, StringRef ImportName,
                              StringRef ImportModule,  uint32_t Flags,
                              InputFile *File, const WasmGlobalType *Type);
 
   void addLazy(ArchiveFile *F, const llvm::object::Archive::Symbol *Sym);
 
   bool addComdat(StringRef Name);

lld/trunk/wasm/SymbolTable.cpp

Symbol *SymbolTable::addDefinedFunction(StringRef Name, uint32_t Flags,
   }
 
   auto ExistingFunction = dyn_cast<FunctionSymbol>(S);
   if (!ExistingFunction) {
     reportTypeError(S, File, WASM_SYMBOL_TYPE_FUNCTION);
     return S;
   }
 
-  if (Function && !signatureMatches(ExistingFunction, &Function->Signature)) {
+  bool CheckSig = true;
+  if (auto UD = dyn_cast<UndefinedFunction>(ExistingFunction))
+    CheckSig = UD->IsCalledDirectly;
+
+  if (CheckSig && Function && !signatureMatches(ExistingFunction, &Function->Signature)) {
     Symbol* Variant;
     if (getFunctionVariant(S, &Function->Signature, File, &Variant))
       // New variant, always replace
       Replace(Variant);
     else if (shouldReplace(S, File, Flags))
       // Variant already exists, replace it after checking shouldReplace
       Replace(Variant);
 
Symbol *SymbolTable::addDefinedEvent(StringRef Name, uint32_t Flags,
   if (shouldReplace(S, File, Flags))
     Replace();
   return S;
 }
 
 Symbol *SymbolTable::addUndefinedFunction(StringRef Name, StringRef ImportName,
                                           StringRef ImportModule,
                                           uint32_t Flags, InputFile *File,
-                                          const WasmSignature *Sig) {
+                                          const WasmSignature *Sig,
+                                          bool IsCalledDirectly) {
   LLVM_DEBUG(dbgs() << "addUndefinedFunction: " << Name <<
              " [" << (Sig ? toString(*Sig) : "none") << "]\n");
 
   Symbol *S;
   bool WasInserted;
   std::tie(S, WasInserted) = insert(Name, File);
   if (S->Traced)
     printTraceSymbolUndefined(Name, File);
 
   auto Replace = [&]() {
     replaceSymbol<UndefinedFunction>(S, Name, ImportName, ImportModule, Flags,
-                                     File, Sig);
+                                     File, Sig, IsCalledDirectly);
   };
 
   if (WasInserted)
     Replace();
   else if (auto *Lazy = dyn_cast<LazySymbol>(S))
     Lazy->fetch();
   else {
     auto ExistingFunction = dyn_cast<FunctionSymbol>(S);
     if (!ExistingFunction) {
       reportTypeError(S, File, WASM_SYMBOL_TYPE_FUNCTION);
       return S;
     }
-    if (!signatureMatches(ExistingFunction, Sig))
+    if (IsCalledDirectly && !signatureMatches(ExistingFunction, Sig))
       if (getFunctionVariant(S, Sig, File, &S))
         Replace();
   }
 
   return S;
 }
 
 Symbol *SymbolTable::addUndefinedData(StringRef Name, uint32_t Flags,

lld/trunk/wasm/Symbols.h

public:
   InputFunction *Function;
 };
 
 class UndefinedFunction : public FunctionSymbol {
 public:
   UndefinedFunction(StringRef Name, StringRef ImportName,
                     StringRef ImportModule, uint32_t Flags,
                     InputFile *File = nullptr,
-                    const WasmSignature *Type = nullptr)
+                    const WasmSignature *Type = nullptr,
+                    bool IsCalledDirectly = true)
       : FunctionSymbol(Name, UndefinedFunctionKind, Flags, File, Type),
-        ImportName(ImportName), ImportModule(ImportModule) {}
+        ImportName(ImportName), ImportModule(ImportModule), IsCalledDirectly(IsCalledDirectly) {}
 
   static bool classof(const Symbol *S) {
     return S->kind() == UndefinedFunctionKind;
   }
 
   StringRef ImportName;
   StringRef ImportModule;
+  bool IsCalledDirectly;
 };
 
 // Section symbols for output sections are different from those for input
 // section.  These are generated by the linker and point the OutputSection
 // rather than an InputSection.
 class OutputSectionSymbol : public Symbol {
 public:
   OutputSectionSymbol(const OutputSection *S)