[PATCH] D81704: [WebAssembly] Adding 64-bit version of R_WASM_MEMORY_ADDR_* relocs

[Sam Clegg via Phabricator via llvm-commits]

sbc100 added a comment.

I don't really grok the tablegen stuff but everything else looks pretty good to me!



================
Comment at: lld/wasm/Driver.cpp:502
   wasmGlobal.Type = {WASM_TYPE_I32, isMutable};
-  wasmGlobal.InitExpr.Value.Int32 = value;
+  wasmGlobal.InitExpr.Value.Int64 = value;
   wasmGlobal.InitExpr.Opcode = WASM_OPCODE_I32_CONST;
----------------
This seems odd.

I wonder if this function should at least take an `in32_t`?


================
Comment at: lld/wasm/InputChunks.cpp:89
         rel.Type != R_WASM_GLOBAL_INDEX_I32) {
-      uint32_t expectedValue = file->calcExpectedValue(rel);
+      auto expectedValue = file->calcExpectedValue(rel);
       if (expectedValue != existingValue)
----------------
I don't think I'm ready for these kind of autos just yet.  I'd rather see what type it is.


================
Comment at: lld/wasm/InputChunks.cpp:117
     uint8_t *loc = buf + rel.Offset + off;
-    uint32_t value = file->calcNewValue(rel);
+    auto value = file->calcNewValue(rel);
     LLVM_DEBUG(dbgs() << "apply reloc: type=" << relocTypeToString(rel.Type));
----------------
ditto


================
Comment at: lld/wasm/InputFiles.cpp:141
+  case R_WASM_MEMORY_ADDR_LEB64:
+>>>>>>> [WebAssembly] Adding 64-bit version of R_WASM_MEMORY_ADDR_* relocs
   case R_WASM_MEMORY_ADDR_SLEB:
----------------
oops?


================
Comment at: lld/wasm/InputFiles.cpp:429
+    auto offset = sym.Info.DataRef.Offset;
+    auto size = sym.Info.DataRef.Size;
     if (sym.isBindingLocal())
----------------
ditto


================
Comment at: llvm/include/llvm/BinaryFormat/Wasm.h:79
-    int32_t Int32;
     int64_t Int64;
     uint32_t Float32;
----------------
How about calling this something else?  Maybe ConstVal?  Or just Int?  

The fact that you have left all the other ones makes me less attracted to this part of the change.    What would it looks like to instead leave this member of the union and instead check all accesses to ensure they match the Opcode?


================
Comment at: llvm/lib/Object/RelocationResolver.cpp:498
   case wasm::R_WASM_MEMORY_ADDR_I32:
+  case wasm::R_WASM_MEMORY_ADDR_I64:
   case wasm::R_WASM_TYPE_INDEX_LEB:
----------------
But we don't support 64 bit relocs o wasm32 and vice versa, right?  I mean I don't think we should.


================
Comment at: llvm/lib/Object/WasmObjectFile.cpp:170
-    Expr.Value.Int32 = readVarint32(Ctx);
-    break;
   case wasm::WASM_OPCODE_I64_CONST:
----------------
Removing this seems wrong.. we don't to support wider const values with this opcode.   


================
Comment at: llvm/lib/Object/WasmObjectFile.cpp:568
+        auto Offset = readVaruint64(Ctx);
+        auto Size = readVaruint64(Ctx);
         if (Offset + Size > DataSegments[Index].Data.Content.size())
----------------
ditto re: auto


================
Comment at: llvm/lib/Object/WasmObjectFile.cpp:1254
       Segment.Data.Offset.Opcode = wasm::WASM_OPCODE_I32_CONST;
-      Segment.Data.Offset.Value.Int32 = 0;
+      Segment.Data.Offset.Value.Int64 = 0;
     }
----------------
Again it doesn't sit right to assign to the `Int64` member for `I32_CONST` opcode.


================
Comment at: llvm/lib/Object/WasmObjectFile.cpp:1352
     assert(Segment.Offset.Opcode == wasm::WASM_OPCODE_I32_CONST);
-    return Segment.Offset.Value.Int32 + Sym.Info.DataRef.Offset;
+    return Segment.Offset.Value.Int64 + Sym.Info.DataRef.Offset;
   }
----------------
ditto.


================
Comment at: llvm/lib/ObjectYAML/WasmYAML.cpp:423
   case wasm::WASM_OPCODE_I64_CONST:
     IO.mapRequired("Value", Expr.Value.Int64);
     break;
----------------
Maybe would make more sense if the union member had a different name.. but I'm leaning towards restoring the different members.


================
Comment at: llvm/lib/Target/WebAssembly/AsmParser/WebAssemblyAsmParser.cpp:854
       }
+      if (getSTI().getTargetTriple().getArch() == Triple::ArchType::wasm64) {
+        // Upgrade 32-bit loads/stores to 64-bit. These mostly differ by having
----------------
Does the triple have something like is64() that might make this shorter?  Or isWasm64?  


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D81704/new/

https://reviews.llvm.org/D81704