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 aauipc
instruction, and then a relocation pointing to a label in thatauipc
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
afonso360 requested wasmtime-compiler-reviewers for a review on PR #7003.
afonso360 requested cfallin for a review on PR #7003.
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 aauipc
instruction, and then a relocation pointing to a label in thatauipc
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
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 aauipc
instruction, and then a relocation pointing to a label in thatauipc
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
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 aauipc
instruction, and then a relocation pointing to a label in thatauipc
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
afonso360 requested alexcrichton for a review on PR #7003.
afonso360 requested wasmtime-core-reviewers for a review on PR #7003.
afonso360 updated PR #7003.
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
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
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 couldLabel(MachLabel)
be replaced withFunc(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 forMachLabelSite
). I think that should still be enough to bubble up to everyone processingMachReloc
since they should have a handle on the location of the function in the final artifact?
afonso360 submitted PR review.
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 keepMachLabel
internal, and have a separate labeling scheme inMachLabelFinalized
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.
afonso360 edited PR review comment.
afonso360 edited PR review comment.
alexcrichton submitted PR review.
alexcrichton created PR review comment:
True! My thinking is:
AddReloc
takesExternalName | MachLabel
, as you've done in this PR- The concept of a public/private label goes away
- The
name
field ofMachReloc
would becomeExternalName | Func(CodeOffset)
Then during
MachBuffer::finish
, when all labels are known, theMachBuffer::relocs
array is processed. This means thatMachBufferFinalized::relocs
would have a different type or alternatively therelocs()
method would returnimpl Iterator
which takesMachBuffer::relocs
and then performs a mapping translating aMachLabel
to aFunc(CodeOffset)
alexcrichton edited PR review comment.
afonso360 submitted PR review.
afonso360 created PR review comment:
Sounds good, I'll do that
afonso360 updated PR #7003.
afonso360 updated PR #7003.
afonso360 updated PR #7003.
alexcrichton submitted PR review:
:+1:
afonso360 merged PR #7003.
Last updated: Dec 23 2024 at 12:05 UTC