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

[Thomas Lively via Phabricator via llvm-commits]

tlively 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>(
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119803