[WebAssembly] Error on relocations against undefined data symbols.

[sbc100]

Summary

We can't meaningfully resolve certain types of relocations against
undefined data symbols. Previously when --allow-undefined was used
we were treating such relocation much like weak data symbols and
simply inserting zeros. This change turns such use cases in to an
error.

This means that --allow-undefined is no longer effective for data
symbols.

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

[sbc100]

fix debug info tests

[sbc100]

remove debugging

[kripken]

As mentioned on the issue, I'd prefer if we got imports for such globals, but if that's not possible then definitely an error instead of just silently emitting a 0 is better, lgtm.

[sbc100]

In D60882#1472279, @kripken wrote:

As mentioned on the issue, I'd prefer if we got imports for such globals, but if that's not possible then definitely an error instead of just silently emitting a 0 is better, lgtm.

We can get imports I think, but it will have to be a codegen option I think, the linker can't convert them. So I see it as a separate compiler issue.

[ruiu]

Although I understand that reporting an error is sometimes useful, are you sure that that's the behavior that users want? I wonder if we would end up adding --really-allow-undefined in the future to override this new behavior.

lld/wasm/InputFiles.cpp

144 ↗

(On Diff #195812)

Can you add a comment for this code block?

[sbc100]

In D60882#1472591, @ruiu wrote:

Although I understand that reporting an error is sometimes useful, are you sure that that's the behavior that users want? I wonder if we would end up adding --really-allow-undefined in the future to override this new behavior.

The problem is that this would actually mean "treat all data symbols as weak undefined with no way to provide them". Which I guess somebody might want but its hard to imagine. the current users of --allow-undefined kind of expect to be able to provide the undefined symbols at runtime.

In reality what we will probably end up doing is linker relaxation to allow "i32.load <foo>" to re-written to "global.get <foo>", although I'm not sure when we will end up going there.

[ruiu]

LGTM

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 --shared -o %t.wasm %t.o 2>&1 | FileCheck %s -check-prefix=BADRELOC
+; RUN: not wasm-ld --allow-undefined -o %t.wasm %t.o 2>&1 | FileCheck %s -check-prefix=ALLOW
+; 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: undefined symbol: data_external
-; BADRELOC: undefined-data.ll.tmp.o: relocation R_WASM_MEMORY_ADDR_LEB cannot be used againt symbol data_external; recompile with -fPIC
+; ALLOW: undefined-data.ll.tmp.o: cannot resolve relocation of type R_WASM_MEMORY_ADDR_LEB against undefined (non-weak) data symbol: data_external
+; SHARED: undefined-data.ll.tmp.o: relocation R_WASM_MEMORY_ADDR_LEB cannot be used againt symbol data_external; recompile with -fPIC

wasm/InputFiles.cpp

uint32_t ObjFile::calcExpectedValue(const WasmRelocation &Reloc) const {
   }
   default:
     llvm_unreachable("unknown relocation type");
   }
 }
 
 // Translate from the relocation's index into the final linked output value.
 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;
+      if (Config->Shared && 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:
     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 (auto *Sym = dyn_cast<DefinedData>(getDataSymbol(Reloc.Index)))
-      if (Sym->isLive())
-        return Sym->getVirtualAddress() + Reloc.Addend;
-    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: {
-    const Symbol* Sym = Symbols[Reloc.Index];
+  case R_WASM_GLOBAL_INDEX_LEB:
     if (auto GS = dyn_cast<GlobalSymbol>(Sym))
       return GS->getGlobalIndex();
     return Sym->getGOTIndex();
-  } case R_WASM_EVENT_INDEX_LEB:
+  case R_WASM_EVENT_INDEX_LEB:
     return getEventSymbol(Reloc.Index)->getEventIndex();
-  case R_WASM_FUNCTION_OFFSET_I32:
-    if (auto *Sym = dyn_cast<DefinedFunction>(getFunctionSymbol(Reloc.Index))) {
-      if (Sym->isLive())
-        return Sym->Function->OutputOffset +
-               Sym->Function->getFunctionCodeOffset() + Reloc.Addend;
-    }
-    return 0;
+  case R_WASM_FUNCTION_OFFSET_I32: {
+    auto *F = cast<DefinedFunction>(Sym);
+    return F->Function->OutputOffset + F->Function->getFunctionCodeOffset() +
+           Reloc.Addend;
   case R_WASM_SECTION_OFFSET_I32:
     return getSectionSymbol(Reloc.Index)->Section->OutputOffset + Reloc.Addend;
   default:
     llvm_unreachable("unknown relocation type");
   }
 }
 
 template <class T>

wasm/Writer.cpp

case R_WASM_TYPE_INDEX_LEB:
       File->TypeIsUsed[Reloc.Index] = true;
       break;
     case R_WASM_GLOBAL_INDEX_LEB: {
       auto* Sym = File->getSymbols()[Reloc.Index];
       if (!isa<GlobalSymbol>(Sym) && !Sym->isInGOT()) {
         Sym->setGOTIndex(NumImportedGlobals++);
         GOTSymbols.push_back(Sym);
       }
+      break;
+    }
+    case R_WASM_MEMORY_ADDR_SLEB:
+    case R_WASM_MEMORY_ADDR_LEB:
+    case R_WASM_MEMORY_ADDR_REL_SLEB: {
+      if (!Config->Relocatable) {
+        auto* Sym = File->getSymbols()[Reloc.Index];
+        if (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: {