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

[Sam Clegg via Phabricator via llvm-commits]

sbc100 added inline comments.


================
Comment at: lld/test/wasm/tag-section.ll:14
+; RUN: llc -filetype=obj -wasm-enable-eh -exception-model=wasm -mattr=+exception-handling -relocation-model=pic %s -o %t.o
+; RUN: wasm-ld --import-undefined --experimental-pic -pie -o %t.wasm %t.o %t1.o %t2.o
+; RUN: obj2yaml %t.wasm | FileCheck %s --check-prefix=PIC
----------------
aheejin wrote:
> I'm not very sure what each of `--experimental-pic` and `-pie` does... Without `--experimental-pic` this doesn't work and without `-pie` it emits a warning, so I ended up putting both. 
Yes, you need them both today.  `-pie` is experimental so you need to opt into this feature.   `-pie` outputs a position independent executable, which is what emscripten used for `MAIN_MODULE` today.


================
Comment at: lld/test/wasm/tag-section.ll:52
+
+; In PIC mode, tags are undefined and imported from JS.
+; PIC:        Sections:
----------------
Maybe add `; PIC-NOT: - Type:            TAG` ?


================
Comment at: lld/wasm/InputFiles.cpp:670
+    // in which we don't define those symbols. These symbols are always binding
+    // global.
+    assert(sym.isBindingGlobal());
----------------
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?


================
Comment at: lld/wasm/Symbols.h:70
            symbolKind == UndefinedGlobalKind ||
-           symbolKind == UndefinedTableKind;
+           symbolKind == UndefinedTableKind || symbolKind == UndefinedTagKind;
   }
----------------
Maybe follow the pattern of one comparison per line here?


================
Comment at: llvm/test/CodeGen/WebAssembly/eh-lsda.ll:4
 
-target triple = "wasm32-unknown-unknown"
+target triple = "wasm32-unknown-emscripten"
 
----------------
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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111388