[PATCH] D91276: [WebAssembly] Add new relocation types for TLS data symbols

[Sam Clegg via Phabricator via llvm-commits]

sbc100 added a comment.

Ah yes, I forgot about the I32 relocation type.    I looks like we don't actually need that since you can't have thread local address in global data... great!

  # cat test.c
  _Thread_local int bar;
  int* a = &bar;
  $ gcc test.c
  test.c:2:10: error: initializer element is not constant
      2 | int* a = &bar;
        |          ^





================
Comment at: lld/test/wasm/tls.s:10
   global.get __tls_base
-  i32.const tls1
+  i32.const tls1 at TLSREL
   i32.add
----------------
tlively wrote:
> Was the previous direct addressing relocation implicit in `i32.const tls1` because it used a symbol name in place of a number literal?
Yes.  This is basically the bug I'm trying to fix here.

`i32.const tls1` is a "normal" relocation against the symbol `tls1`.   However, as you can see from the comments in the lld changes here we had special handling of "normal" relocations in the linker when the target symbol happened to be in a TLS area.   

This new mode is more explicit and also allows us to catch bugs (e.g. @tls relocation against non-tls symbols and vice versa).   


================
Comment at: lld/wasm/InputFiles.cpp:242
+    // backward compat with old object files built with `-fPIC`.
+    if (D->segment && D->segment->outputSeg->name == ".tdata")
+      return D->getOutputSegmentOffset() + reloc.Addend;
----------------
tlively wrote:
> What about `.tbss`? Are there other possible thread-local section names? Same question down in `scanRelocations` and elsewhere.
We currently combine all tls data into a single `.tdata` section here in the linker.   So there is always exactly one output TLS segment.


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyInstrInfo.cpp:246
+ArrayRef<std::pair<unsigned, const char *>>
+WebAssemblyInstrInfo::getSerializableBitmaskMachineOperandTargetFlags() const {
+  static const std::pair<unsigned, const char *> TargetFlags[] = {
----------------
tlively wrote:
> What is this function used for?
Good question :)  Maybe I should revert this part.. I noticed that other platforms implement this but we were missing it.  I'm guessing its for debugging of MIR or some thing like that? 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91276