[PATCH] D41954: [WebAssembly] Refactor symbol index handling (LLVM)

[Sam Clegg via Phabricator via llvm-commits]

sbc100 added inline comments.


================
Comment at: include/llvm/BinaryFormat/Wasm.h:93
 struct WasmFunction {
-  uint32_t Index;
+  uint32_t WasmIndex; // index into code array + num imported fns
   std::vector<WasmLocalDecl> Locals;
----------------
How about just:

```
uint32_t Index;   // Function index space
```

There is really only one index that a function can have.

If we want to be extra clear we could also add new types for `FunctionIndex` and `GlobalIndex` I suppose.


================
Comment at: include/llvm/Object/Wasm.h:57
+  // index space consists of the imports followed by the exports (each of
+  // which creates a Symbol).
+  uint32_t SymbolIndex;
----------------
Its a single index space right?  How about just "Index into the symbol index space, which consists of imports and exports"


================
Comment at: include/llvm/Object/Wasm.h:165
+  uint32_t getNumExportedGlobals() const { return NumExportedGlobals; }
+  uint32_t getNumExportedFunctions() const { return NumExportedFunctions; }
 
----------------
Do we need these new methods?


================
Comment at: include/llvm/Object/Wasm.h:268
+  std::vector<WasmSymbol> FunctionSymbols;
+  std::vector<WasmSymbol> GlobalSymbols;
   std::vector<StringRef> Comdats;
----------------
Do these needs to be separate?  Why not just have "Symbols"?


================
Comment at: lib/MC/WasmObjectWriter.cpp:609
     return SymbolIndices[RelEntry.Symbol];
   case wasm::R_WEBASSEMBLY_TYPE_INDEX_LEB:
     if (!TypeIndices.count(RelEntry.Symbol))
----------------
I guess we can make this an if/else now since all relocations except TYPE_INDEX are now symbol-based.

It would be nice if we could unify TYPE_INDEX too but I don't think we can sadly since its used by call_indirect to call function pointers for which there is not a symbol.


================
Comment at: lib/MC/WasmObjectWriter.cpp:824
   write8(wasm::WASM_OPCODE_I32_CONST);
-  encodeSLEB128(0, getStream());
+  encodeSLEB128(InitialTableOffset, getStream());
   write8(wasm::WASM_OPCODE_END);
----------------
Why this change?


================
Comment at: lib/MC/WasmObjectWriter.cpp:947
     ArrayRef<WasmDataSegment> Segments, uint32_t DataSize,
-    ArrayRef<std::pair<StringRef, uint32_t>> SymbolFlags,
+    ArrayRef<WasmSymEntry> SymbolFlags,
     ArrayRef<std::pair<uint16_t, uint32_t>> InitFuncs,
----------------
Rename this argument to `Symbolnfo`?


================
Comment at: lib/Object/WasmObjectFile.cpp:743
+      if (Ex.Index < NumImportedFunctions ||
+          Ex.Index >= FunctionTypes.size() + NumImportedFunctions)
         return make_error<GenericBinaryError>("Invalid function export",
----------------
isValidFunctionWasmIndex?


================
Comment at: lib/Object/WasmObjectFile.cpp:750
+      if (Ex.Index < NumImportedGlobals ||
+          Ex.Index >= Globals.size() + NumImportedGlobals)
         return make_error<GenericBinaryError>("Invalid global export",
----------------
Should we add isValidGlobalIndex?  Maybe that plus the above can be a separate simply change?


================
Comment at: lib/Object/WasmObjectFile.cpp:926
+         ? GlobalSymbols[Index - FunctionSymbols.size()]
+         : FunctionSymbols[Index];
 }
----------------
Again, not sure we need to separate these do we?  Seems like just makes our lives harder.  


================
Comment at: test/tools/llvm-nm/wasm/weak-symbols.yaml:61
     SymbolInfo:
-      - Name:            weak_global_func
+      - Kind:            FUNCTION
+        Index:           1
----------------
It would probably be useful to have the symbol index here too so it clear which symbol the info is for.   I've been going this for other sections too:  https://reviews.llvm.org/D41877

I guess you would need to rename the existing Index: field then.  Perhaps to WasmIndex?



Repository:
  rL LLVM

https://reviews.llvm.org/D41954