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

[Sam Clegg via Phabricator via llvm-commits]

sbc100 added a subscriber: ruiu.
sbc100 added a comment.

lgtm % nits and the question of whether we leave SigIndex as part of the EventType in wasm.h.



================
Comment at: wasm/InputFiles.cpp:61
+      "\n       Global Imports : " + Twine(WasmObj->getNumImportedGlobals()) +
+      "\n       Event Imports : " + Twine(WasmObj->getNumImportedEvents()));
 }
----------------
Align on `:`


================
Comment at: wasm/InputFiles.cpp:280
+    const WasmEvent &E = WasmObj->events()[I];
+    Events.emplace_back(make<InputEvent>(Types[EventTypes[I]], E, this));
+  }
----------------
It would make sense to me if the type was part of the Event struct.


================
Comment at: wasm/SymbolTable.cpp:146
+  if (!isa<EventSymbol>(Existing)) {
+    reportTypeError(Existing, File, WASM_SYMBOL_TYPE_GLOBAL);
+    return;
----------------
TYPE_EVENT?


================
Comment at: wasm/SymbolTable.cpp:160
+         toString(Existing->getFile()) + "\n>>> defined as " +
+         toString(*NewSig) + " in " + toString(File));
+}
----------------
I would expect this to looks basically the same as checkFunctionType.

What are we storing the Attribute exactly?  (should it be Attributes plural?)


================
Comment at: wasm/SymbolTable.cpp:195
 
+DefinedEvent *SymbolTable::addSyntheticEvent(StringRef Name, uint32_t Flags,
+                                             InputEvent *Event) {
----------------
I would drop all references to syntheticevents until/unless you actually need them.


================
Comment at: wasm/Symbols.h:318
+
+class UndefinedEvent : public EventSymbol {
+public:
----------------
IIRC we don't have undefined events yet do we?    Is it worth leaving that out until we have a use case for them?


================
Comment at: wasm/Writer.cpp:196
+      // Reassign the new signature after merging all object files
+      Import.SigIndex = lookupType(*EventSym->Signature);
+    } else {
----------------
If you leave SigIndex as part of the EventType could you write this as:

```
Import.Event.Attributes = EventSym->getEventType().Attributes;
Import.Event.SigIndex = lookupType(*EventSym->Signature);
```


================
Comment at: wasm/Writer.cpp:797
     else
-      cast<GlobalSymbol>(Sym)->setGlobalIndex(NumImportedGlobals++);
+      assert(false && "Invalid symbol for imports");
   }
----------------
A tip from @ruiu for this kind of case is to use a `cast` after the last `else` rather than a `dyn_cast`.  This will fail in the same way without needing the additional assert.


Repository:
  rLLD LLVM Linker

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

https://reviews.llvm.org/D54875