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

Wed Nov 28 13:31:52 PST 2018

sbc100 added inline comments.

================
Comment at: include/llvm/BinaryFormat/Wasm.h:88
   uint32_t Attribute;
-  uint32_t SigIndex;
 };
----------------
aheejin wrote:
> sbc100 wrote:
> > dschuff wrote:
> > > sbc100 wrote:
> > > > 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.
> > > I think in the future we may have more globals (even aside from the special known ones such as stack pointer, memory_base, table_base): there may be references once we have support for reference types, and there may be languages other than C that either use LLVM or want to produce LLVM-compatible object files. So I think it makes sense for the linker and object file code to have the capability of treating them in a generic way. I guess that doesn't necessarily mean we can't copy them (e.g. unlike functions, but like events, they will be small) but we should at least consider how we want to handle linking for them even if we don't use it in clang for now.
> > I agree.  Actually we don't copy them right now in order to gain mutability, but simply to handle being passed as stack allocated synthetic global.  It would be easy enough to change the ownership model here.  I don't think it relates to this change actually.
> @sbc100 In the initial code example you suggested:
> ```
> 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); 
> }
> ```
> `InputEvent` can't be `const` anymore because now we are modifying `WasmEvent` inside it. And also existing `InputGlobal::getType` returns `const WasmGlobalType&`, which `InputEvent` mimics in D54875. If we want to change something inside `InputEvent` after it is created, this method can't be `const` anymore. That's fine with me, but `InputEvent::getType()` returns `WasmEventType&` while `InputGlobal::getType()` returns `const WasmGlobalType&` seems inconsistent.
> 
> And the reason I asked why those member variables are by value and not reference is not really relevant to whether we should take out `SigIndex` out of a certain type (e.g. `EventType`). I agree they are separate things. I still think generalizing `SigIndex` so that every type can optionally have one is more extensible in face of multi-value globals. 
Sorry I don't understand.  The above code should not be modifying the InputEvent.  It modifies NewEv which is on the stack right?   NewEv is the output event where InputEvent is the input event which we should not change.  Maybe I made a mistake in my code?

Repository:
  rL LLVM

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

https://reviews.llvm.org/D54873