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

[Sam Clegg via Phabricator via llvm-commits]

sbc100 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)
----------------
aheejin wrote:
> aheejin wrote:
> > 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?
> I ended up renaming invokes to use two leading underscores in this patch. I wanted to it as a separate patch, but that will again need three different patches each from LLVM, Binaryen, and Emscripten, and landing it through the roller using a double roll twice is also a pain, so I ended up doing it here. Will do the same for the pending Binaryen and Emscripten paches.
I was actually thinking of the case where there a *not* a collision.

e.g. `invoke_foo` 

This cannot possibly collide today because `foo` is not a valid signature.  The code today seems to allow this and pass through such symbols.  wasm-emscripten-finalize includes this function in the normal `exports` section not in the `invokeFuncs` section of the metadata.

By renaming the functions to use two underscores we solve this issue but I still think it might be nicer for `getMCSymbolForFunction` to return an additional bool if it created a wrapper.. rather then trying to detect this post-hoc with a call to isEmscriptenInvokeWrapperName on the result.   


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88697