[PATCH] D66356: [WebAssembly] Forbid use of EM_ASM with setjmp/longjmp

[Guanzhong Chen via Phabricator via llvm-commits]

quantum added inline comments.


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyLowerEmscriptenEHSjLj.cpp:550
+  Function *EmAsmConstAsyncMainF =
+      M.getFunction("emscripten_asm_const_async_on_main_thread");
+
----------------
tlively wrote:
> Where does this list of functions come from? How do we know it's exhaustive?
Stated that the list is exhaustive and link the header where these functions came from.


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyLowerEmscriptenEHSjLj.cpp:554
+         Callee == EmAsmConstIntSyncMainF ||
+         Callee == EmAsmConstDoubleSyncMainF || Callee == EmAsmConstAsyncMainF;
+}
----------------
tlively wrote:
> Maybe use `std::any_of` over an array of the function names as plain strings to reduce repetition?
This is the style used in this file, so we are doing it for consistency.


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyLowerEmscriptenEHSjLj.cpp:1004
+      if (isEmAsmCall(M, Callee))
+        report_fatal_error("Cannot use EM_ASM* with setjmp/longjmp in " +
+                               F.getName() + ". Please consider using EM_JS.",
----------------
kripken wrote:
> How about "alongside" instead of "with", to hint that the issue is using both in the same function?
> 
> And/or maybe replace the final "." with ", or refactor the EM_ASM to another function"?
Done.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66356