[PATCH] D54875: [WebAssembly] Add support for the event section

[Heejin Ahn via Phabricator via llvm-commits]

aheejin added inline comments.


================
Comment at: wasm/InputEntity.h:31
+
+class InputEntity {
+public:
----------------
ruiu wrote:
> We should keep the class hierarchy as shallow as they can be, and we should avoid using inheritance if we can live without it. If something can be written just as a non-member function, we should write that way. I'm trying to not use too many object-oriented features in lld.
> 
> It looks like this abstract class is not really needed to me; you could just define InputGlobal and InputEvent as two separate classes. Can you make that change?
Done. Do you think it's ok to keep the two classes `InputGlobal` and `InputEvent` in this same header `InputEntity.h`? If so, do you have any other opinions on the file name?


================
Comment at: wasm/SymbolTable.cpp:160
+         toString(Existing->getFile()) + "\n>>> defined as " +
+         toString(*NewSig) + " in " + toString(File));
+}
----------------
sbc100 wrote:
> I would expect this to looks basically the same as checkFunctionType.
> 
> What are we storing the Attribute exactly?  (should it be Attributes plural?)
> I would expect this to looks basically the same as checkFunctionType.
That's not possible, because we have to check both the `Attribute` and the signature. `WasmEventType`'s (in)equality operator just checks whether `Attribute` matches, because comparing `SigIndex` of `WasmEventType`s from different objects does not make sense.

> What are we storing the Attribute exactly?
The possible list of attributes is [[ https://github.com/llvm-mirror/llvm/blob/c448b1493f2e013141162f9b753347d36d0abc3a/include/llvm/BinaryFormat/Wasm.h#L271-L274 | here ]], which is, only `WASM_EVENT_ATTRIBUTE_EXCEPTION` for now. The `Attribute` field is supposed to store the kind of event, for later when we get to support other kinds of events that modifies control flow in a various way. Possible candidates have been discussed [[ https://github.com/WebAssembly/exception-handling/blob/master/proposals/old/Level-1%2BN.md#level-3 | here ]], but we don't have a concrete plan to implement any of these yet now.

> (should it be Attributes plural?)
Why do you think so?


================
Comment at: wasm/Symbols.h:318
+
+class UndefinedEvent : public EventSymbol {
+public:
----------------
ruiu wrote:
> sbc100 wrote:
> > IIRC we don't have undefined events yet do we?    Is it worth leaving that out until we have a use case for them?
> Do you really have to distinguish this type of object from other symbols by class? It feels like it is too much use of class inheritance.
@sbc100 Deleted it.

@ruiu This does not necessarily increase the depth of inheritance; we need at least `EventSymbol` and `DefinedEvent` (which is child of `EventSymbol`). `EventSymbol` has a different characteristics from all other existing symbols such as `FunctionSymbol` or `GlobalSymbol`, so we can't merge them. We don't currently have `UndefinedEvent` so I can delete it.


================
Comment at: wasm/WriterUtils.cpp:214
+  return (Type.Attribute == WASM_EVENT_ATTRIBUTE_EXCEPTION ? "exception"
+                                                           : "unknown");
 }
----------------
sbc100 wrote:
> if sig was part of this we could print it here too.
How? `WasmEventType` just contains an int sigindex, not a `WasmSignature` object.


Repository:
  rLLD LLVM Linker

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

https://reviews.llvm.org/D54875