[PATCH] D92321: [WebAssembly] Allow element sections for nonzero table numbers

[Andy Wingo via Phabricator via llvm-commits]

wingo marked 9 inline comments as done.
wingo added inline comments.


================
Comment at: lld/wasm/SyntheticSections.cpp:450
+    writeU8(os, elemKind, "elem kind");
+  }
+
----------------
wingo wrote:
> sbc100 wrote:
> > sbc100 wrote:
> > > This flag is never set so this is dead code isn't it?
> > > 
> > > I'm guessing that when we add actual multi table here we will still want to separate the linker-synthetic indirectFunctionTable from other tables that are composed of actual input segments, so I'm not sure it makes since to make this code too generic here.
> > I still think this block should be completely removed.   This method exists only for writing the indirectFunctionTable which by definition does not have this flag and does need this code block.   If we ever change the type of indirectFunctionTable we can change this function.  
> > 
> > When we add a method for writing a generic element section it most likely won't be a SyntheticSections like this one is but one that is made up of InputChunk..  that method will want to be more generic but I don't see why this one should be.  
> Aaah yes I had removed it locally; I think I must have fat-fingered the "arc diff".  Tx for sharp eye.
As noted below, this isn't dead code at all.  You have to write the elem kind if the table number is nonzero.  LMK your thoughts here.


================
Comment at: llvm/lib/MC/WasmObjectWriter.cpp:936
+    W->OS << ElemKind;
+  }
+
----------------
sbc100 wrote:
> wingo wrote:
> > sbc100 wrote:
> > > This is dead code no?  Since that flag cannot be set?
> > > 
> > > In fact all these changes to writeElemSection look unused (and therefore untested), perhaps we can split this out and make it part of a future PR?
> > So the tableNumber changes above are definitely used and tested.  Definitely possible to have non-zero table numbers for the indirect function table.  I could remove this HAS_ELEM_KIND bit here but let me push back gently -- we would like to test that this output is spec-conforming and also that it "makes" sense" from a LLVM point of view (e.g. can round trip through `tables.s`).  But the former is more important than the latter.  By structuring this function to look more like the [binary spec]((https://webassembly.github.io/reference-types/core/binary/modules.html#binary-elemsec)) -- e.g. checking the `HAS_TABLE_NUMBER` flag instead of checking `TableNumber` -- I have greater confidence reading the code that it does what the spec says.
> Sure, I don't really care if you check TableNumber of HAS_TABLE_NUMBER above.
> 
> But these six lines seems compltely pointless and in fact that are dead code right?    This function doesn't write arbitrary tables it *only* writes the special __indirect_function_table (at least today).   Can that ever have an ElemKind?  
Paging this back in -- this code was not dead.  See the definition for WASM_ELEM_SEGMENT_HAS_ELEM_KIND.  I have restored it here.  WDYT?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92321