[WebAssemblyLowerEmscriptenEHSjLj] Don't assign import names to generated function declarations #71599
Conversation
| @llvm/pr-subscribers-backend-webassembly Author: Sam Clegg (sbc100) ChangesThese days we would prefer that the linker report errors when these symbols are not defined. Assigning import names like this tells the linker not to report errors when these symbols are missing and simply import them. However, this leads to much less useful undefined symbol errors after the linker is done. Note that keep the old behavior for the See emscripten-core/emscripten#20536 Full diff: https://github.com/llvm/llvm-project/pull/71599.diff 4 Files Affected:
diff --git a/llvm/lib/Target/WebAssembly/WebAssemblyLowerEmscriptenEHSjLj.cpp b/llvm/lib/Target/WebAssembly/WebAssemblyLowerEmscriptenEHSjLj.cpp index 5e84fff351b23d1..4c64c50974538f3 100644 --- a/llvm/lib/Target/WebAssembly/WebAssemblyLowerEmscriptenEHSjLj.cpp +++ b/llvm/lib/Target/WebAssembly/WebAssemblyLowerEmscriptenEHSjLj.cpp @@ -448,7 +448,12 @@ static std::string getSignature(FunctionType *FTy) { static Function *getEmscriptenFunction(FunctionType *Ty, const Twine &Name, Module *M) { - Function* F = Function::Create(Ty, GlobalValue::ExternalLinkage, Name, M); + return Function::Create(Ty, GlobalValue::ExternalLinkage, Name, M); +} + +static Function *getInvokeFunction(FunctionType *Ty, const Twine &Name, + Module *M) { + Function* F = getEmscriptenFunction(Ty, Name, M); // Tell the linker that this function is expected to be imported from the // 'env' module. if (!F->hasFnAttribute("wasm-import-module")) { @@ -591,7 +596,7 @@ Function *WebAssemblyLowerEmscriptenEHSjLj::getInvokeWrapper(CallBase *CI) { FunctionType *FTy = FunctionType::get(CalleeFTy->getReturnType(), ArgTys, CalleeFTy->isVarArg()); - Function *F = getEmscriptenFunction(FTy, "__invoke_" + Sig, M); + Function *F = getInvokeFunction(FTy, "__invoke_" + Sig, M); InvokeWrappers[Sig] = F; return F; } diff --git a/llvm/test/CodeGen/WebAssembly/lower-em-ehsjlj-options.ll b/llvm/test/CodeGen/WebAssembly/lower-em-ehsjlj-options.ll index aa4d87756c87d0a..9694e1c5b442ee3 100644 --- a/llvm/test/CodeGen/WebAssembly/lower-em-ehsjlj-options.ll +++ b/llvm/test/CodeGen/WebAssembly/lower-em-ehsjlj-options.ll @@ -6,18 +6,10 @@ target triple = "wasm32-unknown-unknown" ; EH: .functype invoke_vi (i32, i32) -> () -; EH: .import_module invoke_vi, env -; EH: .import_name invoke_vi, invoke_vi ; EH-NOT: .functype __invoke_void_i32 -; EH-NOT: .import_module __invoke_void_i32 -; EH-NOT: .import_name __invoke_void_i32 ; SJLJ: .functype emscripten_longjmp (i32, i32) -> () -; SJLJ: .import_module emscripten_longjmp, env -; SJLJ: .import_name emscripten_longjmp, emscripten_longjmp ; SJLJ-NOT: .functype emscripten_longjmp_jmpbuf -; SJLJ-NOT: .import_module emscripten_longjmp_jmpbuf -; SJLJ-NOT: .import_name emscripten_longjmp_jmpbuf %struct.__jmp_buf_tag = type { [6 x i32], i32, [32 x i32] } diff --git a/llvm/test/CodeGen/WebAssembly/lower-em-sjlj.ll b/llvm/test/CodeGen/WebAssembly/lower-em-sjlj.ll index 7115b01ed1618e9..5bb052b65a8d3ae 100644 --- a/llvm/test/CodeGen/WebAssembly/lower-em-sjlj.ll +++ b/llvm/test/CodeGen/WebAssembly/lower-em-sjlj.ll @@ -305,13 +305,6 @@ attributes #0 = { returns_twice } attributes #1 = { noreturn } attributes #2 = { nounwind } attributes #3 = { allocsize(0) } -; CHECK-DAG: attributes #{{[0-9]+}} = { nounwind "wasm-import-module"="env" "wasm-import-name"="getTempRet0" } -; CHECK-DAG: attributes #{{[0-9]+}} = { nounwind "wasm-import-module"="env" "wasm-import-name"="setTempRet0" } -; CHECK-DAG: attributes #{{[0-9]+}} = { "wasm-import-module"="env" "wasm-import-name"="__invoke_void" } -; CHECK-DAG: attributes #{{[0-9]+}} = { "wasm-import-module"="env" "wasm-import-name"="saveSetjmp" } -; CHECK-DAG: attributes #{{[0-9]+}} = { "wasm-import-module"="env" "wasm-import-name"="testSetjmp" } -; CHECK-DAG: attributes #{{[0-9]+}} = { noreturn "wasm-import-module"="env" "wasm-import-name"="emscripten_longjmp" } -; CHECK-DAG: attributes #{{[0-9]+}} = { "wasm-import-module"="env" "wasm-import-name"="__invoke_ptr_i32_ptr" } ; CHECK-DAG: attributes #[[ALLOCSIZE_ATTR]] = { allocsize(1) } !llvm.dbg.cu = !{!2} diff --git a/llvm/test/CodeGen/WebAssembly/lower-wasm-ehsjlj.ll b/llvm/test/CodeGen/WebAssembly/lower-wasm-ehsjlj.ll index 25471eb50081b39..ce417a5db26308b 100644 --- a/llvm/test/CodeGen/WebAssembly/lower-wasm-ehsjlj.ll +++ b/llvm/test/CodeGen/WebAssembly/lower-wasm-ehsjlj.ll @@ -109,7 +109,7 @@ catch: ; preds = %catch.start catchret from %2 to label %catchret.dest ; CHECK: catch: ; preds = %catch.start ; CHECK-NEXT: %exn = load ptr, ptr %exn.slot15, align 4 -; CHECK-NEXT: %5 = call ptr @__cxa_begin_catch(ptr %exn) #7 [ "funclet"(token %2) ] +; CHECK-NEXT: %5 = call ptr @__cxa_begin_catch(ptr %exn) #3 [ "funclet"(token %2) ] ; CHECK-NEXT: invoke void @__cxa_end_catch() [ "funclet"(token %2) ] ; CHECK-NEXT: to label %.noexc unwind label %catch.dispatch.longjmp |
| ✅ With the latest revision this PR passed the C/C++ code formatter. |
| Sorry for the delayed reply. Does this work because now those symbols arrive not from JS code but from our C/C++ library code? Then does this work with dynamic linking too? Also check the clang-format thing |
The reason this now works is that we pass a list of known JS function to the linker, which tells the linker its OK to import them. So we no longer need to tell the linker in to import them using this method. The advantage is that when the list of available JS symbols doesn't include one of these we get a nice error from wasm-ld. |
…ated function declarations These days we would prefer that the linker report errors when these symbols are not defined. Assigning import names like this tells the linker not to report errors when these symbols are missing and simply import them. However, this leads to much less useful undefined symbol errors after the linker is done. Note that keep the old behavior for the `invoke_xxx` functions since its not possible for is to declare all possible permutations ahead of time in a library. See emscripten-core/emscripten#20536
Not necessarily related to this PR, but can you point where in emscripten we do that, to help my understanding of how they work? Thanks! |
4 participants
These days we would prefer that the linker report errors when these symbols are not defined. Assigning import names like this tells the linker not to report errors when these symbols are missing and simply import them. However, this leads to much less useful undefined symbol errors after the linker is done.
Note that keep the old behavior for the
invoke_xxxfunctions since its not possible for is to declare all possible permutations ahead of time in a library.See emscripten-core/emscripten#20536