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

[Heejin Ahn via Phabricator via llvm-commits]

aheejin added inline comments.


================
Comment at: include/llvm/MC/MCSymbolWasm.h:74
   }
+  void setEventType(wasm::WasmEventType ET) { EventType = ET; }
 };
----------------
sbc100 wrote:
> aardappel wrote:
> > aheejin wrote:
> > > @aardappel I changed the types of `GlobalType` and `EventType` to `llvm::Optional` and now we can remove those `GlobalTypeSet` and `EventTypeSet`. Thanks for the suggestion.
> > awesome.. that's a solution I hadn't thought of.
> lgtm, as long as making these Optional doesn't require the destructor being called.   IIRC we use of have a non-POD type in MCWasmSymbol before and we failed with msan because of that.  Do Optional POD types like this allocate?  I assume not.
That's my understanding. `llvm::Optional`, like `std::optional`, uses placement new and does not allocate memory  in heap.


================
Comment at: tools/yaml2obj/yaml2wasm.cpp:482
-      if (Type < LastType) {
-        errs() << "Out of order section type: " << Type << "\n";
-        return 1;
----------------
sbc100 wrote:
> I feel like the section ordering constraint should still be enforces somewhere,  presumably when parsing the yaml into the in-memory sections.  But we also need to enforce it when parsing the binaries in WasmObjectFile and I'm OK with that being done in a later change.
How can we enforce it when reading file? The event section appears between global section and export section in the section order now but it has section code 0xC. In both binary and yaml files, the event section occurs before the export section, so the assumption of monotonically increasing section ordering is not valid anymore.


Repository:
  rL LLVM

https://reviews.llvm.org/D54096