[lld][WebAssembly] Report undefined symbols during scanRelocations

[sbc100]

Summary

This puts handling of undefined symbols in a single location. Its
also more in line with the ELF backend which only reports undefined
symbols based on relocations.

One side effect is that we no longer report undefined symbols that are
only referenced in GC'd sections.

This also fixes a crash reported in the emscripten toolchain:
https://github.com/emscripten-core/emscripten/issues/8930.

[kripken]

This looks right to me, but I'm not an expert on this stuff.

[sbc100]

• Report undefined symbols when scanning relocations

[sbc100]

• cleanup test

[sbc100]

I expanded this change a little. @ruiu PTAL.

[ruiu]

LGTM

[MaskRay]

lld/wasm/Relocations.cpp

36 ↗

(On Diff #208351)

Not using early-return pattern may be cleaner here?

[sbc100]

• feedback

[sbc100]

• cleanup

lld/trunk/test/wasm/undefined-data.ll

 ; RUN: llc -filetype=obj %s -o %t.o
 ; RUN: not wasm-ld -o %t.wasm %t.o 2>&1 | FileCheck %s -check-prefix=UNDEF
-; RUN: not wasm-ld --allow-undefined -o %t.wasm %t.o 2>&1 | FileCheck %s -check-prefix=ALLOW
+; RUN: not wasm-ld --allow-undefined -o %t.wasm %t.o 2>&1 | FileCheck %s -check-prefix=UNDEF
 ; RUN: not wasm-ld --shared -o %t.wasm %t.o 2>&1 | FileCheck %s -check-prefix=SHARED
 
 target triple = "wasm32-unknown-unknown"
 
 @data_external = external global i32
 
 define i32 @_start() {
 entry:
     %0 = load i32, i32* @data_external, align 4
     ret i32 %0
 }
 
 ; UNDEF: error: {{.*}}undefined-data.ll.tmp.o: undefined symbol: data_external
-; ALLOW: error: {{.*}}undefined-data.ll.tmp.o: cannot resolve relocation of type R_WASM_MEMORY_ADDR_LEB against undefined (non-weak) data symbol: data_external
 ; SHARED: error: {{.*}}undefined-data.ll.tmp.o: relocation R_WASM_MEMORY_ADDR_LEB cannot be used against symbol data_external; recompile with -fPIC

lld/trunk/test/wasm/undefined.ll

 ; RUN: llc -filetype=obj %s -o %t.o
 ; RUN: wasm-ld --allow-undefined -o %t.wasm %t.o
 
-; Fails due to undefined 'foo' 
+; Fails due to undefined 'foo'
 ; RUN: not wasm-ld --undefined=baz -o %t.wasm %t.o 2>&1 | FileCheck %s
 ; CHECK: error: {{.*}}.o: undefined symbol: foo
 ; CHECK-NOT: undefined symbol: baz
 
 ; Succeeds if we pass a file containing 'foo' as --allow-undefined-file.
 ; RUN: echo 'foo' > %t.txt
 ; RUN: wasm-ld --allow-undefined-file=%t.txt -o %t.wasm %t.o
 
 ; Succeeds even if a missing symbol is added via --export
 ; RUN: wasm-ld --allow-undefined --export=xxx -o %t.wasm %t.o
 
 target triple = "wasm32-unknown-unknown"
 
 ; Takes the address of the external foo() resulting in undefined external
-@bar = hidden local_unnamed_addr global i8* bitcast (i32 ()* @foo to i8*), align 4
+@bar = global i8* bitcast (i32 ()* @foo to i8*), align 4
[e-mail] = appending global [1 x i8**] [i8** @bar], section "llvm.metadata"
 
 declare i32 @foo() #0
 
 define hidden void @_start() local_unnamed_addr #0 {
 entry:
     ret void
 }

lld/trunk/wasm/InputFiles.cpp

uint32_t ObjFile::calcNewValue(const WasmRelocation &Reloc) const {
   const Symbol* Sym = nullptr;
   if (Reloc.Type != R_WASM_TYPE_INDEX_LEB) {
     Sym = Symbols[Reloc.Index];
 
     // We can end up with relocations against non-live symbols.  For example
     // in debug sections.
     if ((isa<FunctionSymbol>(Sym) || isa<DataSymbol>(Sym)) && !Sym->isLive())
       return 0;
-
-    // Special handling for undefined data symbols.  Most relocations against
-    // such symbols cannot be resolved.
-    if (isa<DataSymbol>(Sym) && Sym->isUndefined()) {
-      if (Sym->isWeak() || Config->Relocatable)
-        return 0;
-      // R_WASM_MEMORY_ADDR_I32 relocations in PIC code are turned into runtime
-      // fixups in __wasm_apply_relocs
-      if (Config->Pic && Reloc.Type == R_WASM_MEMORY_ADDR_I32)
-        return 0;
-      if (Reloc.Type != R_WASM_GLOBAL_INDEX_LEB) {
-        llvm_unreachable(
-          ("invalid relocation against undefined data symbol: " + toString(*Sym))
-              .c_str());
-      }
-    }
   }
 
   switch (Reloc.Type) {
   case R_WASM_TABLE_INDEX_I32:
   case R_WASM_TABLE_INDEX_SLEB:
   case R_WASM_TABLE_INDEX_REL_SLEB:
     if (Config->Pic && !getFunctionSymbol(Reloc.Index)->hasTableIndex())
       return 0;
     return getFunctionSymbol(Reloc.Index)->getTableIndex();
   case R_WASM_MEMORY_ADDR_SLEB:
   case R_WASM_MEMORY_ADDR_I32:
   case R_WASM_MEMORY_ADDR_LEB:
   case R_WASM_MEMORY_ADDR_REL_SLEB:
+    if (isa<UndefinedData>(Sym))
+      return 0;
     return cast<DefinedData>(Sym)->getVirtualAddress() + Reloc.Addend;
   case R_WASM_TYPE_INDEX_LEB:
     return TypeMap[Reloc.Index];
   case R_WASM_FUNCTION_INDEX_LEB:
     return getFunctionSymbol(Reloc.Index)->getFunctionIndex();
   case R_WASM_GLOBAL_INDEX_LEB:
     if (auto GS = dyn_cast<GlobalSymbol>(Sym))
       return GS->getGlobalIndex();

lld/trunk/wasm/Relocations.cpp

 
 using namespace lld;
 using namespace lld::wasm;
 
 static bool requiresGOTAccess(const Symbol *Sym) {
   return Config->Pic && !Sym->isHidden() && !Sym->isLocal();
 }
 
+static bool allowUndefined(const Symbol* Sym) {
+  // Historically --allow-undefined doesn't work for data symbols since we don't
+  // have any way to represent these as imports in the final binary.  The idea
+  // behind allowing undefined symbols is to allow importing these symbols from
+  // the embedder and we can't do this for data symbols (at least not without
+  // compiling with -fPIC)
+  if (isa<DataSymbol>(Sym))
+    return false;
+  return (Config->AllowUndefined ||
+          Config->AllowUndefinedSymbols.count(Sym->getName()) != 0);
+}
+
+static void reportUndefined(const Symbol* Sym) {
+  assert(Sym->isUndefined());
+  assert(!Sym->isWeak());
+  if (!allowUndefined(Sym))
+    error(toString(Sym->getFile()) + ": undefined symbol: " + toString(*Sym));
+}
+
 void lld::wasm::scanRelocations(InputChunk *Chunk) {
   if (!Chunk->Live)
     return;
   ObjFile *File = Chunk->File;
   ArrayRef<WasmSignature> Types = File->getWasmObj()->types();
   for (const WasmRelocation &Reloc : Chunk->getRelocations()) {
     if (Reloc.Type == R_WASM_TYPE_INDEX_LEB) {
       // Mark target type as live
case R_WASM_TABLE_INDEX_REL_SLEB:
       if (requiresGOTAccess(Sym))
         break;
       Out.ElemSec->addEntry(cast<FunctionSymbol>(Sym));
       break;
     case R_WASM_GLOBAL_INDEX_LEB:
       if (!isa<GlobalSymbol>(Sym))
         Out.ImportSec->addGOTEntry(Sym);
       break;
-    case R_WASM_MEMORY_ADDR_SLEB:
-    case R_WASM_MEMORY_ADDR_LEB:
-    case R_WASM_MEMORY_ADDR_REL_SLEB:
-      if (!Config->Relocatable && Sym->isUndefined() && !Sym->isWeak()) {
-        error(toString(File) + ": cannot resolve relocation of type " +
-              relocTypeToString(Reloc.Type) +
-              " against undefined (non-weak) data symbol: " + toString(*Sym));
-      }
-      break;
     }
 
     if (Config->Pic) {
       switch (Reloc.Type) {
       case R_WASM_TABLE_INDEX_SLEB:
       case R_WASM_MEMORY_ADDR_SLEB:
       case R_WASM_MEMORY_ADDR_LEB:
         // Certain relocation types can't be used when building PIC output,
         // since they would require absolute symbol addresses at link time.
         error(toString(File) + ": relocation " + relocTypeToString(Reloc.Type) +
               " cannot be used against symbol " + toString(*Sym) +
               "; recompile with -fPIC");
         break;
       case R_WASM_TABLE_INDEX_I32:
       case R_WASM_MEMORY_ADDR_I32:
         // These relocation types are only present in the data section and
         // will be converted into code by `generateRelocationCode`.  This code
         // requires the symbols to have GOT entires.
         if (requiresGOTAccess(Sym))
           Out.ImportSec->addGOTEntry(Sym);
         break;
       }
+    } else {
+      // Report undefined symbols
+      if (Sym->isUndefined() && !Config->Relocatable && !Sym->isWeak())
+        reportUndefined(Sym);
     }
+
   }
 }

lld/trunk/wasm/SymbolTable.h

 class SymbolTable {
 public:
   void wrap(Symbol *Sym, Symbol *Real, Symbol *Wrap);
 
   void addFile(InputFile *File);
 
   void addCombinedLTOObject();
 
-  void reportRemainingUndefines();
-
   ArrayRef<Symbol *> getSymbols() const { return SymVector; }
 
   Symbol *find(StringRef Name);
 
   void replace(StringRef Name, Symbol* Sym);
 
   void trace(StringRef Name);
 

lld/trunk/wasm/SymbolTable.cpp

void SymbolTable::addCombinedLTOObject() {
 
   for (StringRef Filename : LTO->compile()) {
     auto *Obj = make<ObjFile>(MemoryBufferRef(Filename, "lto.tmp"), "");
     Obj->parse(true);
     ObjectFiles.push_back(Obj);
   }
 }
 
-void SymbolTable::reportRemainingUndefines() {
-  for (const auto& Pair : SymMap) {
-    const Symbol *Sym = SymVector[Pair.second];
-    if (!Sym->isUndefined() || Sym->isWeak())
-      continue;
-    if (Config->AllowUndefinedSymbols.count(Sym->getName()) != 0)
-      continue;
-    if (!Sym->IsUsedInRegularObj)
-      continue;
-    error(toString(Sym->getFile()) + ": undefined symbol: " + toString(*Sym));
-  }
-}
-
 Symbol *SymbolTable::find(StringRef Name) {
   auto It = SymMap.find(CachedHashStringRef(Name));
   if (It == SymMap.end() || It->second == -1)
     return nullptr;
   return SymVector[It->second];
 }
 
 void SymbolTable::replace(StringRef Name, Symbol* Sym) {

lld/trunk/wasm/Writer.cpp

void Writer::run() {
 
   if (!Config->Relocatable) {
     // Create linker synthesized functions
     if (Config->PassiveSegments)
       createInitMemoryFunction();
     if (Config->Pic)
       createApplyRelocationsFunction();
     createCallCtorsFunction();
-
-    // Make sure we have resolved all symbols.
-    if (!Config->AllowUndefined)
-      Symtab->reportRemainingUndefines();
-
-    if (errorCount())
-      return;
   }
 
   log("-- calculateTypes");
   calculateTypes();
   log("-- calculateExports");
   calculateExports();
   log("-- calculateCustomSections");
   calculateCustomSections();