[lld][WebAssembly] Ensure stub symbols always get address 0

[sbc100]

Summary

Without this extra flag we can't distingish between stub functions and
functions that happen to have address 0 (relative to __table_base).

Adding this flag bit the base symbol class actually avoids growing the
SymbolUnion struct which would not be true if we added it to the
FunctionSymbol subclass (due to bitbacking).

The previous approach of setting it's table index to zero worked for
normal static relocations but not for -fPIC code.

See https://github.com/emscripten-core/emscripten/issues/12819

[sbc100]

Update test

[sbc100]

• more

[sbc100]

• more

[sbc100]

Revert part

[sbc100]

• add comment

[kripken]

I don't know enough to tell if these changes are sufficient, but they make sense to me.

lld/test/wasm/weak-undefined-pic.s

48

lld/wasm/SymbolTable.cpp

676–678

lld/wasm/SyntheticSections.cpp

335

386

[sbc100]

feedback

[dschuff]

lld/test/wasm/weak-undefined-pic.s

31

72

lld/wasm/Symbols.h

162

If getting slot 0 (and comparing equal to the nullptr) is also a requirement for stub functions then that should maybe be mentioned here too.

lld/test/wasm/weak-undefined-pic.s

+# Checks handling of undefined weak external functions.  When the
+# static linker decides they are undefined, check GOT relocations
+# resolve to zero (i.e. a global that contains zero.).
+#
+# RUN: llvm-mc -filetype=obj -triple=wasm32-unknown-unknown -o %t.o %s
+# RUN: wasm-ld %t.o -o %t1.wasm
+# RUN: obj2yaml %t1.wasm | FileCheck %s
+#
+# With `--unresolved-symbols=ignore-all` the behaviour should be the same
+# as the default.>
+#
+# RUN: wasm-ld --unresolved-symbols=ignore-all %t.o -o %t2.wasm
+# RUN: obj2yaml %t2.wasm | FileCheck %s
+
+.globl get_foo_addr
+get_foo_addr:
+  .functype get_foo_addr () -> (i32)
+  global.get foo@GOT
+  end_function
+
+.globl _start
+_start:
+  .functype _start () -> ()
+  call get_foo_addr
+  end_function
+
+.weak foo
+.functype foo () -> (i32)
+
+# Verify that we do not generate dynamnic relocations for the GOT entry.
+
+# CHECK-NOT: __wasm_apply_relocs
+
+# Verify that we do not generate an import for foo
+
+# CHECK-NOT:  - Type:            IMPORT
+
+#      CHECK:   - Type:            GLOBAL
+# CHECK-NEXT:     Globals:
+# CHECK-NEXT:       - Index:           0
+# CHECK-NEXT:         Type:            I32
+# CHECK-NEXT:         Mutable:         true
+# CHECK-NEXT:         InitExpr:
+# CHECK-NEXT:           Opcode:          I32_CONST
+# CHECK-NEXT:           Value:           66560
+# Global 'undefined_weak:foo' representing the GOT entry for goo
+# Unlike other internal GOT entries that need to mutable this one
+# is immutable and not updated by `__wasm_apply_relocs`
+# CHECK-NEXT:       - Index:           1
+# CHECK-NEXT:         Type:            I32
+# CHECK-NEXT:         Mutable:         false
+# CHECK-NEXT:         InitExpr:
+# CHECK-NEXT:           Opcode:          I32_CONST
+# CHECK-NEXT:           Value:           0
+
+#      CHECK:  - Type:            CUSTOM
+# CHECK-NEXT:    Name:            name
+# CHECK-NEXT:    FunctionNames:
+# CHECK-NEXT:      - Index:           0
+# CHECK-NEXT:        Name:            'undefined_weak:foo'
+# CHECK-NEXT:      - Index:           1
+# CHECK-NEXT:        Name:            get_foo_addr
+# CHECK-NEXT:      - Index:           2
+# CHECK-NEXT:        Name:            _start
+# CHECK-NEXT:    GlobalNames:
+# CHECK-NEXT:      - Index:           0
+# CHECK-NEXT:        Name:            __stack_pointer
+# CHECK-NEXT:      - Index:           1
+# CHECK-NEXT:        Name:            'undefined_weak:foo'
+
+# With `--unresolved-symbols=import-fuctions` the resolution should 
+# is defered to the dynamic linker and the global should be imported.
+#
+# RUN: wasm-ld --unresolved-symbols=import-functions %t.o -o %t3.wasm
+# RUN: obj2yaml %t3.wasm | FileCheck %s --check-prefix=IMPORT
+
+#      IMPORT:  - Type:            IMPORT
+# IMPORT-NEXT:    Imports:
+# IMPORT-NEXT:      - Module:          env
+# IMPORT-NEXT:        Field:           foo
+# IMPORT-NEXT:        Kind:            FUNCTION
+# IMPORT-NEXT:        SigIndex:        0
+
+#      IMPORT:  - Type:            GLOBAL
+# IMPORT-NEXT:    Globals:
+# IMPORT-NEXT:      - Index:           0
+# IMPORT-NEXT:        Type:            I32
+# IMPORT-NEXT:        Mutable:         true
+# IMPORT-NEXT:        InitExpr:
+# IMPORT-NEXT:          Opcode:          I32_CONST
+# IMPORT-NEXT:          Value:           66560
+# IMPORT-NEXT:      - Index:           1
+# IMPORT-NEXT:        Type:            I32
+# IMPORT-NEXT:        Mutable:         false
+# IMPORT-NEXT:        InitExpr:
+# IMPORT-NEXT:          Opcode:          I32_CONST
+# IMPORT-NEXT:          Value:           1
+
+#      IMPORT:    GlobalNames:
+# IMPORT-NEXT:      - Index:           0
+# IMPORT-NEXT:        Name:            __stack_pointer
+# IMPORT-NEXT:      - Index:           1
+# IMPORT-NEXT:        Name:            foo

lld/wasm/Driver.cpp

if (sym && sym->isDefined())
       sym->forceExport = true;
     else if (config->unresolvedSymbols == UnresolvedPolicy::ReportError)
       error(Twine("symbol exported via --export not found: ") +
             arg->getValue());
     else if (config->unresolvedSymbols == UnresolvedPolicy::Warn)
       warn(Twine("symbol exported via --export not found: ") + arg->getValue());
   }
 
-  if (!config->relocatable) {
+  if (!config->relocatable &&
+      config->unresolvedSymbols != UnresolvedPolicy::ImportFuncs) {
     // Add synthetic dummies for weak undefined functions.  Must happen
     // after LTO otherwise functions may not yet have signatures.
     symtab->handleWeakUndefines();
   }
 
   if (entrySym)
     entrySym->setHidden(false);
 

lld/wasm/MarkLive.cpp

for (const WasmRelocation reloc : c->getRelocations()) {
       // zero is only reachable via "call", not via "call_indirect".  The stub
       // functions used for weak-undefined symbols have this behaviour (compare
       // equal to null pointer, only reachable via direct call).
       if (reloc.Type == R_WASM_TABLE_INDEX_SLEB ||
           reloc.Type == R_WASM_TABLE_INDEX_SLEB64 ||
           reloc.Type == R_WASM_TABLE_INDEX_I32 ||
           reloc.Type == R_WASM_TABLE_INDEX_I64) {
         auto *funcSym = cast<FunctionSymbol>(sym);
-        if (funcSym->hasTableIndex() && funcSym->getTableIndex() == 0)
+        if (funcSym->isStub)
           continue;
       }
 
       enqueue(sym);
     }
   }
 }
 

lld/wasm/Relocations.cpp

case UnresolvedPolicy::Warn:
       break;
     case UnresolvedPolicy::Ignore:
       if (auto *f = dyn_cast<UndefinedFunction>(sym)) {
         if (!f->stubFunction) {
           LLVM_DEBUG(dbgs()
                      << "ignoring undefined symbol: " + toString(*sym) + "\n");
           f->stubFunction = symtab->createUndefinedStub(*f->getSignature());
           f->stubFunction->markLive();
-          f->setTableIndex(0);
+          // Mark the function itself as a stub which prevents it from being
+          // assigned a table entry.
+          f->isStub = true;
         }
       }
       break;
     case UnresolvedPolicy::ImportFuncs:
       break;
     }
   }
 }

lld/wasm/SymbolTable.cpp

InputFunction *SymbolTable::replaceWithUnreachable(Symbol *sym,
                                                    StringRef debugName) {
   auto *func = make<SyntheticFunction>(sig, sym->getName(), debugName);
   func->setBody(unreachableFn);
   syntheticFunctions.emplace_back(func);
   // Mark new symbols as local. For relocatable output we don't want them
   // to be exported outside the object file.
   replaceSymbol<DefinedFunction>(sym, debugName, WASM_SYMBOL_BINDING_LOCAL,
                                  nullptr, func);
-  // 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);
+  // Ensure it the stub function doesn't get a table entry.  Its address
+  // should alwasys compare equal to the null pointer.
+  sym->isStub = true;
   return func;
 }
 
 void SymbolTable::replaceWithUndefined(Symbol *sym) {
   // 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_weak:" + toString(*sym));
   replaceWithUnreachable(sym, *sym->getSignature(), debugName);

lld/wasm/Symbols.h

public:
 
   void setGOTIndex(uint32_t index);
   bool hasGOTIndex() const { return gotIndex != INVALID_INDEX; }
 
 protected:
   Symbol(StringRef name, Kind k, uint32_t flags, InputFile *f)
       : name(name), file(f), symbolKind(k), referenced(!config->gcSections),
         requiresGOT(false), isUsedInRegularObj(false), forceExport(false),
-        canInline(false), traced(false), flags(flags) {}
+        canInline(false), traced(false), isStub(false), flags(flags) {}
 
   StringRef name;
   InputFile *file;
   uint32_t outputSymbolIndex = INVALID_INDEX;
   uint32_t gotIndex = INVALID_INDEX;
   Kind symbolKind;
 
 public:
public:
   // False if LTO shouldn't inline whatever this symbol points to. If a symbol
   // is overwritten after LTO, LTO shouldn't inline the symbol because it
   // doesn't know the final contents of the symbol.
   bool canInline : 1;
 
   // True if this symbol is specified by --trace-symbol option.
   bool traced : 1;
 
+  // True if this symbol is a linker-synthesized stub function (traps when
+  // called) and should otherwise be treated as missing/undefined.  See
+  // SymbolTable::replaceWithUndefined.
+  bool isStub : 1;
+
   uint32_t flags;
 };
 
 class FunctionSymbol : public Symbol {
 public:
   static bool classof(const Symbol *s) {
     return s->kind() == DefinedFunctionKind ||
            s->kind() == UndefinedFunctionKind;

lld/wasm/SyntheticSections.cpp

if (auto *d = dyn_cast<DefinedData>(sym)) {
       // Get __memory_base
       writeU8(os, WASM_OPCODE_GLOBAL_GET, "GLOBAL_GET");
       writeUleb128(os, WasmSym::memoryBase->getGlobalIndex(), "__memory_base");
 
       // Add the virtual address of the data symbol
       writeU8(os, opcode_ptr_const, "CONST");
       writeSleb128(os, d->getVirtualAddress(), "offset");
     } else if (auto *f = dyn_cast<FunctionSymbol>(sym)) {
+      if (f->isStub)
+        continue;
       // Get __table_base
       writeU8(os, WASM_OPCODE_GLOBAL_GET, "GLOBAL_GET");
       writeUleb128(os, WasmSym::tableBase->getGlobalIndex(), "__table_base");
 
       // Add the table index to __table_base
       writeU8(os, opcode_ptr_const, "CONST");
       writeSleb128(os, f->getTableIndex(), "offset");
     } else {
void GlobalSection::writeBody() {
   raw_ostream &os = bodyOutputStream;
 
   writeUleb128(os, numGlobals(), "global count");
   for (InputGlobal *g : inputGlobals)
     writeGlobal(os, g->global);
   // TODO(wvo): when do these need I64_CONST?
   for (const Symbol *sym : internalGotSymbols) {
     WasmGlobal global;
-    global.Type = {WASM_TYPE_I32, config->isPic};
+    // In the case of dynamic linking, internal GOT entries
+    // need to be mutable since the get updated to the correct
+    // runtime value during `__wasm_apply_relocs`.
+    bool mutable_ = config->isPic & !sym->isStub;
+    global.Type = {WASM_TYPE_I32, mutable_};
     global.InitExpr.Opcode = WASM_OPCODE_I32_CONST;
     if (auto *d = dyn_cast<DefinedData>(sym))
       global.InitExpr.Value.Int32 = d->getVirtualAddress();
     else if (auto *f = dyn_cast<FunctionSymbol>(sym))
-      global.InitExpr.Value.Int32 = f->getTableIndex();
+      global.InitExpr.Value.Int32 = f->isStub ? 0 : f->getTableIndex();
     else {
       assert(isa<UndefinedData>(sym));
       global.InitExpr.Value.Int32 = 0;
     }
     writeGlobal(os, global);
   }
   for (const DefinedData *sym : dataAddressGlobals) {
     WasmGlobal global;

 }
 
 void StartSection::writeBody() {
   raw_ostream &os = bodyOutputStream;
   writeUleb128(os, WasmSym::initMemory->getFunctionIndex(), "function index");
 }
 
 void ElemSection::addEntry(FunctionSymbol *sym) {
-  if (sym->hasTableIndex())
+  if (sym->hasTableIndex() || sym->isStub)
     return;
   sym->setTableIndex(config->tableBase + indirectFunctions.size());
   indirectFunctions.emplace_back(sym);
 }
 
 void ElemSection::writeBody() {
   raw_ostream &os = bodyOutputStream;
 
   writeUleb128(os, 1, "segment count");
   writeUleb128(os, 0, "table index");

lld/wasm/Writer.cpp

static bool shouldImport(Symbol *sym) {
 
   return false;
 }
 
 void Writer::calculateImports() {
   for (Symbol *sym : symtab->getSymbols()) {
     if (!sym->isUndefined())
       continue;
-    if (sym->isWeak() && !config->relocatable)
-      continue;
     if (!sym->isLive())
       continue;
     if (!sym->isUsedInRegularObj)
       continue;
     if (shouldImport(sym)) {
       LLVM_DEBUG(dbgs() << "import: " << sym->getName() << "\n");
       out.importSec->addImport(sym);
     }