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

[Heejin Ahn via Phabricator via llvm-commits]

aheejin added inline comments.


================
Comment at: include/llvm/CodeGen/WasmEHFuncInfo.h:24
 
+enum ThrowTag { CPP_EXCEPTION = 0, C_LONGJMP = 1 };
+
----------------
aheejin wrote:
> 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.
> 
Changed the name to `EventTag` because this is also gonna be used by other instructions like `if_except`.


================
Comment at: lib/CodeGen/AsmPrinter/WasmException.cpp:29
+  if (Asm->OutContext.lookupSymbol(NameStr)) {
+    MCSymbol *ExceptionSym = Asm->GetExternalSymbolSymbol("__cpp_exception");
+    Asm->OutStreamer->EmitLabel(ExceptionSym);
----------------
dschuff wrote:
> What is the linkage for this symbol? Is it 'external' in the linkage sense (in which case we have to worry about multiple definitions) or undefined ( in which case where should it be defined?)
As you saw below, its linkage is 'weak' and 'external' so that if there are multiple compilation units, it will be resolved in the linker. And this is the place where it gets defined. This is why it is emitted here, in which way each compilation module gets a (weak) definition.


================
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.
----------------
dschuff wrote:
> aheejin wrote:
> > 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?
> `WasmSignature` would definitely be a better name, and this does represent the same concept as `wasm::WasmSignature` (hence why I put the TODO to merge them).  I think `WasmType` might be confused with e.g. ValueType.  Since `WasmSignature` is taken we could call this... hm... I don't know what would be a good but not-ugly name. We could call it something ugly like `EncodedSignature` or `FunctionOrEventType` to make it clearer that this should be fixed; or namespace it like `ObjectFile::WasmSignature` to make it really clear that this is the same thing as `WasmSignature`.
This is not in `wasm::` namespace, so we can actually use `WasmSignature`. (There are also other classes both defined here and `wasm::` namespace in [[ https://github.com/llvm-mirror/llvm/blob/master/include/llvm/BinaryFormat/Wasm.h | `include/llvm/BinaryFormat/Wasm.h` ]], such as `WasmFunction`)
Changed the class name to `WasmSignature` and other local/parameter names to match it.


================
Comment at: lib/Target/WebAssembly/WebAssemblyMCInstLower.cpp:102
+    // weak.
+    WasmSym->setWeak(true); // TODO aheejin how to set odr?
+    WasmSym->setExternal(true);
----------------
dschuff wrote:
> sbc100 wrote:
> > dschuff wrote:
> > > I guess this answers my question above.
> > > We could have this symbol weakly defined in throwing/catching object files, and then strongly defined in libcxxabi? Or we could use a COMDAT?
> > Why would need a strong definition in libcxxabi?  For that matter there is no syntax for defining it in code so I'm not sure how we would do that (assembly I guess?)
> Sorry I was thinking of extern_weak. Then you would need a definition. But yeah weakly-defined should work here. And yeah, there's no way to define an event-type symbol in C, but assembly syntax should support it.
We don't need a strongly defined symbol because when there are multiple weak symbols the linker can choose any one of them. Currently event symbols are defined as weak. And it is emitted in `WasmException::endModule()` in `lib/CodeGen/AsmPrinter/WasmException.cpp` in this CL, which serves as its definition. The proof this is defined is, in `test/MC/WebAssembly/event-symbol.ll` test, the event symbol's flags are shown as `BINDING_WEAK` but not `UNDEFINED`. And if it is undefined this should show up in the import section, which it does not.

I don't know how COMDAT is different from a weak symbol in this case, so if there's advantage using that instead please let me know.


Repository:
  rL LLVM

https://reviews.llvm.org/D54096