[PATCH] D64537: [WebAssembly] Implement thread-local storage (local-exec model)

[Sam Clegg via Phabricator via llvm-commits]

sbc100 added a comment.

I wonder if `__tls_base` should be allocated by the embedder (or by the parent/creator thread).   Then it could be an *immutable* global import that is allocated up front.   I guess `__stack_pointer` should be treated in the same way (except mutable).

I don't want to block this change on this, but just putting the idea out there.



================
Comment at: lld/test/wasm/data-layout.ll:43
+; CHECK-NEXT:       - Index:           3
+; CHECK-NEXT:         Type:            I32
 ; CHECK-NEXT:         Mutable:         false
----------------
quantum wrote:
> tlively wrote:
> > These globals don't have enough information to tell the reader what they even are, and they don't have anything to do with the data layout, so how about skipping these in the test with a comment saying what is being skipped?
> Going to skip over `__tls_base` and `__tls_size`.
Is there any reason these symbols need to exist in non-threaded builds.  The cost of two extra globals is small but not nothing.

I'd also rather not skip over them here.  If TLS changes I'd like to see this test need updating.


================
Comment at: lld/test/wasm/data-segment-merging.ll:32
+; MERGE-NEXT:      - Index:           1
+; MERGE-NEXT:        Name:            __wasm_init_tls
 ; MERGE-NOT:       - Index:
----------------
Again, can we avoid creating this completely for non-threaded builds?


================
Comment at: lld/test/wasm/tls.ll:57
+;   memory.init 0, 0
+;   end
+
----------------
quantum wrote:
> tlively wrote:
> > quantum wrote:
> > > aheejin wrote:
> > > > Hmm, I think there might be a way to actually print disassembly results. There are '*.test' files in test directory, in which we have some examples. For example, [[ https://github.com/llvm/llvm-project/blob/master/lld/test/wasm/export-table.test | this test ]] has a sequence of commands you want to run, and you can put input files in a separate directory. That test uses `obj2yaml`, but can we possibly use `llvm-objdump` or something to disassemble?
> > > We already know that we can do something like
> > > 
> > >     Run: obj2yaml %t.wasm | sed -n '/Body:/{s/^\s*Body:\s*//;s/../0x& /gp}'  | llvm-mc -disassemble -triple=wasm32
> > > 
> > > to compare the actual assembly. As for `llvm-objdump`, it seems to be unable to disassemble the WASM properly:
> > > 
> > > 
> > > ```
> > > .../tools/lld/test/wasm/Output/tls.ll.tmp.wasm:	file format WASM
> > > 
> > > 
> > > Disassembly of section CODE:
> > > 
> > > 00000000 CODE:
> > >         # 4 functions in section.
> > >        1: 02 00                        	block   	invalid_type
> > >        3: 0b                           	end
> > >        4: 10 00                        	call	0
> > >        6: 20 00                        	local.get	0
> > >        8: 24 01                        	global.set	1
> > >        a: 20 00                        	local.get	0
> > >        c: 41 00                        	i32.const	0
> > >        e: 41 08                        	i32.const	8
> > >       10: fc 08 00 00                  	memory.init	0, 0
> > >       14: 0b                           	end
> > >       15: 0f                           	return
> > >       16: 00                           	llvm-objdump: lib/Target/WebAssembly/WebAssemblyGenAsmWriter.inc:2032: void llvm::WebAssemblyInstPrinter::printInstruction(const llvm::MCInst *, llvm::raw_ostream &): Assertion `Bits != 0 && "Cannot print this instruction."' failed.
> > > 
> > > ```
> > It might be worth filing an LLVM bug for this (or possibly fixing in a separate CL).
> Going to fix this at some point in the future.
This is known issue with the disassembler in that it doesn't know where the functions start and stop in executable output, only in object files.


================
Comment at: lld/wasm/Writer.cpp:629
+  // Merge .tbss into .tdata so that they share the same offsets.
+  if (name.startswith(".tbss."))
+    return ".tdata";
----------------
Maybe write this as a single conditional so its clear even without this comment?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64537