[PATCH] D111388: [WebAssembly] Make EH work with dynamic linking

[Heejin Ahn via Phabricator via llvm-commits]

aheejin added inline comments.


================
Comment at: lld/wasm/InputFiles.cpp:670
+    // in which we don't define those symbols. These symbols are always binding
+    // global.
+    assert(sym.isBindingGlobal());
----------------
sbc100 wrote:
> Maybe remove of rephrase this comment..  It conceivable that other folks could build other object (either via .s or some other mechanism that could contain undefined tags).    Or remove the limitation?
> 
> I'm curious why the compiler wasn't complaining before if this enum value was missing from this swtich?
> Maybe remove of rephrase this comment..  It conceivable that other folks could build other object (either via .s or some other mechanism that could contain undefined tags).    Or remove the limitation?

Removed the limitation.

> I'm curious why the compiler wasn't complaining before if this enum value was missing from this swtich?

I think it's because `WasmSymbolInfo.Kind`'s type is not `enum WasmSymbolType` but `uint32_t`: https://github.com/llvm/llvm-project/blob/63aab4065b452ae5693481dfd12fed3c184261c7/llvm/include/llvm/BinaryFormat/Wasm.h#L192


================
Comment at: lld/wasm/Symbols.h:70
            symbolKind == UndefinedGlobalKind ||
-           symbolKind == UndefinedTableKind;
+           symbolKind == UndefinedTableKind || symbolKind == UndefinedTagKind;
   }
----------------
sbc100 wrote:
> Maybe follow the pattern of one comparison per line here?
This is what clang-format insists. Currently our wasm lld code conforms to clang-format nearly 99%, so I'm not sure if this is worth making an exception..?


================
Comment at: llvm/test/CodeGen/WebAssembly/eh-lsda.ll:4
 
-target triple = "wasm32-unknown-unknown"
+target triple = "wasm32-unknown-emscripten"
 
----------------
sbc100 wrote:
> It seems like a shame to use coverage of eh-lsda for the non-emscripten targets?   cna you remove the target tripple completely here and instead set it from the command line.  Then you can use emscripten for the PIC test and unknown for the non-PIC test?
For EH/LSDA we don't do anything differently for emscripten and non-emscripten targets, but yeah, will change to a command line argument, and also will add wasm64 tests. So this will test 6 combinations:
1. `wasm32-unknown-unknown`
2. `wasm64-unknown-unknown`
3. `wasm32-unknown-emscripten`
4. `wasm64-unknown-emscripten`
5. `wasm32-unknown-emscripten` + PIC
6. `wasm64-unknown-emscripten` + PIC


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111388