[PATCH] D54873: [WebAssembly] Make signature index not a part of EventType

[Heejin Ahn via Phabricator via llvm-commits]

aheejin marked an inline comment as done.
aheejin added inline comments.


================
Comment at: include/llvm/BinaryFormat/Wasm.h:88
   uint32_t Attribute;
-  uint32_t SigIndex;
 };
----------------
sbc100 wrote:
> My initial reaction is that we should leave this as is.
> 
> The linker shouldn't be modifying the input EventTypes but instead creating a new set of output eventtypes based on the input ones.  At least that how it makes sense to me.  I will take a look at the lld side change and see if I can make some suggestions there.
You mean we should create separate set of everything, like `WasmEventType`, `WasmGlobalType`, ... and copy all data from the old structures to the new structures in the linker to resolve this? Wouldn't that be too much duplication? We already have separate `WasmEventType` and `WasmGlobalType`, ... in `WasmObjectWriter` side (lib/MC). I would want to avoid adding another set of all these, just to resolve signature index. And if we only add `WasmEventType` for the output type in the linker, that would look inconsistent as well.

And the original reason `SigIndex` was a part of `WasmEventType` was just the [spec](https://github.com/WebAssembly/exception-handling/blob/master/proposals/Exceptions.md#event_type) has a table structured that way, which is not very important reason. (This CL does not change the binary spec itself)

And the other thing is, taking `SigIndex` out of a certain type (here `WasmEventType`) might be more extensible way, for when we have multi-value globals. In that case every type can have an optional `SigIndex` seems a easier way to handle things.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D54873