Wasmtime GitHub notifications bot (Jul 29 2020 at 21:31): Wasmtime GitHub notifications bot (Jul 29 2020 at 21:31):cfallin opened PR #2081 from aarch64-baldrdash-fix to main:
In the Baldrdash (SpiderMonkey) embedding, we must take care to
zero-extend all function arguments to callees in integer registers when
the types are narrower than 64 bits. This is because, unlike the native
SysV ABI, the Baldrdash ABI expects high bits to be cleared. Not doing
so leads to difficult-to-trace errors where high bits falsely tag an
int32 as e.g. an object pointer, leading to potential security issues.<!--
Please ensure that the following steps are all taken care of before submitting
the PR.
[ ] This has been discussed in issue #..., or if not, please tell us why
here.[ ] A short description of what this does, why it is needed; if the
description becomes long, the matter should probably be discussed in an issue
first.[ ] This PR contains test cases, if meaningful.
- [ ] A reviewer from the core maintainer team has been assigned for this PR.
If you don't know who could review this, please indicate so. The list of
suggested reviewers on the right can help you.Please ensure all communication adheres to the code of conduct.
-->
Wasmtime GitHub notifications bot (Jul 29 2020 at 21:31):cfallin requested bnjbvr and julian-seward1 for a review on PR #2081.
Wasmtime GitHub notifications bot (Jul 29 2020 at 21:31):cfallin requested bnjbvr and julian-seward1 for a review on PR #2081.
Wasmtime GitHub notifications bot (Jul 30 2020 at 13:19):bnjbvr submitted PR Review.
Wasmtime GitHub notifications bot (Jul 30 2020 at 20:10):cfallin updated PR #2081 from aarch64-baldrdash-fix to main:
In the Baldrdash (SpiderMonkey) embedding, we must take care to
zero-extend all function arguments to callees in integer registers when
the types are narrower than 64 bits. This is because, unlike the native
SysV ABI, the Baldrdash ABI expects high bits to be cleared. Not doing
so leads to difficult-to-trace errors where high bits falsely tag an
int32 as e.g. an object pointer, leading to potential security issues.<!--
Please ensure that the following steps are all taken care of before submitting
the PR.
[ ] This has been discussed in issue #..., or if not, please tell us why
here.[ ] A short description of what this does, why it is needed; if the
description becomes long, the matter should probably be discussed in an issue
first.[ ] This PR contains test cases, if meaningful.
- [ ] A reviewer from the core maintainer team has been assigned for this PR.
If you don't know who could review this, please indicate so. The list of
suggested reviewers on the right can help you.Please ensure all communication adheres to the code of conduct.
-->
Wasmtime GitHub notifications bot (Jul 31 2020 at 12:11):julian-seward1 submitted PR Review.
Wasmtime GitHub notifications bot (Jul 31 2020 at 12:11):julian-seward1 created PR Review Comment:
Presumably (correct me if I'm wrong) this is correct but a bit suboptimal, in that we're extending a value that we read from the stack in a previous insn; and in the ideal case we could have just done a suitable widening load? Is that a correct understanding? I'm not saying this should be fixed, but just checking I have this right.
Wasmtime GitHub notifications bot (Jul 31 2020 at 12:13):julian-seward1 submitted PR Review.
Wasmtime GitHub notifications bot (Jul 31 2020 at 12:13):julian-seward1 created PR Review Comment:
I'm not 100% sure what I'm looking at here, but I was a bit mystified in that I can't see any signed widening insns in the expected outputs. But it looks like the patch above handles both signed and unsigned widening. If a signed-widening version of this test case does make sense, please can you add it?
Wasmtime GitHub notifications bot (Jul 31 2020 at 12:14):julian-seward1 submitted PR Review.
Wasmtime GitHub notifications bot (Jul 31 2020 at 12:14):julian-seward1 created PR Review Comment:
And ditto observation for the x64 equivalent of this, AFAICS.
Wasmtime GitHub notifications bot (Jul 31 2020 at 12:14):julian-seward1 submitted PR Review.
Wasmtime GitHub notifications bot (Jul 31 2020 at 12:42):bnjbvr submitted PR Review.
Wasmtime GitHub notifications bot (Jul 31 2020 at 12:42):bnjbvr created PR Review Comment:
+1; this only tests uext, not sext, which would be nice to have tests for!
Wasmtime GitHub notifications bot (Jul 31 2020 at 12:42):bnjbvr submitted PR Review.
Wasmtime GitHub notifications bot (Jul 31 2020 at 16:57):cfallin updated PR #2081 from aarch64-baldrdash-fix to main:
In the Baldrdash (SpiderMonkey) embedding, we must take care to
zero-extend all function arguments to callees in integer registers when
the types are narrower than 64 bits. This is because, unlike the native
SysV ABI, the Baldrdash ABI expects high bits to be cleared. Not doing
so leads to difficult-to-trace errors where high bits falsely tag an
int32 as e.g. an object pointer, leading to potential security issues.<!--
Please ensure that the following steps are all taken care of before submitting
the PR.
[ ] This has been discussed in issue #..., or if not, please tell us why
here.[ ] A short description of what this does, why it is needed; if the
description becomes long, the matter should probably be discussed in an issue
first.[ ] This PR contains test cases, if meaningful.
- [ ] A reviewer from the core maintainer team has been assigned for this PR.
If you don't know who could review this, please indicate so. The list of
suggested reviewers on the right can help you.Please ensure all communication adheres to the code of conduct.
-->
Wasmtime GitHub notifications bot (Jul 31 2020 at 16:58):cfallin submitted PR Review.
Wasmtime GitHub notifications bot (Jul 31 2020 at 16:58):cfallin created PR Review Comment:
Added
sexttests as well; thanks!
Wasmtime GitHub notifications bot (Jul 31 2020 at 17:00):cfallin updated PR #2081 from aarch64-baldrdash-fix to main:
In the Baldrdash (SpiderMonkey) embedding, we must take care to
zero-extend all function arguments to callees in integer registers when
the types are narrower than 64 bits. This is because, unlike the native
SysV ABI, the Baldrdash ABI expects high bits to be cleared. Not doing
so leads to difficult-to-trace errors where high bits falsely tag an
int32 as e.g. an object pointer, leading to potential security issues.<!--
Please ensure that the following steps are all taken care of before submitting
the PR.
[ ] This has been discussed in issue #..., or if not, please tell us why
here.[ ] A short description of what this does, why it is needed; if the
description becomes long, the matter should probably be discussed in an issue
first.[ ] This PR contains test cases, if meaningful.
- [ ] A reviewer from the core maintainer team has been assigned for this PR.
If you don't know who could review this, please indicate so. The list of
suggested reviewers on the right can help you.Please ensure all communication adheres to the code of conduct.
-->
Wasmtime GitHub notifications bot (Jul 31 2020 at 17:02):cfallin submitted PR Review.
Wasmtime GitHub notifications bot (Jul 31 2020 at 17:02):cfallin created PR Review Comment:
Other way around -- we're storing a value in a register to the stack; sadly there's no store instruction (as far as I know) that can sign/zero-extend the bottom N bits of a GPR into 64 bits and then store that 64 bits, all in one go.
Wasmtime GitHub notifications bot (Jul 31 2020 at 17:19):cfallin updated PR #2081 from aarch64-baldrdash-fix to main:
In the Baldrdash (SpiderMonkey) embedding, we must take care to
zero-extend all function arguments to callees in integer registers when
the types are narrower than 64 bits. This is because, unlike the native
SysV ABI, the Baldrdash ABI expects high bits to be cleared. Not doing
so leads to difficult-to-trace errors where high bits falsely tag an
int32 as e.g. an object pointer, leading to potential security issues.<!--
Please ensure that the following steps are all taken care of before submitting
the PR.
[ ] This has been discussed in issue #..., or if not, please tell us why
here.[ ] A short description of what this does, why it is needed; if the
description becomes long, the matter should probably be discussed in an issue
first.[ ] This PR contains test cases, if meaningful.
- [ ] A reviewer from the core maintainer team has been assigned for this PR.
If you don't know who could review this, please indicate so. The list of
suggested reviewers on the right can help you.Please ensure all communication adheres to the code of conduct.
-->
Wasmtime GitHub notifications bot (Jul 31 2020 at 19:13):cfallin merged PR #2081.
Last updated: Oct 23 2024 at 20:03 UTC