cfallin requested bnjbvr and julian-seward1 for a review on PR #2540.
cfallin requested bnjbvr and julian-seward1 for a review on PR #2540.
cfallin opened PR #2540 from x64-tls
to main
:
Depends on #2538 and includes that commit in this PR.
This follows the implementation in the legacy x86 backend, including
hardcoded sequence that is compatible with what the linker expects. We
could potentially do better here, but it is likely not necessary.Thanks to @bjorn3 for a bugfix to an earlier version of this.
bjorn3 submitted PR Review.
bjorn3 created PR Review Comment:
Why this change?
bjorn3 submitted PR Review.
bjorn3 created PR Review Comment:
Can you add a binemit test? The exact byte sequence is important.
cfallin updated PR #2540 from x64-tls
to main
:
Depends on #2538 and includes that commit in this PR.
This follows the implementation in the legacy x86 backend, including
hardcoded sequence that is compatible with what the linker expects. We
could potentially do better here, but it is likely not necessary.Thanks to @bjorn3 for a bugfix to an earlier version of this.
cfallin submitted PR Review.
cfallin created PR Review Comment:
Ah, this (and
legalizer/tls.rs
) was leftover from an earlier version of the change; reverted to original. Thanks!
cfallin submitted PR Review.
cfallin created PR Review Comment:
Unfortunately the
binemit
filetest backend doesn't supportMachInst
yet; I'll add an issue for that.
bjorn3 submitted PR Review.
bjorn3 created PR Review Comment:
Can you add a
todo!()
for non elf tls?
bjorn3 submitted PR Review.
cfallin updated PR #2540 from x64-tls
to main
:
Depends on #2538 and includes that commit in this PR.
This follows the implementation in the legacy x86 backend, including
hardcoded sequence that is compatible with what the linker expects. We
could potentially do better here, but it is likely not necessary.Thanks to @bjorn3 for a bugfix to an earlier version of this.
cfallin updated PR #2540 from x64-tls
to main
:
Depends on #2538 and includes that commit in this PR.
This follows the implementation in the legacy x86 backend, including
hardcoded sequence that is compatible with what the linker expects. We
could potentially do better here, but it is likely not necessary.Thanks to @bjorn3 for a bugfix to an earlier version of this.
cfallin submitted PR Review.
cfallin created PR Review Comment:
Done!
bjorn3 submitted PR Review.
cfallin updated PR #2540 from x64-tls
to main
:
Depends on #2538 and includes that commit in this PR.
This follows the implementation in the legacy x86 backend, including
hardcoded sequence that is compatible with what the linker expects. We
could potentially do better here, but it is likely not necessary.Thanks to @bjorn3 for a bugfix to an earlier version of this.
cfallin updated PR #2540 from x64-tls
to main
:
Depends on #2538 and includes that commit in this PR.
This follows the implementation in the legacy x86 backend, including
hardcoded sequence that is compatible with what the linker expects. We
could potentially do better here, but it is likely not necessary.Thanks to @bjorn3 for a bugfix to an earlier version of this.
cfallin updated PR #2540 from x64-tls
to main
:
Depends on #2538 and includes that commit in this PR.
This follows the implementation in the legacy x86 backend, including
hardcoded sequence that is compatible with what the linker expects. We
could potentially do better here, but it is likely not necessary.Thanks to @bjorn3 for a bugfix to an earlier version of this.
cfallin updated PR #2540 from x64-tls
to main
:
Depends on #2538 and includes that commit in this PR.
This follows the implementation in the legacy x86 backend, including
hardcoded sequence that is compatible with what the linker expects. We
could potentially do better here, but it is likely not necessary.Thanks to @bjorn3 for a bugfix to an earlier version of this.
cfallin updated PR #2540 from x64-tls
to main
:
Depends on #2538 and includes that commit in this PR.
This follows the implementation in the legacy x86 backend, including
hardcoded sequence that is compatible with what the linker expects. We
could potentially do better here, but it is likely not necessary.Thanks to @bjorn3 for a bugfix to an earlier version of this.
bnjbvr submitted PR Review.
bnjbvr submitted PR Review.
bnjbvr created PR Review Comment:
Would it make sense to use the return value of
ABIMachineSpec::get_regs_clobbered_by_call
here?
bnjbvr created PR Review Comment:
Is there something meaningful we can add as a comment, explaining what the sequence does?
Also: can it be reframed as other Vcode::Inst instructions that would be recursively generated? This would act as documentation, and help understand and maintaining this sequence a bit better.
bjorn3 submitted PR Review.
bjorn3 created PR Review Comment:
The thing is that it needs this exact byte sequence. Any tiny change will result in a linker error as the linker doesn't know how to rewrite it anymore. For the old backend I wrote a few comments that you could use here: https://github.com/bytecodealliance/wasmtime/blob/f579d088baac9df6bb852ecc19d8216fb94438a1/cranelift/codegen/meta/src/isa/x86/recipes.rs#L3356-L3377
cfallin updated PR #2540 from x64-tls
to main
:
Depends on #2538 and includes that commit in this PR.
This follows the implementation in the legacy x86 backend, including
hardcoded sequence that is compatible with what the linker expects. We
could potentially do better here, but it is likely not necessary.Thanks to @bjorn3 for a bugfix to an earlier version of this.
cfallin submitted PR Review.
cfallin created PR Review Comment:
Added comments similar to the original x86-backend ones, as well as a comment noting that the linker expects exactly these bytes.
cfallin submitted PR Review.
cfallin created PR Review Comment:
Ah yes, good idea! Done.
bnjbvr submitted PR Review.
bnjbvr submitted PR Review.
bnjbvr created PR Review Comment:
Double-check me here: we can hardcore SystemV here because this instruction is specific for Elf, right? If that's not the case, could we get the call_conv from the caller; if that's the case, can you add a comment to that effect please?
bjorn3 submitted PR Review.
bjorn3 created PR Review Comment:
Yes, this instruction is specific for ELF and must use the System-V call conv independently of the call conv of the function using TLS. Mach-O and PE use different TLS mechanisms.
cfallin updated PR #2540 from x64-tls
to main
:
Depends on #2538 and includes that commit in this PR.
This follows the implementation in the legacy x86 backend, including
hardcoded sequence that is compatible with what the linker expects. We
could potentially do better here, but it is likely not necessary.Thanks to @bjorn3 for a bugfix to an earlier version of this.
cfallin submitted PR Review.
cfallin created PR Review Comment:
Comment added!
cfallin merged PR #2540.
Last updated: Dec 23 2024 at 12:05 UTC