[PATCH] D43147: [WebAssembly] Add first class symbol table to wasm objects

[Nicholas Wilson via Phabricator via llvm-commits]

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

Great! All looks familiar, we've been rehashing this code for a fortnight so there shouldn't be much left to pick over.

You say this diff is a combination of two reviews - but I don't think it is, it looks like the second patch only. I'm sure you've got the rest of the changes there, they're just not showing up in Phabricator.

I've made some picky comments, but really I'd happy for it to be committed as-is. Just a few formatting things I think I may have fixed in later revisions - maybe you took your branch off a couple of revisions back from the last update I made? Doesn't matter.



================
Comment at: include/llvm/BinaryFormat/Wasm.h:259
 
+const unsigned WASM_SYMBOL_UNDEFINED          = 0x100;
+
----------------
Should this be renumbered down to 0x8 or 0x10, to use the next available bit, now that it's written out?


================
Comment at: include/llvm/MC/MCSymbolWasm.h:23
+    GLOBAL
+  };
+
----------------
I thought we weren't duplicating this enum, can't it be a wasm::WasmSymbolType? Would also save the function in WasmObjectWriter to translate between the two enums.


================
Comment at: include/llvm/Object/Wasm.h:95
+      Out << ", ElemIndex=" << Info.ElementIndex;
+    } else if ((Info.Flags & wasm::WASM_SYMBOL_UNDEFINED) == 0) {
+      Out << ", Segment=" << Info.DataRef.Segment;
----------------
Could be `else if (isDefined())`


================
Comment at: lib/MC/WasmObjectWriter.cpp:1016
+    if (WS.isData())
+      Info.DataRef = DataLocations[&WS];
+    else
----------------
When I got rid of the `Optional<WasmDataRef>`, I made it so that only defined data symbols get put in DataLocations. So actually this is not only reading from DataLocations, it's inserting into it.

To avoid that, in my code I avoiding doing the insertion:

```
if (!WS.isData())
  Info.ElementIndex = WasmIndices[&WS];
else if (WS.isDefined())
  Info.DataRef = DataLocations[&WS];
```


================
Comment at: lib/MC/WasmObjectWriter.cpp:1073-1077
         Import.Type = PtrType;
         Import.IsMutable = false;
-        WasmIndices[&WS] = NumGlobalImports++;
         // If this global is the stack pointer, make it mutable.
         if (WS.getName() == "__stack_pointer")
           Import.IsMutable = true;
----------------
I actually simplified this in a later commit - it could go in here. I don't like all the special-casing of the stack pointer!

```
Import.Type = WS.getGlobalType().Type;
Import.IsMutable = WS.getGlobalType().Mutable;
```


================
Comment at: lib/MC/WasmObjectWriter.cpp:1143
     if (WS.isFunction()) {
+      unsigned WasmIndex;
       if (WS.isDefined()) {
----------------
I'm sure I renamed this from WasmIndex to just Index at your request...


================
Comment at: lib/Object/WasmObjectFile.cpp:419
+      if (IsDefined) {
+        assert(isDefinedFunctionIndex(Info.ElementIndex));
+        Info.Name = readString(Ptr);
----------------
Should these be checked in all builds, not just debug builds with assertions? Otherwise it's just inconsistent with the index check on the line above - in fact it could just be

```
if (!isValidFunctionIndex(Info.ElementIndex) ||
    IsDefined != isDefinedFunctionIndex(Info.ElementIndex))
  return make_error<>("invalid function symbol index", ...);
```


Repository:
  rL LLVM

https://reviews.llvm.org/D43147