[PATCH] D119803: [WebAssembly] Make __wasm_lpad_context thread-local

Tue Feb 15 19:17:37 PST 2022

aheejin added inline comments.

================
Comment at: llvm/lib/CodeGen/WasmEHPrepare.cpp:220
+  // 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>(
----------------
tlively wrote:
> aheejin wrote:
> > aheejin wrote:
> > > tlively wrote:
> > > > sbc100 wrote:
> > > > > tlively wrote:
> > > > > > aheejin wrote:
> > > > > > > sbc100 wrote:
> > > > > > > > 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?
> > > > > > > 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.
> > > > > > 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.
> > > > > 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?
> > > > 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.
> > > @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 [[ https://github.com/llvm/llvm-project/blob/ea0676f97d734196b15da7553cd407e6a36cef2d/llvm/lib/Target/WebAssembly/WebAssemblyLowerEmscriptenEHSjLj.cpp#L938-L939 | 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.
> > @tlively I don't think there's difference between TLS uses of `__THREW__`/`__threwValue` and `__wasm_lpad_context`.
> > 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
@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?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119803