wasmtime / PR #7003 riscv64: Implement ELF TLS GD Relocat... · git-wasmtime

Stream: git-wasmtime

Topic: wasmtime / PR #7003 riscv64: Implement ELF TLS GD Relocat...

Wasmtime GitHub notifications bot (Sep 12 2023 at 12:14):

afonso360 opened PR #7003 from afonso360:riscv-tls-2 to bytecodealliance:main:

:wave: Hey,

This PR implements ELF TLS GD relocations in the RISC-V backend. This is required to get the `rustc_codegen_cranelift``'s testsuite passing on this architecture.

The TLS GD model for RISC-V works slightly differently from the rest of the existing architectures. Usually you have 2 or 3 relocations pointing to a symbol and then a call to __tls_get_addr. RISC-V however has one relocation pointing to the symbol in a auipc instruction, and then a relocation pointing to a label in that auipc instruction, not the final symbol itself.
label:
    auipc a0,0                    # R_RISCV_TLS_GD_HI20 (symbol)
    addi  a0,a0,0                 # R_RISCV_PCREL_LO12_I (label)
    call __tls_get_addr@plt
This presents a problem as we currently don't have a way to export labels into the final binary, or a way to target labels with relocations.

Most of this PR is dealing with that issue. In the first commit I've introduced a notion of label visibility that allows a label to be marked as public, and if so, be carried all the way through to binary object emission.

Secondly is altering our allowed relocation targets. Currently we only allow external names, such as libcalls or other functions. I've altered this to allow targeting other labels within the function.

Only the final commit deals with implementing the TLS GD relocations in the RISC-V backend.

This doesn't have a great deal of tests as its quite difficult to test these kinds of binary emissions. But I've manually tested this by running cg_clif's testsuite.

CC: @bjorn3

Wasmtime GitHub notifications bot (Sep 12 2023 at 12:14):

afonso360 requested wasmtime-compiler-reviewers for a review on PR #7003.

Wasmtime GitHub notifications bot (Sep 12 2023 at 12:14):

afonso360 requested cfallin for a review on PR #7003.

Wasmtime GitHub notifications bot (Sep 12 2023 at 12:14):

afonso360 edited PR #7003:

:wave: Hey,

This PR implements ELF TLS GD relocations in the RISC-V backend. This is required to get the rustc_codegen_cranelift's testsuite passing on this architecture.

The TLS GD model for RISC-V works slightly differently from the rest of the existing architectures. Usually you have 2 or 3 relocations pointing to a symbol and then a call to __tls_get_addr. RISC-V however has one relocation pointing to the symbol in a auipc instruction, and then a relocation pointing to a label in that auipc instruction, not the final symbol itself.
label:
    auipc a0,0                    # R_RISCV_TLS_GD_HI20 (symbol)
    addi  a0,a0,0                 # R_RISCV_PCREL_LO12_I (label)
    call __tls_get_addr@plt
This presents a problem as we currently don't have a way to export labels into the final binary, or a way to target labels with relocations.

Most of this PR is dealing with that issue. In the first commit I've introduced a notion of label visibility that allows a label to be marked as public, and if so, be carried all the way through to binary object emission.

Secondly is altering our allowed relocation targets. Currently we only allow external names, such as libcalls or other functions. I've altered this to allow targeting other labels within the function.

Only the final commit deals with implementing the TLS GD relocations in the RISC-V backend.

This doesn't have a great deal of tests as its quite difficult to test these kinds of binary emissions. But I've manually tested this by running cg_clif's testsuite.

CC: @bjorn3

Wasmtime GitHub notifications bot (Sep 12 2023 at 12:15):

afonso360 edited PR #7003:

:wave: Hey,

This PR implements ELF TLS GD relocations in the RISC-V backend. This is required to get the rustc_codegen_cranelift's testsuite passing on this architecture.

The TLS GD model for RISC-V works slightly differently from the rest of the existing architectures. Usually you have 2 or 3 relocations pointing to a symbol and then a call to __tls_get_addr. RISC-V however has one relocation pointing to the symbol in a auipc instruction, and then a relocation pointing to a label in that auipc instruction, not the final symbol itself.
label:
    auipc a0,0                    # R_RISCV_TLS_GD_HI20 (symbol)
    addi  a0,a0,0                 # R_RISCV_PCREL_LO12_I (label)
    call __tls_get_addr@plt
This presents a problem as we currently don't have a way to export labels into the final binary, or a way to target labels with relocations.

Most of this PR is dealing with that issue. In the first commit I've introduced a notion of label visibility that allows a label to be marked as public, and if so, be carried all the way through to binary object emission.

Secondly is altering our allowed relocation targets. Currently we only allow external names, such as libcalls or other functions. I've altered this to allow targeting other labels within the function.

Only the final commit deals with implementing the TLS GD relocations in the RISC-V backend.

I also considered implementing TLS descriptors, but their spec isn't finalized yet (https://github.com/riscv-non-isa/riscv-elf-psabi-doc/issues/94)

This doesn't have a great deal of tests as its quite difficult to test these kinds of binary emissions. But I've manually tested this by running cg_clif's testsuite.

CC: @bjorn3

Wasmtime GitHub notifications bot (Sep 12 2023 at 12:19):

afonso360 edited PR #7003:

:wave: Hey,

This PR implements ELF TLS GD relocations in the RISC-V backend. This is required to get the rustc_codegen_cranelift's testsuite passing on this architecture.

The TLS GD model for RISC-V works slightly differently from the rest of the existing architectures. Usually you have 2 or 3 relocations pointing to a symbol and then a call to __tls_get_addr. RISC-V however has one relocation pointing to the symbol in a auipc instruction, and then a relocation pointing to a label in that auipc instruction, not the final symbol itself.
label:
    auipc a0,0                    # R_RISCV_TLS_GD_HI20 (symbol)
    addi  a0,a0,0                 # R_RISCV_PCREL_LO12_I (label)
    call __tls_get_addr@plt
This presents a problem as we currently don't have a way to export labels into the final binary, or a way to target labels with relocations.

Most of this PR is dealing with that issue. In the first commit I've introduced a notion of label visibility that allows a label to be marked as public, and if so, be carried all the way through to binary object emission.

Secondly is altering our allowed relocation targets. Currently we only allow external names, such as libcalls or other functions. I've altered this to allow targeting other labels within the function.

Only the final commit deals with implementing the TLS GD relocations in the RISC-V backend.

I also considered implementing TLS descriptors, but their spec isn't finalized yet (https://github.com/riscv-non-isa/riscv-elf-psabi-doc/issues/94)

This doesn't have a great deal of tests as its quite difficult to test these kinds of binary emissions. But I've manually tested this by running cg_clif's testsuite.

CC: @bjorn3

Wasmtime GitHub notifications bot (Sep 12 2023 at 12:50):

afonso360 requested alexcrichton for a review on PR #7003.

Wasmtime GitHub notifications bot (Sep 12 2023 at 12:50):

afonso360 requested wasmtime-core-reviewers for a review on PR #7003.

Wasmtime GitHub notifications bot (Sep 12 2023 at 12:50):

afonso360 updated PR #7003.

Wasmtime GitHub notifications bot (Sep 13 2023 at 22:50):

alexcrichton submitted PR review:

ELF bits all look good to me (in that I'm assuming you're checking correctness and otherwise can confirm they're idiomatic for Cranelift)

For the relocation/label bits I wrote out a suggestion below and am wondering if it can help simplify things a bit and keep MachLabel internal

Wasmtime GitHub notifications bot (Sep 13 2023 at 22:50):

alexcrichton submitted PR review:

ELF bits all look good to me (in that I'm assuming you're checking correctness and otherwise can confirm they're idiomatic for Cranelift)

For the relocation/label bits I wrote out a suggestion below and am wondering if it can help simplify things a bit and keep MachLabel internal

Wasmtime GitHub notifications bot (Sep 13 2023 at 22:50):

alexcrichton created PR review comment:

A possible idea for this -- previously MachLabel was "purely internal" which I felt was a nice property about the type and its usage but this is causing it to "leak" from the compilation unit for public labels. That being said, labels are all fixed by the time a function finishes, so as a possible alternative could Label(MachLabel) be replaced with Func(CodeOffset)?

That would represent how a relocation can be applied against the function itself, but it's a relative offset from the start of the function. That would avoid the need to export MachLabel and keeps it internal and avoids any need to perform lookups/translation from a label to an offset (e.g. no need for MachLabelSite). I think that should still be enough to bubble up to everyone processing MachReloc since they should have a handle on the location of the function in the final artifact?

Wasmtime GitHub notifications bot (Sep 14 2023 at 16:54):

afonso360 submitted PR review.

Wasmtime GitHub notifications bot (Sep 14 2023 at 16:54):

afonso360 created PR review comment:

I don't think we can do that, since in the RISC-V backend we have to create a relocation to this label while the function is still being lowered, and if we just saved the offset it's possible that it could change later right? (I think this is the case, not 100% sure).

I think we could maybe keep RelocTarget as is, but rename all the labels that are going to be published while finalizing. That way we keep MachLabel internal, and have a separate labeling scheme in MachLabelFinalized that is public. I think that could work, but it also means that we would have to:

Renumber published labels (there shouldn't be many)

Walk through all relocations and update them if they point to a label.

We can do this only if we have at least 1 published label, so we don't slow down the common case.

Wasmtime GitHub notifications bot (Sep 14 2023 at 16:54):

afonso360 edited PR review comment.

Wasmtime GitHub notifications bot (Sep 14 2023 at 16:57):

afonso360 edited PR review comment.

Wasmtime GitHub notifications bot (Sep 14 2023 at 19:40):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Sep 14 2023 at 19:40):

alexcrichton created PR review comment:

True! My thinking is:

AddReloc takes ExternalName | MachLabel, as you've done in this PR

The concept of a public/private label goes away

The name field of MachReloc would become ExternalName | Func(CodeOffset)

Then during MachBuffer::finish, when all labels are known, the MachBuffer::relocs array is processed. This means that MachBufferFinalized::relocs would have a different type or alternatively the relocs() method would return impl Iterator which takes MachBuffer::relocs and then performs a mapping translating a MachLabel to a Func(CodeOffset)

Wasmtime GitHub notifications bot (Sep 14 2023 at 19:40):

alexcrichton edited PR review comment.

Wasmtime GitHub notifications bot (Sep 15 2023 at 09:32):

afonso360 submitted PR review.

Wasmtime GitHub notifications bot (Sep 15 2023 at 09:32):

afonso360 created PR review comment:

Sounds good, I'll do that

Wasmtime GitHub notifications bot (Sep 16 2023 at 14:49):

afonso360 updated PR #7003.

Wasmtime GitHub notifications bot (Sep 17 2023 at 18:49):

afonso360 updated PR #7003.

Wasmtime GitHub notifications bot (Sep 17 2023 at 19:37):

afonso360 updated PR #7003.

Wasmtime GitHub notifications bot (Sep 18 2023 at 14:08):

alexcrichton submitted PR review:

:+1:

Wasmtime GitHub notifications bot (Sep 18 2023 at 15:25):

afonso360 merged PR #7003.

Last updated: Nov 22 2024 at 17:03 UTC