[PATCH] D88697: [WebAssembly] Rename Emscripten EH functions

[Heejin Ahn via Phabricator via llvm-commits]

aheejin added inline comments.


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyAsmPrinter.cpp:204
+      // Emscripten EH symbol so we don't emit the same symbol twice.
+      if (Sym->getName().startswith("invoke_"))
+        if (!EmSymbols.insert(Sym).second)
----------------
sbc100 wrote:
> What happens here if there is a user symbol that starts with `invoke_`?   Perhaps getMCSymbolForFunction could also return bool if it detected an `__invoke_` wrapper? 
> 
> Might be worth adding a function called `invoke_ignoreme` as a normal symbol in one of the tests?
> What happens here if there is a user symbol that starts with `invoke_`?

We haven't handled this situation so far, and this patch does not change this. Invokes' format has set in Emscripten, so any user code that uses function name starting with `invoke_` has a problem now. I think we should consider prepending Emscripten's invokes with two underscores as in the names generated by LowerEmscriptenEHSjLj pass, given that functions with two leading underscores are mostly considered compiler-specific functions. But I'd like to do it as a separate patch, as that changes the interface between LLVM, Binaryen, and Emscripten. Do you think this is a good idea?

> Perhaps getMCSymbolForFunction could also return bool if it detected an `__invoke_` wrapper?

What will it be used for?


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyLowerEmscriptenEHSjLj.cpp:640-641
+// arguments of type {i32, i32} and longjmp takes {jmp_buf*, i32}, so we need a
+// ptrtoint instruction here to make the type match. jmp_buf* will eventually be
+// lowered to i32 in the wasm backend.
+static void replaceLongjmpWithEmscriptenLongjmp(Function *LongjmpF,
----------------
tlively wrote:
> Do we emit an error if EmscriptenEHSjLj is used with wasm64?
No. Supporting wasm64 in Emscripten EHSjLj will be a simple task, but I guess no one needs it now and supporting it should be a separate CL. Erroring out sounds good to me for an interim measure. Did that.

The condition here is little tricky: Emscripten always enables SjLj, so we can't error out whenever SjLj is enabled. But we can check by whether `setjmp` or `longjmp` is actually used, so we error out only when one of those are used.
Whether we really use EH is hard to check unless we scan every single instruction in all functions. Anyway Emscripten EH is not enabled by default, so we error out whenever EH is enabled. (EH does not use `emscripten_longjmp`, but apparently other global variables used in this pass also needs adjusting to i64, which is not hard if we need to do that)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88697