[lld][WebAssembly] Perform data relocations during start function

[sbc100]

Summary

We already perform memory initialization and apply global relocations
during start. It makes sense to performs data relocations too. I think
the reason we were not doing this already is solely historical.

[sbc100]

• update

[sbc100]

• update

[sbc100]

• rebase

[tlively]

Who was previously responsible for applying the data relocs? If it was Emscripten runtime code, does that mean this change will have to land alongside the corresponding Emscripten change? Is there anyone who would be broken by this ABI break who we should notify?

lld/wasm/Writer.cpp

1267–1274

Can we add a test that exercises this case?

[sbc100]

In D117412#3250155, @tlively wrote:

Who was previously responsible for applying the data relocs? If it was Emscripten runtime code, does that mean this change will have to land alongside the corresponding Emscripten change? Is there anyone who would be broken by this ABI break who we should notify?

Previously it was treated as the very first ctor in __wasm_call_ctors.. (which emscripten take care of calling). Now it happens earlier, removing that window where the relocations have not yet been applied.

[sbc100]

• add testing

[sbc100]

• add tests

[sbc100]

lld/wasm/Writer.cpp

1267–1274

I think we have coverage of all three branches above now.

[dschuff]

This change (probably) broke the SAFE_HEAP tests on the emscripten waterfall: https://logs.chromium.org/logs/emscripten-releases/buildbucket/cr-buildbucket/8824622360264175889/+/u/Emscripten_testsuite__SAFE_HEAP_/stdout
There are other changes in the LLVM roll (https://chromium.googlesource.com/emscripten-releases/+/cd22f208c7dda41d695549e17f8ddc4ae533769d) but this is the only wasm-related one.

[sbc100]

In D117412#3256093, @dschuff wrote:

This change (probably) broke the SAFE_HEAP tests on the emscripten waterfall: https://logs.chromium.org/logs/emscripten-releases/buildbucket/cr-buildbucket/8824622360264175889/+/u/Emscripten_testsuite__SAFE_HEAP_/stdout
There are other changes in the LLVM roll (https://chromium.googlesource.com/emscripten-releases/+/cd22f208c7dda41d695549e17f8ddc4ae533769d) but this is the only wasm-related one.

Yup.. I noticed that too. Fix is in https://github.com/WebAssembly/binaryen/pull/4463

lld/test/wasm/data-segments.ll

 ;      PASSIVE-PIC:  - Type:            START
 ; PASSIVE-PIC-NEXT:    StartFunction:   2
 ; PASSIVE-PIC-NEXT:  - Type:            DATACOUNT
 ; PASSIVE-PIC-NEXT:    Count:           2
 ; PASSIVE-PIC-NEXT:  - Type:            CODE
 ; PASSIVE-PIC-NEXT:    Functions:
 ; PASSIVE-PIC-NEXT:      - Index:           0
 ; PASSIVE-PIC-NEXT:        Locals:          []
-; PASSIVE-PIC-NEXT:        Body:            10030B
+; PASSIVE-PIC-NEXT:        Body:            0B
 ; PASSIVE-PIC-NEXT:      - Index:           1
 ; PASSIVE-PIC-NEXT:        Locals:          []
 ; PASSIVE-PIC-NEXT:        Body:            0B
 ; PASSIVE-PIC-NEXT:      - Index:           2
 ; PASSIVE-PIC-NEXT:        Locals:
 ; PASSIVE32-PIC-NEXT:          - Type:            I32
 ; PASSIVE64-PIC-NEXT:          - Type:            I64
 ; PASSIVE-PIC-NEXT:              Count:           1

 ; NOPIC-DIS-NEXT:      [[PTR]].const   1060
 ; PIC-DIS-NEXT:        [[PTR]].const   36
 ; PIC-DIS-NEXT:        global.get      1
 ; PIC-DIS-NEXT:        [[PTR]].add
 ; DIS-NEXT:            i32.const       0
 ; DIS-NEXT:            i32.const       10000
 ; DIS-NEXT:            memory.fill     0
 
+; PIC-DIS-NEXT:        call 3
+
 ; NOPIC-DIS-NEXT:      [[PTR]].const   11060
 ; PIC-DIS-NEXT:        local.get       0
 
 ; DIS-NEXT:            i32.const       2
 ; DIS-NEXT:            i32.atomic.store        0
 
 ; NOPIC-DIS-NEXT:      [[PTR]].const   11060
 ; PIC-DIS-NEXT:        local.get       0

lld/test/wasm/pie.ll

 ; RUN: llc -relocation-model=pic -mattr=+mutable-globals -filetype=obj %s -o %t.o
 ; RUN: wasm-ld --no-gc-sections --experimental-pic -pie -o %t.wasm %t.o
 ; RUN: obj2yaml %t.wasm | FileCheck %s
+; RUN: llvm-objdump --disassemble-symbols=__wasm_start --no-show-raw-insn --no-leading-addr %t.wasm | FileCheck %s --check-prefixes DISASSEM
 
 target triple = "wasm32-unknown-emscripten"
 
 @data = global i32 2, align 4
 @data_external = external global i32
 @indirect_func = local_unnamed_addr global i32 ()* @foo, align 4
 
 @data_addr = local_unnamed_addr global i32* @data, align 4

 ; CHECK-NEXT:         GlobalMutable:   false
 ; CHECK-NEXT:       - Module:          env
 ; CHECK-NEXT:         Field:           __table_base
 ; CHECK-NEXT:         Kind:            GLOBAL
 ; CHECK-NEXT:         GlobalType:      I32
 ; CHECK-NEXT:         GlobalMutable:   false
 
 ; CHECK:        - Type:            START
-; CHECK-NEXT:     StartFunction:   3
+; CHECK-NEXT:     StartFunction:   4
 
 ; CHECK:        - Type:            CUSTOM
 ; CHECK-NEXT:     Name:            name
 ; CHECK-NEXT:     FunctionNames:
 ; CHECK-NEXT:       - Index:           0
 ; CHECK-NEXT:         Name:            external_func
 ; CHECK-NEXT:       - Index:           1
 ; CHECK-NEXT:         Name:            __wasm_call_ctors
 ; CHECK-NEXT:       - Index:           2
 ; CHECK-NEXT:         Name:            __wasm_apply_data_relocs
 ; CHECK-NEXT:       - Index:           3
 ; CHECK-NEXT:         Name:            __wasm_apply_global_relocs
+; CHECK-NEXT:       - Index:           4
+; CHECK-NEXT:         Name:            __wasm_start
 
+; DISASSEM:       <__wasm_start>:
+; DISASSEM-EMPTY:
+; DISASSEM-NEXT:   call 2
+; DISASSEM-NEXT:   call 3
+; DISASSEM-NEXT:   end
 
 ; Run the same test with threading support.  In this mode
 ; we expect __wasm_init_memory and __wasm_apply_data_relocs
 ; to be generated along with __wasm_start as the start
 ; function.
 
 ; RUN: llc -relocation-model=pic -mattr=+mutable-globals,+atomics,+bulk-memory -filetype=obj %s -o %t.shmem.o
 ; RUN: wasm-ld --no-gc-sections --shared-memory --allow-undefined --experimental-pic -pie -o %t.shmem.wasm %t.shmem.o
 ; RUN: obj2yaml %t.shmem.wasm | FileCheck %s --check-prefix=SHMEM
+; RUN: llvm-objdump --disassemble-symbols=__wasm_start --no-show-raw-insn --no-leading-addr %t.shmem.wasm | FileCheck %s --check-prefixes DISASSEM-SHMEM
 
-; SHMEM:         - Type:            CODE
-; SHMEM:           - Index:           6
-; SHMEM-NEXT:        Locals:          []
-; SHMEM-NEXT:        Body:            100310050B
+; SHMEM:         - Type:            START
+; SHMEM-NEXT:      StartFunction:   6
+
+; DISASSEM-SHMEM:       <__wasm_start>:
+; DISASSEM-SHMEM-EMPTY:
+; DISASSEM-SHMEM-NEXT:   call 3
+; DISASSEM-SHMEM-NEXT:   call 5
+; DISASSEM-SHMEM-NEXT:   end
 
 ; SHMEM:         FunctionNames:
 ; SHMEM-NEXT:      - Index:           0
 ; SHMEM-NEXT:        Name:            external_func
 ; SHMEM-NEXT:      - Index:           1
 ; SHMEM-NEXT:        Name:            __wasm_call_ctors
 ; SHMEM-NEXT:      - Index:           2
 ; SHMEM-NEXT:        Name:            __wasm_init_tls

lld/wasm/Writer.cpp

if (config->isPic) {
     if (out.globalSec->needsRelocations()) {
       WasmSym::applyGlobalRelocs = symtab->addSyntheticFunction(
           "__wasm_apply_global_relocs", WASM_SYMBOL_VISIBILITY_HIDDEN,
           make<SyntheticFunction>(nullSignature, "__wasm_apply_global_relocs"));
       WasmSym::applyGlobalRelocs->markLive();
     }
   }
 
-  if (WasmSym::applyGlobalRelocs && WasmSym::initMemory) {
+  int startCount = 0;
+  if (WasmSym::applyGlobalRelocs)
+    startCount++;
+  if (WasmSym::WasmSym::initMemory || WasmSym::applyDataRelocs)
+    startCount++;
+
+  // If there is only one start function we can just use that function
+  // itself as the Wasm start function, otherwise we need to synthesize
+  // a new function to call them in sequence.
+  if (startCount > 1) {
     WasmSym::startFunction = symtab->addSyntheticFunction(
         "__wasm_start", WASM_SYMBOL_VISIBILITY_HIDDEN,
         make<SyntheticFunction>(nullSignature, "__wasm_start"));
     WasmSym::startFunction->markLive();
   }
 }
 
 void Writer::createInitMemoryFunction() {
for (const OutputSegment *s : segments) {
           writeU8(os, WASM_OPCODE_MISC_PREFIX, "bulk-memory prefix");
           writeUleb128(os, WASM_OPCODE_MEMORY_INIT, "memory.init");
           writeUleb128(os, s->index, "segment index immediate");
           writeU8(os, 0, "memory index immediate");
         }
       }
     }
 
+    // Memory init is now complete.  Apply data relocation if there
+    // are any.
+    if (WasmSym::applyDataRelocs) {
+      writeU8(os, WASM_OPCODE_CALL, "CALL");
+      writeUleb128(os, WasmSym::applyDataRelocs->getFunctionIndex(),
+                   "function index");
+    }
+
     if (config->sharedMemory) {
       // Set flag to 2 to mark end of initialization
       writeGetFlagAddress();
       writeI32Const(os, 2, "flag value");
       writeU8(os, WASM_OPCODE_ATOMICS_PREFIX, "atomics prefix");
       writeUleb128(os, WASM_OPCODE_I32_ATOMIC_STORE, "i32.atomic.store");
       writeMemArg(os, 2, 0);
 
std::string bodyContent;
     // End the function
     writeU8(os, WASM_OPCODE_END, "END");
   }
 
   createFunction(WasmSym::initMemory, bodyContent);
 }
 
 void Writer::createStartFunction() {
+  // If the start function exists when we have more than one function to call.
   if (WasmSym::startFunction) {
     std::string bodyContent;
     {
       raw_string_ostream os(bodyContent);
       writeUleb128(os, 0, "num locals");
+      if (WasmSym::initMemory) {
         writeU8(os, WASM_OPCODE_CALL, "CALL");
         writeUleb128(os, WasmSym::initMemory->getFunctionIndex(),
                      "function index");
+      } else if (WasmSym::applyDataRelocs) {
+        // When initMemory is present it calls applyDataRelocs.  If not,
+        // we must call it directly.
+        writeU8(os, WASM_OPCODE_CALL, "CALL");
+        writeUleb128(os, WasmSym::applyDataRelocs->getFunctionIndex(),
+                     "function index");
+      }
+      if (WasmSym::applyGlobalRelocs) {
         writeU8(os, WASM_OPCODE_CALL, "CALL");
         writeUleb128(os, WasmSym::applyGlobalRelocs->getFunctionIndex(),
                      "function index");
+      }
       writeU8(os, WASM_OPCODE_END, "END");
     }
     createFunction(WasmSym::startFunction, bodyContent);
   } else if (WasmSym::initMemory) {
     WasmSym::startFunction = WasmSym::initMemory;
   } else if (WasmSym::applyGlobalRelocs) {
     WasmSym::startFunction = WasmSym::applyGlobalRelocs;
+  } else if (WasmSym::applyDataRelocs) {
+    WasmSym::startFunction = WasmSym::applyDataRelocs;
   }
 }
 
 // For -shared (PIC) output, we create create a synthetic function which will
 // apply any relocations to the data segments on startup.  This function is
 // called `__wasm_apply_data_relocs` and is added at the beginning of
 // `__wasm_call_ctors` before any of the constructors run.
 void Writer::createApplyDataRelocationsFunction() {

 }
 
 // Create synthetic "__wasm_call_ctors" function based on ctor functions
 // in input object.
 void Writer::createCallCtorsFunction() {
   // If __wasm_call_ctors isn't referenced, there aren't any ctors, and we
   // aren't calling `__wasm_apply_data_relocs` for Emscripten-style PIC, don't
   // define the `__wasm_call_ctors` function.
-  if (!WasmSym::callCtors->isLive() && !WasmSym::applyDataRelocs &&
-      initFunctions.empty())
+  if (!WasmSym::callCtors->isLive() && initFunctions.empty())
     return;
 
   // First write the body's contents to a string.
   std::string bodyContent;
   {
     raw_string_ostream os(bodyContent);
     writeUleb128(os, 0, "num locals");
 
-    if (WasmSym::applyDataRelocs) {
+    if (WasmSym::applyDataRelocs && !WasmSym::initMemory) {
       writeU8(os, WASM_OPCODE_CALL, "CALL");
       writeUleb128(os, WasmSym::applyDataRelocs->getFunctionIndex(),
                    "function index");
     }
 
     // Call constructors
     for (const WasmInitEntry &f : initFunctions) {
       writeU8(os, WASM_OPCODE_CALL, "CALL");