[PATCH] D41954: [WebAssembly] Add symbol table to LLVM, 1/2

[Sam Clegg via Phabricator via llvm-commits]

sbc100 accepted this revision.
sbc100 added a comment.
This revision is now accepted and ready to land.

Ok.. I think we are almost there now.

I'm still not a fan of landing this in two parts, but I'm prepared to do it if land them close together I guess.



================
Comment at: include/llvm/Object/Wasm.h:55
 
-  // Index into either the function or global index space.
-  uint32_t ElementIndex;
+  // Index into either the Symbol index space, which consists of all Symbols
+  // together, in order of declaration.
----------------
I think "either" is not supposed to be here.


================
Comment at: include/llvm/Object/Wasm.h:61
+  // or global, and consists of the imports then definitions.
+  uint32_t WasmIndex;
 
----------------
Can we avoid renaming this for now and continue to call it ElementIndex?  Especially since the above "SymbolIndex" is removed in the followup change.   We can rename it later perhaps, but I keeping it the same will reduce churn and make this change more concise.


================
Comment at: include/llvm/Object/Wasm.h:105
+        << ", Flags=" << Flags << " SymbolIndex=" << SymbolIndex
+        << ", WasmIndex=" << WasmIndex;
   }
----------------
Revert this change, since SymbolIndex is a temporary measure?


================
Comment at: lib/Object/WasmObjectFile.cpp:458
         Init.FunctionIndex = readVaruint32(Ptr);
-        if (!isValidFunctionIndex(Init.FunctionIndex))
+        if (!isValidFunctionSymbolIndex(Init.FunctionIndex))
           return make_error<GenericBinaryError>("Invalid function index: " +
----------------
So we want to INIT_FUNCS to refer to symbols rather than functions?


================
Comment at: lib/Object/WasmObjectFile.cpp:969
   case WasmSymbol::SymbolType::GLOBAL_EXPORT: {
-    uint32_t GlobalIndex = Sym.ElementIndex - NumImportedGlobals;
+    uint32_t GlobalIndex = Sym.WasmIndex - NumImportedGlobals;
     assert(GlobalIndex < Globals.size());
----------------
Revert this?


Repository:
  rL LLVM

https://reviews.llvm.org/D41954