[PATCH] D62210: [WebAssembly] Implement __builtin_return_address for emscripten

[Sam Clegg via Phabricator via llvm-commits]

sbc100 added inline comments.


================
Comment at: llvm/include/llvm/IR/RuntimeLibcalls.def:543
+// Return address
+HANDLE_LIBCALL(RETURN_ADDRESS, nullptr)
+
----------------
This seems a little strange given that its in generic code and also its that only actual LIBCALL with nullptr as the second arg here.  Can you explain why this is needed?   I would imagine it should have a comment since its different from all the others.


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyISelLowering.cpp:277
+  // Define the emscripten name for return address helper.
+  setLibcallName(RTLIB::RETURN_ADDRESS, "emscripten_return_address");
+
----------------
quantum wrote:
> sbc100 wrote:
> > I kind of agree with Dan re: the naming of this.
> > 
> > If anything we should we at least put "__" at the start so that its clear this is not a normal user function.
> > 
> > What is this called on other plaforms?  Why not just call it "__builtin_return_address"?
> The function already exists in emscripten under that name. In emscripten, these functions don't have the `__` prefix, for example, `emscripten_get_callstack`.
> 
> On other platforms, `__builtin_return_address` is lowered to code that reads LR or pulls it out of the stack, instead of a helper function.
OK, fair enough.

Can you add a TODO() here to replace this with a more generic thing (wither from tool-conventions, or from wasi I guess) in the future?


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyRuntimeLibcallSignatures.cpp:497
+
+    Map["emscripten_return_address"] = RTLIB::RETURN_ADDRESS;
   }
----------------
Does this need to be protected by an `if (is_emscripten())` check too?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62210