Wasmtime GitHub notifications bot (Jan 04 2021 at 06:21): Wasmtime GitHub notifications bot (Jan 04 2021 at 06:21):cfallin requested bnjbvr and julian-seward1 for a review on PR #2540.
Wasmtime GitHub notifications bot (Jan 04 2021 at 06:21):cfallin requested bnjbvr and julian-seward1 for a review on PR #2540.
Wasmtime GitHub notifications bot (Jan 04 2021 at 06:21):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.
Wasmtime GitHub notifications bot (Jan 04 2021 at 12:32):bjorn3 submitted PR Review.
Wasmtime GitHub notifications bot (Jan 04 2021 at 12:32):bjorn3 created PR Review Comment:
Why this change?
Wasmtime GitHub notifications bot (Jan 04 2021 at 12:33):bjorn3 submitted PR Review.
Wasmtime GitHub notifications bot (Jan 04 2021 at 12:33):bjorn3 created PR Review Comment:
Can you add a binemit test? The exact byte sequence is important.
Wasmtime GitHub notifications bot (Jan 04 2021 at 18:30):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.
Wasmtime GitHub notifications bot (Jan 04 2021 at 18:31):cfallin submitted PR Review.
Wasmtime GitHub notifications bot (Jan 04 2021 at 18:31):cfallin created PR Review Comment:
Ah, this (and
legalizer/tls.rs) was leftover from an earlier version of the change; reverted to original. Thanks!
Wasmtime GitHub notifications bot (Jan 04 2021 at 18:31):cfallin submitted PR Review.
Wasmtime GitHub notifications bot (Jan 04 2021 at 18:31):cfallin created PR Review Comment:
Unfortunately the
binemitfiletest backend doesn't supportMachInstyet; I'll add an issue for that.
Wasmtime GitHub notifications bot (Jan 04 2021 at 18:40):bjorn3 submitted PR Review.
Wasmtime GitHub notifications bot (Jan 04 2021 at 18:40):bjorn3 created PR Review Comment:
Can you add a
todo!()for non elf tls?
Wasmtime GitHub notifications bot (Jan 04 2021 at 18:40):bjorn3 submitted PR Review.
Wasmtime GitHub notifications bot (Jan 04 2021 at 18:47):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.
Wasmtime GitHub notifications bot (Jan 04 2021 at 18:49):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.
Wasmtime GitHub notifications bot (Jan 04 2021 at 18:50):cfallin submitted PR Review.
Wasmtime GitHub notifications bot (Jan 04 2021 at 18:50):cfallin created PR Review Comment:
Done!
Wasmtime GitHub notifications bot (Jan 04 2021 at 18:51):bjorn3 submitted PR Review.
Wasmtime GitHub notifications bot (Jan 04 2021 at 18:58):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.
Wasmtime GitHub notifications bot (Jan 04 2021 at 20:55):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.
Wasmtime GitHub notifications bot (Jan 04 2021 at 20:59):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.
Wasmtime GitHub notifications bot (Jan 06 2021 at 02:11):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.
Wasmtime GitHub notifications bot (Jan 06 2021 at 18:01):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.
Wasmtime GitHub notifications bot (Jan 12 2021 at 14:34):bnjbvr submitted PR Review.
Wasmtime GitHub notifications bot (Jan 12 2021 at 14:34):bnjbvr submitted PR Review.
Wasmtime GitHub notifications bot (Jan 12 2021 at 14:34):bnjbvr created PR Review Comment:
Would it make sense to use the return value of
ABIMachineSpec::get_regs_clobbered_by_callhere?
Wasmtime GitHub notifications bot (Jan 12 2021 at 14:34):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.
Wasmtime GitHub notifications bot (Jan 12 2021 at 15:06):bjorn3 submitted PR Review.
Wasmtime GitHub notifications bot (Jan 12 2021 at 15:06):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
Wasmtime GitHub notifications bot (Jan 14 2021 at 23:16):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.
Wasmtime GitHub notifications bot (Jan 14 2021 at 23:17):cfallin submitted PR Review.
Wasmtime GitHub notifications bot (Jan 14 2021 at 23:17):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.
Wasmtime GitHub notifications bot (Jan 14 2021 at 23:17):cfallin submitted PR Review.
Wasmtime GitHub notifications bot (Jan 14 2021 at 23:17):cfallin created PR Review Comment:
Ah yes, good idea! Done.
Wasmtime GitHub notifications bot (Jan 15 2021 at 11:34):bnjbvr submitted PR Review.
Wasmtime GitHub notifications bot (Jan 15 2021 at 11:34):bnjbvr submitted PR Review.
Wasmtime GitHub notifications bot (Jan 15 2021 at 11:34):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?
Wasmtime GitHub notifications bot (Jan 15 2021 at 13:31):bjorn3 submitted PR Review.
Wasmtime GitHub notifications bot (Jan 15 2021 at 13:31):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.
Wasmtime GitHub notifications bot (Jan 18 2021 at 06:49):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.
Wasmtime GitHub notifications bot (Jan 18 2021 at 06:49):cfallin submitted PR Review.
Wasmtime GitHub notifications bot (Jan 18 2021 at 06:49):cfallin created PR Review Comment:
Comment added!
Wasmtime GitHub notifications bot (Jan 18 2021 at 07:26):cfallin merged PR #2540.
Last updated: Nov 22 2024 at 16:03 UTC