[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:
> 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.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D54873