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

[Derek Schuff via Phabricator via llvm-commits]

dschuff added inline comments.


================
Comment at: lib/CodeGen/AsmPrinter/WasmException.cpp:29
+  if (Asm->OutContext.lookupSymbol(NameStr)) {
+    MCSymbol *ExceptionSym = Asm->GetExternalSymbolSymbol("__cpp_exception");
+    Asm->OutStreamer->EmitLabel(ExceptionSym);
----------------
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?)


================
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.
----------------
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`.


================
Comment at: lib/Target/WebAssembly/WebAssemblyMCInstLower.cpp:102
+    // weak.
+    WasmSym->setWeak(true); // TODO aheejin how to set odr?
+    WasmSym->setExternal(true);
----------------
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?


Repository:
  rL LLVM

https://reviews.llvm.org/D54096