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

[Sam Clegg via Phabricator via llvm-commits]

sbc100 added inline comments.


================
Comment at: include/llvm/BinaryFormat/Wasm.h:88
   uint32_t Attribute;
-  uint32_t SigIndex;
 };
----------------
aheejin wrote:
> sbc100 wrote:
> > aheejin wrote:
> > > 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.
> > I think so yes.  If you look at the InputGlobal class in lld today it looks like already makes copy of the InputGlobal.   I imagine the InputEvent class would have the WasmEvent in it.  These object do not need to be the exact objects that get written to the output.  It happens that for InputGlobal we do end up calling writeGlobal with that as the input, but for events you could do something like this:
> > 
> > e.g.
> > ```
> > for (const InputEvent *E : InputEvent)  {
> >   WasmEvent NewEv = *E->Event;
> >   // Replace the input sig index with the output sig index.
> >   NewEv->Type->SigIndex = lookupType(E->Type>SigIndex);
> >    writeEvent(OS, NewEv); 
> > }
> > ```
> I'm not sure which option you are suggesting.
> 
> 1. Create a new set of `WasmGlobal`, `WasmEvent` and other similar structures in lld side
> 2. Create only a new `WasmEvent` class in lld side (which I think is inconsistent)
> 3. Use the current [[ https://github.com/llvm-mirror/llvm/blob/0833f2640ef8874438edb516785b58faae6acd38/include/llvm/BinaryFormat/Wasm.h#L91-L95 | `wasm::WasmEvent` ]] defined in [[ https://github.com/llvm-mirror/llvm/blob/master/include/llvm/BinaryFormat/Wasm.h | include/BinaryFormat/Wasm.h ]], but just create a new instance of this
> 
> I thought you were suggesting either 1 or 2 and now you sound like 3.
> 
> And currently, the `InputGlobal` class and its constructor looks like this:
> ```
> class InputGlobal {                                                              
> public:                                                                          
>   InputGlobal(const WasmGlobal &G, ObjFile *F)                                   
>       : File(F), Global(G), Live(!Config->GcSections) {}
>   ...                                                                                                                                                  
>   WasmGlobal Global;                                                             
>   ...                                    
> };
> ```
> By the way, is the member variable `WasmGlobal Global` not being a reference but a value, so that we get to copy `WasmGlobal` in the constructor, intentional? I'm wondering if that was intentionally by value to allow modification in cases like this.
I think I'm advocating for 3, which I believe matches what we do with Globals and Imports, etc today.

The fact that the WasmGlobal is, I think, so that createSyntheticSymbols() can pass a stack allocated new global and have it copied.

In the case of Event I think its reasonable to use a const reference to the Event in the input object here.  The idea is that this member represents the item in the input file, so it cannot be written as-is to the output, a new event based on the input event is what should be written.  The input event won't have the correct indexes etc within it.

Remember that for Globals and Events we normally/currently have exactly 1 of each in the link.  (__stack_pointer and __cpp_exception).  So there is really no need to try to avoid copying them at this point.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D54873