[PATCH] D102044: [lld][WebAssembly] Disallow exporting of TLS symbols

[Sam Clegg via Phabricator via llvm-commits]

sbc100 added inline comments.


================
Comment at: lld/test/wasm/tls-export.s:9
+
+.section  .tdata.tls1,"",@
+.globl  tls1
----------------
dschuff wrote:
> What is it that actually designates the symbol as TLS? Is it just the section name?
> edit: ok yeah i just read the rest of the code. That's surprising; I guess in the object file (and eventually in dylibs) there's a flag in the symbol table, right? or is it still just based on the section name? is ELF like this too, or do they have a section or symbol flag?
Currently in wasm its solely based on magic section names.    I believe this is true for ELF too, which I did/do find surprising.  I could also be wrong.  

Symbols themselves are not ever TLS, but they can point into TLS sections.  That is my understanding.


================
Comment at: lld/test/wasm/tls-export.s:16
+
+.section  .custom_section.target_features,"",@
+  .int8 3
----------------
dschuff wrote:
> Hm, you know, If we're going to be writing a lot of these asm tests with post-MVP features, maybe we actually do want to have an assembler directive for it at some point.
Indeed, that does seem like something we will need.  (either that or some kind command line flag but I don't know if such flags for `mc` are a thing).


================
Comment at: lld/wasm/InputChunks.h:121
+  bool isTLS() {
+    return getName().startswith(".tdata") || getName().startswith(".tbss");
+  }
----------------
dschuff wrote:
> I guess there doesn't need to be a `.trodata` because if it's read-only it can always be shared across threads....
Yes as far as I know that is not a thing.


================
Comment at: lld/wasm/OutputSegment.h:35
 
+  bool isTLS() const { return name == ".tdata"; }
+
----------------
dschuff wrote:
> I guess this is asymmetric with input chunks because BSS doesn't end up in any kind of segment or descriptor in the output file (it's just implicitly-zeroed space)?
We merge tdata and tbss when we create the output segments.   I think the idea is that we want them
to share a single base address (__tls_base + offset should work for anything in any TLS section) avoiding
the need for a separate `__tls_bss_base`).

```
static StringRef getOutputDataSegmentName(StringRef name) {                      
  // We only support one thread-local segment, so we must merge the segments        
  // despite --no-merge-data-segments.                                           
  // We also need to merge .tbss into .tdata so they share the same offsets.                                      
  if (name.startswith(".tdata") || name.startswith(".tbss"))                     
    return ".tdata";           
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102044