[WebAssemblyLowerEmscriptenEHSjLj] Avoid setting import_name where possible #128564

sbc100 · 2025-02-24T19:51:34Z

This change effectively reverts 296ccef (https://reviews.llvm.org/D77192)

Most of these symbols are just normal C symbols that get imported from wither libcompiler-rt or from emscripten's JS library code. In most cases it should not be necessary to give them explicit import names.

The advantage of doing this is that we can wasm-ld can/will fail with a useful error message when these symbols are missing. As opposed to today where it will simply import them and defer errors until later (when they are less specific).

llvmbot · 2025-02-24T19:52:16Z

@llvm/pr-subscribers-backend-webassembly

Author: Sam Clegg (sbc100)

Changes

Most of these symbols are just normal C symbols that get imported from wither libcompiler-rt or from emscripten's JS library code. In most cases it should not be necessary to give them explicit import names.

The advantage of doing this is that we can wasm-ld can/will fail with a useful error message when these symbols are missing. As opposed to today where it will simply import them and defer errors until later (when they are less specific).

Full diff: https://github.com/llvm/llvm-project/pull/128564.diff

1 Files Affected:

(modified) llvm/lib/Target/WebAssembly/WebAssemblyLowerEmscriptenEHSjLj.cpp (+17-12)

diff --git a/llvm/lib/Target/WebAssembly/WebAssemblyLowerEmscriptenEHSjLj.cpp b/llvm/lib/Target/WebAssembly/WebAssemblyLowerEmscriptenEHSjLj.cpp
index c60cf69c30104..6eb61bd4b3159 100644
--- a/llvm/lib/Target/WebAssembly/WebAssemblyLowerEmscriptenEHSjLj.cpp
+++ b/llvm/lib/Target/WebAssembly/WebAssemblyLowerEmscriptenEHSjLj.cpp
@@ -440,9 +440,14 @@ static std::string getSignature(FunctionType *FTy) {
   return Sig;
 }
 
+static Function *getFunction(FunctionType *Ty, const Twine &Name,
+                                       Module *M) {
+  return Function::Create(Ty, GlobalValue::ExternalLinkage, Name, M);
+}
+
 static Function *getEmscriptenFunction(FunctionType *Ty, const Twine &Name,
                                        Module *M) {
-  Function* F = Function::Create(Ty, GlobalValue::ExternalLinkage, Name, M);
+  Function* F = getFunction(Ty, Name, M);
   // Tell the linker that this function is expected to be imported from the
   // 'env' module.
   if (!F->hasFnAttribute("wasm-import-module")) {
@@ -927,11 +932,11 @@ bool WebAssemblyLowerEmscriptenEHSjLj::runOnModule(Module &M) {
   // exception handling and setjmp/longjmp handling
   ThrewGV = getGlobalVariable(M, getAddrIntType(&M), TM, "__THREW__");
   ThrewValueGV = getGlobalVariable(M, IRB.getInt32Ty(), TM, "__threwValue");
-  GetTempRet0F = getEmscriptenFunction(
-      FunctionType::get(IRB.getInt32Ty(), false), "getTempRet0", &M);
-  SetTempRet0F = getEmscriptenFunction(
-      FunctionType::get(IRB.getVoidTy(), IRB.getInt32Ty(), false),
-      "setTempRet0", &M);
+  GetTempRet0F = getFunction(FunctionType::get(IRB.getInt32Ty(), false),
+                             "getTempRet0", &M);
+  SetTempRet0F =
+      getFunction(FunctionType::get(IRB.getVoidTy(), IRB.getInt32Ty(), false),
+                  "setTempRet0", &M);
   GetTempRet0F->setDoesNotThrow();
   SetTempRet0F->setDoesNotThrow();
 
@@ -942,13 +947,13 @@ bool WebAssemblyLowerEmscriptenEHSjLj::runOnModule(Module &M) {
     // Register __resumeException function
     FunctionType *ResumeFTy =
         FunctionType::get(IRB.getVoidTy(), IRB.getPtrTy(), false);
-    ResumeF = getEmscriptenFunction(ResumeFTy, "__resumeException", &M);
+    ResumeF = getFunction(ResumeFTy, "__resumeException", &M);
     ResumeF->addFnAttr(Attribute::NoReturn);
 
     // Register llvm_eh_typeid_for function
     FunctionType *EHTypeIDTy =
         FunctionType::get(IRB.getInt32Ty(), IRB.getPtrTy(), false);
-    EHTypeIDF = getEmscriptenFunction(EHTypeIDTy, "llvm_eh_typeid_for", &M);
+    EHTypeIDF = getFunction(EHTypeIDTy, "llvm_eh_typeid_for", &M);
   }
 
   // Functions that contains calls to setjmp but don't have other longjmpable
@@ -988,14 +993,14 @@ bool WebAssemblyLowerEmscriptenEHSjLj::runOnModule(Module &M) {
       // Register emscripten_longjmp function
       FunctionType *FTy = FunctionType::get(
           IRB.getVoidTy(), {getAddrIntType(&M), IRB.getInt32Ty()}, false);
-      EmLongjmpF = getEmscriptenFunction(FTy, "emscripten_longjmp", &M);
+      EmLongjmpF = getFunction(FTy, "emscripten_longjmp", &M);
       EmLongjmpF->addFnAttr(Attribute::NoReturn);
     } else { // EnableWasmSjLj
       Type *Int8PtrTy = IRB.getPtrTy();
       // Register __wasm_longjmp function, which calls __builtin_wasm_longjmp.
       FunctionType *FTy = FunctionType::get(
           IRB.getVoidTy(), {Int8PtrTy, IRB.getInt32Ty()}, false);
-      WasmLongjmpF = getEmscriptenFunction(FTy, "__wasm_longjmp", &M);
+      WasmLongjmpF = getFunction(FTy, "__wasm_longjmp", &M);
       WasmLongjmpF->addFnAttr(Attribute::NoReturn);
     }
 
@@ -1009,11 +1014,11 @@ bool WebAssemblyLowerEmscriptenEHSjLj::runOnModule(Module &M) {
       FunctionType *FTy = FunctionType::get(
           IRB.getVoidTy(), {SetjmpFTy->getParamType(0), Int32Ty, Int32PtrTy},
           false);
-      WasmSetjmpF = getEmscriptenFunction(FTy, "__wasm_setjmp", &M);
+      WasmSetjmpF = getFunction(FTy, "__wasm_setjmp", &M);
 
       // Register __wasm_setjmp_test function
       FTy = FunctionType::get(Int32Ty, {Int32PtrTy, Int32PtrTy}, false);
-      WasmSetjmpTestF = getEmscriptenFunction(FTy, "__wasm_setjmp_test", &M);
+      WasmSetjmpTestF = getFunction(FTy, "__wasm_setjmp_test", &M);
 
       // wasm.catch() will be lowered down to wasm 'catch' instruction in
       // instruction selection.

github-actions · 2025-02-24T19:54:56Z

✅ With the latest revision this PR passed the C/C++ code formatter.

Depends on llvm/llvm-project#128564 to allow wasm-ld to report the object file in question. Fixes: emscripten-core#23732

aheejin · 2025-02-25T05:22:18Z

Does the rationale in 296ccef not apply anymore? I guess I can understand why it's unnecessary in case of C symbol + static linking but

Is it fine in case of C symbols with dynamic linking?
Is it fine in case of JS symbols with static/dynamic linking?

Now we have only two functions that are labeled with wasm-import-module: invokes and __cxa_matching_catchs. Are they different from other JS functions?

sbc100 · 2025-02-25T17:55:47Z

Does the rationale in 296ccef not apply anymore? I guess I can understand why it's unnecessary in case of C symbol + static linking but

Is it fine in case of C symbols with dynamic linking?

Is it fine in case of JS symbols with static/dynamic linking?

Now we have only two functions that are labeled with wasm-import-module: invokes and __cxa_matching_catchs. Are they different from other JS functions?

Yes the invoke_xxx and __cxa_matching_catch_xxx function are still explicitly marked with import-name and import-module.

This is because their names are dynamic and we have no way for the static linker to declare their existence in a JS library (at least not without enumerating all of them somehow, which seems hard/impossible).

One way of thinking about this is that explicitly marking these symbols are imported is something we should avoid, if we can. But for dynamic symbol names I don't know of any way to avoid it.

sbc100 · 2025-02-25T17:57:21Z

Thanks for pointing out 296ccef. I think I will use the to make this PR more explict.

…ssible This change effectively reverts 296ccef (https://reviews.llvm.org/D77192) Most of these symbols are just normal C symbols that get imported from wither libcompiler-rt or from emscripten's JS library code. In most cases it should not be necessary to give them explicit import names. The advantage of doing this is that we can wasm-ld can/will fail with a useful error message when these symbols are missing. As opposed to today where it will simply import them and defer errors until later (when they are less specific).

sbc100 · 2025-02-25T18:12:43Z

Updated to reflect that this is basically a revert of 296ccef.

sbc100 · 2025-02-25T19:26:21Z

Gentle ping (I've got an emscripten-side change waiting on this). BTW I ran the whole other and core tests suites in emscripten with this patch applied.

dschuff · 2025-02-25T22:25:19Z

llvm/lib/Target/WebAssembly/WebAssemblyLowerEmscriptenEHSjLj.cpp

-                                       Module *M) {
-  Function* F = Function::Create(Ty, GlobalValue::ExternalLinkage, Name, M);
+static Function *getFunction(FunctionType *Ty, const Twine &Name, Module *M) {
+  return Function::Create(Ty, GlobalValue::ExternalLinkage, Name, M);


Is this function really an improvement now over just calling Function::Create directly?

I'm happy to switch to that if you prefer?

I guess it avoids typing GlobalValue::ExternalLinkage a lot?

Yeah I guess I could go either way on that. LGTM for whatever you want to do.

aheejin

Sorry for the delay.

Chatted with @sbc100 offline, and answering my own question: The reason that we needed this years ago but not anymore is because we generate libemscripten_js_symbols.so file that supplies all JS symbol names so wasm-ld knows they should be imported: https://github.com/emscripten-core/emscripten/blob/71e31f4cbbfd922d16fe814769590be4342d1e17/tools/building.py#L126-L136

llvmbot added the backend:WebAssembly label Feb 24, 2025

sbc100 requested a review from aheejin February 24, 2025 19:54

sbc100 force-pushed the EmscriptenEHSjLj branch from e13b625 to 307fc50 Compare February 24, 2025 19:59

sbc100 added a commit to sbc100/emscripten that referenced this pull request Feb 24, 2025

Add better error message when mixing exceptions modes. NFC

4fe29d7

Depends on llvm/llvm-project#128564 to allow wasm-ld to report the object file in question. Fixes: emscripten-core#23732

sbc100 force-pushed the EmscriptenEHSjLj branch from 307fc50 to 5779a1a Compare February 24, 2025 21:13

sbc100 requested a review from dschuff February 24, 2025 21:13

This was referenced Feb 24, 2025

Add better error message when mixing exceptions modes. NFC emscripten-core/emscripten#23738

Open

Clearer linker error due to sjlj ABI mismatch? emscripten-core/emscripten#23732

Open

sbc100 force-pushed the EmscriptenEHSjLj branch from 5779a1a to e287ef5 Compare February 25, 2025 18:11

dschuff reviewed Feb 25, 2025

View reviewed changes

aheejin approved these changes Feb 26, 2025

View reviewed changes

sbc100 merged commit 147d9d6 into llvm:main Feb 26, 2025
11 checks passed

sbc100 deleted the EmscriptenEHSjLj branch February 26, 2025 22:05

huaatian mentioned this pull request Feb 28, 2025

fix live interval empty issue huaatian/llvm-project#1

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WebAssemblyLowerEmscriptenEHSjLj] Avoid setting import_name where possible #128564

[WebAssemblyLowerEmscriptenEHSjLj] Avoid setting import_name where possible #128564

sbc100 commented Feb 24, 2025 •

edited

Loading

llvmbot commented Feb 24, 2025

github-actions bot commented Feb 24, 2025 •

edited

Loading

aheejin commented Feb 25, 2025

sbc100 commented Feb 25, 2025

sbc100 commented Feb 25, 2025

sbc100 commented Feb 25, 2025

sbc100 commented Feb 25, 2025

dschuff Feb 25, 2025

sbc100 Feb 25, 2025

sbc100 Feb 25, 2025

dschuff Feb 26, 2025

aheejin left a comment

[WebAssemblyLowerEmscriptenEHSjLj] Avoid setting import_name where possible #128564

[WebAssemblyLowerEmscriptenEHSjLj] Avoid setting import_name where possible #128564

Conversation

sbc100 commented Feb 24, 2025 • edited Loading

llvmbot commented Feb 24, 2025

github-actions bot commented Feb 24, 2025 • edited Loading

aheejin commented Feb 25, 2025

sbc100 commented Feb 25, 2025

sbc100 commented Feb 25, 2025

sbc100 commented Feb 25, 2025

sbc100 commented Feb 25, 2025

dschuff Feb 25, 2025

Choose a reason for hiding this comment

sbc100 Feb 25, 2025

Choose a reason for hiding this comment

sbc100 Feb 25, 2025

Choose a reason for hiding this comment

dschuff Feb 26, 2025

Choose a reason for hiding this comment

aheejin left a comment

Choose a reason for hiding this comment

sbc100 commented Feb 24, 2025 •

edited

Loading

github-actions bot commented Feb 24, 2025 •

edited

Loading