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

[Nicholas Wilson via Phabricator via llvm-commits]

ncw added a comment.

In https://reviews.llvm.org/D42495#991420, @sbc100 wrote:

> Some initial comments.  Looks good in general!
>
> Although I like small changes, I'm not sure I want to land that part 1 of this separately.   Lets see how this evolves and how they look when combined too.


Parts 1 & 2 should certainly be in different svn commits (I feel), because otherwise you'd have a single commit where basically every line of test code changes. In the current split, roughly half the test output changes in each commit, which is rather more reassuring. Are you suggesting actually squashing them into a single mega-review?

I can see your point though that Parts 1 & 2 might want to be merged in immediate succession. Is your reasoning simply that you don't want to break the binary format too often?

In practice by the time a couple of other chunks are split off it'll be harder to merge the patch queue in one blast, rather than incrementally. Part 1 is sensible on its own - all functionality works and tests pass.

There are a few smaller changes that we could clean up now while pondering the big ones: https://reviews.llvm.org/D42095, https://reviews.llvm.org/D42539, https://reviews.llvm.org/D42540.



================
Comment at: include/llvm/BinaryFormat/Wasm.h:75
+  uint32_t Index;
+  WasmGlobalType Type;
   WasmInitExpr InitExpr;
----------------
sbc100 wrote:
> This separation of GlobalType looks good, but can perhaps be standalone change unrelated to symbols?
Good point, candidate for splitting.


================
Comment at: include/llvm/BinaryFormat/Wasm.h:229
   WASM_COMDAT_FUNCTION    = 0x1,
+  WASM_COMDAT_GLOBAL      = 0x2,
+};
----------------
sbc100 wrote:
> Can this be split out and added separately?  (Or is it only relevant in this context?)
If it were split out, it would have to come //after// the big symbol change refactor, since before that point globals are all fake. I guess that would be possible to do that, but it would be a fairly small post-commit. I can remove it, for now.


================
Comment at: include/llvm/Object/Wasm.h:41
+    DATA     = wasm::WASM_SYMTAB_DATA,
+    GLOBAL   = wasm::WASM_SYMTAB_GLOBAL,
   };
----------------
sbc100 wrote:
> If these mirror the existing types then maybe just drop this enum completely?
Possibly, yes, but it does mirror existing usage. The enums in BinaryFormat are `enum : unsigned` and are used for assigning values read out of a binary, and the enums in Object are `enum class` and are used for logical operations like switch statements where you want the strongest compiler warnings.


================
Comment at: include/llvm/Object/Wasm.h:60
   // together, in order of declaration.
   uint32_t SymbolIndex;
   // For a function or global symbol, the index into the the Wasm index space.
----------------
sbc100 wrote:
> I think we can drop SymbolIndex completely now can't we?  The yaml can still include it just like it does for the other Index's but I don't think needs to be part of this data structure does it?
I could check - the //concept// of SymbolIndex is still very much used, but maybe the variable isn't? I'd be surprised, but if it is now obsolete I'll remove it.


================
Comment at: lib/MC/WasmObjectWriter.cpp:1002-1019
-  // In the special .global_variables section, we've encoded global
-  // variables used by the function. Translate them into the Globals
-  // list.
-  MCSectionWasm *GlobalVars =
-      Ctx.getWasmSection(".global_variables", SectionKind::getMetadata());
-  if (!GlobalVars->getFragmentList().empty()) {
-    if (GlobalVars->getFragmentList().size() != 1)
----------------
Now this I will split out into another commit.

Do you know how this code got here? It's completely unused, and appears like it wouldn't even work. That's because we're emitting globals here that aren't related to any MCSymbols, so they don't generate relocations even in the existing code - and without a R_GLOBAL relocation how could this have ever worked after linking?


================
Comment at: lib/MC/WasmObjectWriter.cpp:1212
+      if (WS.isDefined()) {
+        // A definition. Write out the Global definition.
+        WasmIndex = NumGlobalImports + Globals.size();
----------------
sbc100 wrote:
> If this only __stack_pointer right now, can we wait on adding support for defined globals?  Can this, and WASM_COMDAT_GLOBAL itself be a separate change?
Yes, this little bit of stub code for defined globals isn't necessary and could be pulled out into a later commit. Would make sense.

Note - although this looks like an un-split mega-diff, I do actually have several followup chunks already split out, but I just haven't made the reviews for them to avoid premature spam.


================
Comment at: test/Object/obj2yaml.test:658
+WASM-NEXT:         Kind:            FUNCTION
+WASM-NEXT:         SymbolIndex:     0
+WASM-NEXT:         Flags:           [  ]
----------------
sbc100 wrote:
> Can you just call this "Index" and have it come first, just like we do for the other indexes (i.e. Type index).    It should always be contiguous and always start at zero but can still be useful in the yaml output I think.  
:( Drat, regenerate every test again.... OK, fair enough, it's worth getting it right while we're at it.


Repository:
  rL LLVM

https://reviews.llvm.org/D42495