[WebAssembly] Make __wasm_lpad_context thread-local

[aheejin]

Summary

This makes __wasm_lpad_context, a struct that is used as a
communication channel between compiler-generated code and personality
function in libunwind, thread local. The library code will be changed to
thread local in the emscripten side.

[sbc100]

llvm/lib/CodeGen/WasmEHPrepare.cpp

220

If this object file was built without atomics and then this code didn't mark the variable as TLS then isn't this object incompatible with shared-memory at link time? In fact wouldn't it fail to link with the defintion of __wasm_lpad_context which is going to be TLS?

i.e. object files build with exception handling (i.e. references to __wasm_lpad_context) but not +atomics are not compatible with shared memory at link time, right?

227

For simplicity can we just check for atomics?

232

Can we simplify the code and avoid the TLS and EnableTLS variables and just do this above?

if (FS.contains("+atomics") && FS.contains("+bulk-memory")) 
   LPadContextGV->setThreadLocalMode(GlobalValue::GeneralDynamicTLSModel)

[aheejin]

Simplify

[aheejin]

llvm/lib/CodeGen/WasmEHPrepare.cpp

220

I don't really remember details about that. I mostly copy-pasted this: https://github.com/llvm/llvm-project/blob/6822d89e776960caa753fd4181fe99feaef5032b/llvm/lib/Target/WebAssembly/WebAssemblyLowerEmscriptenEHSjLj.cpp#L418-L427
@tlively may know the details about the rules..?

The difference is the existing code checks the subtarget to check the features, but here in lib/CodeGen we don't have the access to TargetMachine so I checked function attributes.

227

Again, I just followed https://github.com/llvm/llvm-project/blob/6822d89e776960caa753fd4181fe99feaef5032b/llvm/lib/Target/WebAssembly/WebAssemblyLowerEmscriptenEHSjLj.cpp#L418-L427.

[tlively]

llvm/lib/CodeGen/WasmEHPrepare.cpp

220

Hmm this is interesting.

If we didn't check the features here and unconditionally marked the symbol as being TLS, then when atomics and bulk-memory are not enabled, we would strip the TLS marker and disallow the object from being linked into multithreaded programs. Last time we thought about this, we decided that that behavior was overly restrictive, so we added this feature check. But @sbc100, you're saying that disallowing the object from being linked into multithreaded programs is the correct, desired behavior because those multithreaded programs will define the symbols to be TLS, so any non-TLS use of them would be incorrect.

What happens when you have a non-TLS use of a symbol that is defined to be TLS? It makes sense to me that that shouldn't work and should be disallowed. If so, we should update both this new code and the existing code @aheejin linked to so that they unconditionally use TLS.

[sbc100]

llvm/lib/CodeGen/WasmEHPrepare.cpp

220

I had thought that a non-TLS relocation against a TLS symbol would cause the linker to error.. and I think it probably should. But it looks like today we only check for the inverse use case:

https://github.com/llvm/llvm-project/blob/4bafe65c2b2f1ce745894a509a6d80c87fb1c335/lld/wasm/Relocations.cpp#L130-L138

I think the reason that we we more careful in WebAssemblyLowerEmscriptenEHSjLj.cpp was that it maybe it could effect object files that don't actually use the setjmp/longjmp feature.. is they right?

In this case, here in WasmEHPrepare.cpp, am I right in thinking that only object files that actually use exception handling will contain references to the __wasm_lpad_context symbol?

[tlively]

llvm/lib/CodeGen/WasmEHPrepare.cpp

220

I'm not sure. @aheejin, is there any difference between when __wasm_lpad_context is used and when __THREW__ and __threwValue are used? Regardless of when they're used, this TLS/non-TLS mismatch still needs to be solved no matter how inconvenient it is for the end user AFAICT, but maybe I'm missing something that makes these two cases different.

[aheejin]

llvm/lib/CodeGen/WasmEHPrepare.cpp

220

@tlively

Can you remind me what the rule is? IIRC it was not as simple as "We disallow linking an object built without atomic/bulk-memory with an object built w/ atomic/bulk-memory", right? In which case do we allow linking of an object built w/ threads and w/o threads?

What happens when you have a non-TLS use of a symbol that is defined to be TLS?

What are examples of a non-TLS use of a TLS symbol?

@sbc100

I think the reason that we we more careful in WebAssemblyLowerEmscriptenEHSjLj.cpp was that it maybe it could effect object files that don't actually use the setjmp/longjmp feature.. is they right?
In this case, here in WasmEHPrepare.cpp, am I right in thinking that only object files that actually use exception handling will contain references to the __wasm_lpad_context symbol?

I think that's right. Currently in LowerEmscriptenEHSjLj we create these variables whether we really need EH/SjLj transformations or not, which I think is better to fix. Also as you said we create __wasm_lpad_context only when we need it.

[aheejin]

llvm/lib/CodeGen/WasmEHPrepare.cpp

220

@tlively I don't think there's difference between TLS uses of __THREW__/__threwValue and __wasm_lpad_context.

[tlively]

llvm/lib/CodeGen/WasmEHPrepare.cpp

220

Can you remind me what the rule is?

We disallow linking an object file built without atomics/bulk-memory _that contain a stripped atomic operation or TLS variable_ into multithreaded programs because they started out as being thread-safe but stripping the atomic operation or TLS means it is no longer thread-safe. Objects with atomics/bulk-memory that don't contain any atomic operations or TLS to begin with are still allowed to be linked into multithreaded programs.

https://github.com/llvm/llvm-project/blob/4625b848793f8cfa4affac8d03b9f498b3e477fe/llvm/lib/Target/WebAssembly/WebAssemblyTargetMachine.cpp#L206-L217

[aheejin]

llvm/lib/CodeGen/WasmEHPrepare.cpp

220

@tlively

We disallow linking an object file built without atomics/bulk-memory _that contain a stripped atomic operation or TLS variable_ into multithreaded programs because they started out as being thread-safe but stripping the atomic operation or TLS means it is no longer thread-safe. Objects with atomics/bulk-memory that don't contain any atomic operations or TLS to begin with are still allowed to be linked into multithreaded programs.

• with -> without in the last sentence, right?
• Is an object that is built w/o threads and contains original normal loads/stores (not stripped ones) OK to be linked with an object that's compiled w/ threads and contains atomic operations?

[sbc100]

llvm/lib/CodeGen/WasmEHPrepare.cpp

220

@tlively

We disallow linking an object file built without atomics/bulk-memory _that contain a stripped atomic operation or TLS variable_ into multithreaded programs because they started out as being thread-safe but stripping the atomic operation or TLS means it is no longer thread-safe. Objects with atomics/bulk-memory that don't contain any atomic operations or TLS to begin with are still allowed to be linked into multithreaded programs.

• with -> without in the last sentence, right?

No I think he means "with" as in "with the +atomics and +bulk-memory features enabled". Having the features is not enough to make you incompatible... its only the usage of those features getting stripped out that make you incompatible.

• Is an object that is built w/o threads and contains original normal loads/stores (not stripped ones) OK to be linked with an object that's compiled w/ threads and contains atomic operations?

Yes, as long as there is no stripping then the object is good to go in either shared or non-shared programs (at least that is the idea).

[aheejin]

llvm/lib/CodeGen/WasmEHPrepare.cpp

220

But thinking about this more, I think these should be marked as TLS unconditionally; they are meant to be thread-local and if this object is linked with a program built with threads, I don't think this will work correctly. Currently __THREW__ and __threwValue are generated even if they are not used, but we can fix that. How about 1. fixing that so those variables are generated only when used and 2. making these variables (__THREW__, __threwValue, and __wasm_lpad_context) unconditionally thread local? @tlively @sbc100

[tlively]

llvm/lib/CodeGen/WasmEHPrepare.cpp

220

with -> without in the last sentence, right?

Yes, sorry about that. Whether or not atomics and bulk memory (or some combination of them) are enabled, objects that had no atomics or TLS to start with should always be allowed to be linked into multithreaded programs.

220

But thinking about this more, I think these should be marked as TLS unconditionally; they are meant to be thread-local and if this object is linked with a program built with threads, I don't think this will work correctly. Currently __THREW__ and __threwValue are generated even if they are not used, but we can fix that. How about 1. fixing that so those variables are generated only when used and 2. making these variables (__THREW__, __threwValue, and __wasm_lpad_context) unconditionally thread local? @tlively @sbc100

This sounds perfect 👍

[sbc100]

llvm/lib/CodeGen/WasmEHPrepare.cpp

220

Sounds good, although I think you can land this change with the unconditional TLS setting independently of what we do in WebAssemblyLowerEmscriptenEHSjLj.cpp (i.e. no need to block on changes to that file).

[aheejin]

Then I'll just land this first and make the discussed changes as a follow-up. Thanks!

llvm/lib/CodeGen/WasmEHPrepare.cpp

for (BasicBlock &BB : F) {
     else if (isa<CleanupPadInst>(Pad))
       CleanupPads.push_back(&BB);
   }
   if (CatchPads.empty() && CleanupPads.empty())
     return false;
 
   assert(F.hasPersonalityFn() && "Personality function not found");
 
-  // __wasm_lpad_context global variable
+  // __wasm_lpad_context global variable.
+  // If the target supports TLS, make this thread-local. We can't just
+  // unconditionally make it thread-local and depend on
+  // CoalesceFeaturesAndStripAtomics to downgrade it, because stripping TLS has
+  // the side effect of disallowing the object from being linked into a
+  // shared-memory module, which we don't want to be responsible for.
   LPadContextGV = cast<GlobalVariable>(
       M.getOrInsertGlobal("__wasm_lpad_context", LPadContextTy));
+  Attribute FSAttr = F.getFnAttribute("target-features");
+  if (FSAttr.isValid()) {
+    StringRef FS = FSAttr.getValueAsString();
+    if (FS.contains("+atomics") && FS.contains("+bulk-memory"))
+      LPadContextGV->setThreadLocalMode(GlobalValue::GeneralDynamicTLSModel);
+  }
+
   LPadIndexField = IRB.CreateConstGEP2_32(LPadContextTy, LPadContextGV, 0, 0,
                                           "lpad_index_gep");
   LSDAField =
       IRB.CreateConstGEP2_32(LPadContextTy, LPadContextGV, 0, 1, "lsda_gep");
   SelectorField = IRB.CreateConstGEP2_32(LPadContextTy, LPadContextGV, 0, 2,
                                          "selector_gep");
 
   // wasm.landingpad.index() intrinsic, which is to specify landingpad index
   LPadIndexF = Intrinsic::getDeclaration(&M, Intrinsic::wasm_landingpad_index);
   // wasm.lsda() intrinsic. Returns the address of LSDA table for the current
   // function.

llvm/test/CodeGen/WebAssembly/wasmehprepare.ll

-; RUN: opt < %s -winehprepare -demote-catchswitch-only -wasmehprepare -S | FileCheck %s
+; RUN: opt < %s -winehprepare -demote-catchswitch-only -wasmehprepare -S | FileCheck %s --check-prefixes=CHECK,NO-TLS
+; RUN: opt < %s -winehprepare -demote-catchswitch-only -wasmehprepare -S --mattr=+atomics,+bulk-memory | FileCheck %s --check-prefixes=CHECK,TLS
 
 target datalayout = "e-m:e-p:32:32-i64:64-n32:64-S128"
 target triple = "wasm32-unknown-unknown"
 
-; CHECK: @__wasm_lpad_context = external global { i32, i8*, i32 }
+; NO-TLS: @__wasm_lpad_context = external global { i32, i8*, i32 }
+; TLS: @__wasm_lpad_context = external thread_local global { i32, i8*, i32 }
 
 @_ZTIi = external constant i8*
 %struct.Temp = type { i8 }
 
 ; A single 'catch (int)' clause.
 ; A wasm.catch() call, wasm.lsda() call, and personality call to generate a
 ; selector should all be genereated after the catchpad.
 ;