wasmtime / PR #4546 cranelift: Add COFF TLS Support · git-wasmtime

Stream: git-wasmtime

Topic: wasmtime / PR #4546 cranelift: Add COFF TLS Support

Wasmtime GitHub notifications bot (Jul 28 2022 at 14:09):

afonso360 opened PR #4546 from x64-tls-coff to main:

:wave: Hey,

This PR is an initial draft at adding support for resolving TLS variables in COFF file formats (Windows). This is the current main blocker on having cg_clif working on windows.

There are still some pending issues in this PR:

Do we need to mark the gs register as used?

Declaring the _tls_index symbol. No idea if it is correct or not.

It is my understanding (mostly based on #1885) that we still need to add COFF TLS support to the object crate for this to become functional.

I'm going to look into modifying the object crate to see how easy/hard its going to be to add support for this, but would really appreciate any help.

Based on my tests compiling cranelift/filetests/filetests/isa/x64/tls_coff.clif we emit pretty much the equivalent machine code and relocations as rustc does.

<details>
<summary>Disassembly of rustc output:</summary>

```
0000000000000000 <getaddr>:
0: 8b 05 00 00 00 00 movl (%rip), %eax # 6 <getaddr+0x6>
0000000000000002: IMAGE_REL_AMD64_REL32 _tls_index
6: 65 48 8b 0c 25 58 00 00 00 movq %gs:88, %rcx
f: 48 8b 04 c1 movq (%rcx,%rax,8), %rax
13: 48 8d 80 00 00 00 00 leaq (%rax), %rax
0000000000000016: IMAGE_REL_AMD64_SECREL tls
1a: c3 retq
</details>

<details>
  <summary>Disassembly of tls_coff.clif output:</summary>

  ```
0000000000000000 <u0:0>:
       0: 55                            pushq   %rbp
       1: 48 89 e5                      movq    %rsp, %rbp
       4: 8b 05 00 00 00 00             movl    (%rip), %eax  # a <u0:0+0xa>
                0000000000000006:  IMAGE_REL_AMD64_REL32        _tls_index
       a: 65 48 8b 0c 25 58 00 00 00    movq    %gs:88, %rcx
      13: 48 8b 04 c1                   movq    (%rcx,%rax,8), %rax
      17: 48 8d 80 00 00 00 00          leaq    (%rax), %rax
                000000000000001a:  IMAGE_REL_AMD64_SECREL       .Ldata0
      1e: 48 89 ec                      movq    %rbp, %rsp
      21: 5d                            popq    %rbp
      22: c3                            retq
</details>

I think the only difference there is the target of the relocation, because in the tls_coff example I didn't name the symbol.

cc: @bjorn3 @cfallin
cc: #1885

Wasmtime GitHub notifications bot (Jul 28 2022 at 14:20):

bjorn3 created PR review comment:

I think this should still be inside the match. For some tls models the resulting value may not be stored in rax. For example they may be able to directly move to the output register.

Wasmtime GitHub notifications bot (Jul 28 2022 at 14:21):

bjorn3 submitted PR review.

Wasmtime GitHub notifications bot (Jul 28 2022 at 14:22):

bjorn3 edited PR review comment.

Wasmtime GitHub notifications bot (Jul 28 2022 at 14:22):

bjorn3 created PR review comment:

I did expect it to not be.

Wasmtime GitHub notifications bot (Jul 28 2022 at 14:22):

bjorn3 submitted PR review.

Wasmtime GitHub notifications bot (Jul 28 2022 at 14:23):

bjorn3 submitted PR review.

Wasmtime GitHub notifications bot (Jul 28 2022 at 14:23):

bjorn3 created PR review comment:

Maybe have a variant similar to LibCall and add a function similar to self.libcall_names to get the symbol name?

Wasmtime GitHub notifications bot (Jul 28 2022 at 14:40):

bjorn3 submitted PR review.

Wasmtime GitHub notifications bot (Jul 28 2022 at 14:40):

bjorn3 created PR review comment:

Looks like object doesn't distinguish between text, data and tls for COFF: https://github.com/gimli-rs/object/blob/404ae26dac1eafc82ecad4f02ba8ccda57f7cb27/src/write/coff.rs#L597 And I couldn't find anywhere COFF distinguishes between data and tls undefined symbols.

Wasmtime GitHub notifications bot (Jul 28 2022 at 14:42):

bjorn3 created PR review comment:

These match what I see produced by rustc I believe.

Wasmtime GitHub notifications bot (Jul 28 2022 at 14:42):

bjorn3 submitted PR review.

Wasmtime GitHub notifications bot (Jul 28 2022 at 17:41):

afonso360 updated PR #4546 from x64-tls-coff to main.

Wasmtime GitHub notifications bot (Jul 28 2022 at 17:46):

afonso360 submitted PR review.

Wasmtime GitHub notifications bot (Jul 28 2022 at 17:46):

afonso360 created PR review comment:

I've added a KnownSymbol struct. However, if we had a lookup function, wouldn't that lead to all kinds of noncompliance with the COFF/PE format?

Wasmtime GitHub notifications bot (Jul 28 2022 at 17:46):

afonso360 edited PR review comment.

Wasmtime GitHub notifications bot (Jul 28 2022 at 17:51):

afonso360 updated PR #4546 from x64-tls-coff to main.

Wasmtime GitHub notifications bot (Jul 28 2022 at 17:53):

afonso360 has marked PR #4546 as ready for review.

Wasmtime GitHub notifications bot (Jul 28 2022 at 18:24):

bjorn3 submitted PR review.

Wasmtime GitHub notifications bot (Jul 28 2022 at 18:24):

bjorn3 created PR review comment:

The lookup function would default to the correct name, but there may be cases where you want a different name. For example I plan to support tls in cg_clif's jit mode by using the TlsGd tls model and then redirecting __tls_get_addr to a function implemented in cg_clif which does the tls emulation, as libc doesn't have knowledge of the tls data objects used by the jitted code.

Wasmtime GitHub notifications bot (Jul 28 2022 at 19:45):

afonso360 updated PR #4546 from x64-tls-coff to main.

Wasmtime GitHub notifications bot (Jul 28 2022 at 19:47):

afonso360 updated PR #4546 from x64-tls-coff to main.

Wasmtime GitHub notifications bot (Aug 10 2022 at 20:25):

afonso360 updated PR #4546 from x64-tls-coff to main.

Wasmtime GitHub notifications bot (Aug 11 2022 at 16:33):

jameysharp has enabled auto merge for PR #4546.

Wasmtime GitHub notifications bot (Aug 11 2022 at 16:33):

jameysharp submitted PR review.

Wasmtime GitHub notifications bot (Aug 11 2022 at 16:33):

jameysharp merged PR #4546.

Last updated: Nov 22 2024 at 16:03 UTC