[PATCH] D54096: [WebAssembly] Add support for the event section
Heejin Ahn via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Tue Nov 6 02:20:26 PST 2018
aheejin added a comment.
Thank you for comments.
================
Comment at: include/llvm/BinaryFormat/Wasm.h:72
+struct WasmEventType {
+ uint32_t Attribute;
+ uint32_t SigIndex;
----------------
aardappel wrote:
> Some comments as to what these refer to, esp the uint32_t types?
This follows the spec [here](https://github.com/WebAssembly/exception-handling/blob/master/proposals/Exceptions.md#event_type). The spec says the attribute data type to be `varuint32`. These `uint32_t` values will be later LEB-encoded in WasmObjectWriter.
As for comments, the spec says "The attribute of the event", which is not very descriptive either, so I didn't add any. Do you have other suggestions on what to add?
================
Comment at: include/llvm/BinaryFormat/Wasm.h:189
WASM_SEC_GLOBAL = 6, // Global declarations
- WASM_SEC_EXPORT = 7, // Exports
- WASM_SEC_START = 8, // Start function declaration
- WASM_SEC_ELEM = 9, // Elements section
- WASM_SEC_CODE = 10, // Function bodies (code)
- WASM_SEC_DATA = 11 // Data segments
+ WASM_SEC_EVENT = 7, // Event declarations
+ WASM_SEC_EXPORT = 8, // Exports
----------------
dschuff wrote:
> sbc100 wrote:
> > These integer values are defined by the spec I think, which means you can't just change them. I imagine you will have to use `13`, or what even number the spec decides on, but specify that it must come between section 6 and section 7.
> >
> > Of course this will make the section parsing a code a little complex but I don't see any way around this.
> I think these enums correspond to section IDs in the spec (https://webassembly.github.io/spec/core/binary/modules.html) and can't be changed. I guess Event should just get number 12, even though it goes here in the ordering? We should probably add that to the spec proposal too.
Changed the event section code to 12 and restored all other section codes. The corresponding spec update is in [this PR](https://github.com/WebAssembly/exception-handling/pull/67).
================
Comment at: include/llvm/CodeGen/WasmEHFuncInfo.h:24
+enum ThrowTag { CPP_EXCEPTION = 0, C_LONGJMP = 1 };
+
----------------
dschuff wrote:
> I don't see uses of these tags. Is the idea that these would be the tags used by LLVM to throw/catch? I guess we'd want to use event symbols for this? Something like `catch __cpp_exception` where cpp_exception is an event symbol that gets a relocation to point to the event section entry with the right signature?
They are not really tags but just enum values used in `WebAssemblyTargetLowering::LowerINTRINSIC_VOID` down there. As you can see there, the code is something like
```
case Intrinsic::wasm_throw: {
int Tag = cast<ConstantSDNode>(Op.getOperand(2).getNode())->getZExtValue();
switch (Tag) {
case CPP_EXCEPTION: { // <---- It's used here
const TargetLowering &TLI = DAG.getTargetLoweringInfo();
MVT PtrVT = TLI.getPointerTy(DAG.getDataLayout());
const char *SymName = MF.createExternalSymbolName("__cpp_exception");
SDValue SymNode =
DAG.getNode(WebAssemblyISD::Wrapper, DL, PtrVT,
DAG.getTargetExternalSymbol(
SymName, PtrVT, WebAssemblyII::MO_SYMBOL_EVENT));
return DAG.getNode(WebAssemblyISD::THROW, DL,
MVT::Other, // outchain type
{
Op.getOperand(0), // inchain
SymNode, // exception symbol
Op.getOperand(3) // thrown value
});
}
default:
llvm_unreachable("Invalid tag!");
}
```
So depending on the immediate enum operand of `throw` intrinsic, we create a different event symbol. `C_LONGJMP` is just a reserved value to use when we add setjmp/longjmp handling.
================
Comment at: include/llvm/MC/MCSymbolWasm.h:27
+ wasm::WasmEventType EventType;
+ bool EventTypeSet = false;
----------------
aardappel wrote:
> nicer to have an "invalid" member of `WasmEventType` ?
How? It is a struct in `include/llvm/BinaryFormat/Wasm.h`:
```
struct WasmEventType {
uint32_t Attribute;
uint32_t SigIndex;
};
```
And there are many other similar struct in that file.
================
Comment at: lib/CodeGen/AsmPrinter/WasmException.cpp:22
+void WasmException::endModule() {
+ // This is the label used in 'throw' instruction to denote this is a C++
+ // exception. This label has to be emitted somewhere once in the module. Check
----------------
aardappel wrote:
> why do we use a label for this?
Maybe the term 'label' was misleading. It's `__cpp_exception` event symbol that's used in `throw` instructions, and to make the symbol //defined//, it had to be emitted somewhere in the module. I'll change the term to just 'symbol', which I guess sounds less confusing.
================
Comment at: lib/MC/WasmObjectWriter.cpp:62
// TODO: Consider using WasmSignature directly instead.
-struct WasmFunctionType {
+struct WasmType {
// Support empty and tombstone instances, needed by DenseMap.
----------------
sbc100 wrote:
> Would `WasmSignature` be a better name?
Wouldn't it be confused with [[ https://github.com/llvm-mirror/llvm/blob/5bc1446f3bee779c5a15a0256169bc7623682121/include/llvm/BinaryFormat/Wasm.h#L275-L285 | `wasm::WasmSignature` ]]? But to think about it they are kind of similar and there is even a TODO there saying consider using it directly instead...
And there are many vectors called `Types` in this file that store this type of objects, so I'm kind of torn. What do you think?
================
Comment at: lib/Target/WebAssembly/MCTargetDesc/WebAssemblyTargetStreamer.cpp:121
+ for (auto Ty : Sym->getSignature()->Params)
+ OS << ", " << WebAssembly::TypeToString(Ty);
+ OS << '\n';
----------------
aardappel wrote:
> We should maybe decide to make all signatures look the same, so they can parsed equally (both by the assembler and a human :)
> We should merge .param and .return? Should that have a symbol name much like .eventtype? Use void instead of just omitting .return?
Opened an [[ https://github.com/WebAssembly/tool-conventions/issues/72 | issue ]] to discuss this.
Repository:
rL LLVM
https://reviews.llvm.org/D54096
More information about the llvm-commits
mailing list