Stream: git-wasmtime

Topic: wasmtime / PR #2276 debug: Normalise value prior to right...

Wasmtime GitHub notifications bot (Oct 07 2020 at 14:11):

ggreif edited PR #2276 from main to main:

Due to the 32-bit WASM/64-bit wasmtime VM schism the DW_OP_shr* operations end up with random bits frobbed from the upper portion of the 64-bit operand word. This fixes up to the location translator such that it inserts (something akin to) DW_OP_lit32; DW_OP_shl; DW_OP_lit32; DW_OP_shrX in front of DW_OP_shrX. The rationale is that this will clean out the upper portion, respecting 32-bit signedness.

<!--

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 (Oct 07 2020 at 14:44):

ggreif edited PR #2276 from main to main:

Due to the 32-bit WASM/64-bit wasmtime VM schism, the DW_OP_shr* operations end up with random bits frobbed from the upper portion of the 64-bit operand word. This fixes up to the location translator such that it inserts (something akin to) DW_OP_lit32; DW_OP_shl; DW_OP_lit32; DW_OP_shrX in front of DW_OP_shrX. The rationale is that this will clean out the upper portion, respecting 32-bit signedness.

<!--

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 (Oct 07 2020 at 14:47):

ggreif edited PR #2276 from main to main:

Due to the 32-bit WASM/64-bit wasmtime VM schism, the DW_OP_shr* operations end up with random bits frobbed from the upper portion of the 64-bit operand word. This PR fixes up the location translator such that it inserts (something akin to) DW_OP_lit32; DW_OP_shl; DW_OP_lit32; DW_OP_shrX in front of DW_OP_shrX. The rationale is that this will clean out the upper portion, respecting 32-bit signedness.

<!--

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 (Oct 07 2020 at 15:30):

yurydelendik submitted PR Review.

Wasmtime GitHub notifications bot (Oct 07 2020 at 15:30):

yurydelendik created PR Review Comment:

How about DW_OP_swap DW_OP_plus_uconst 32 instead of DW_OP_const1u 32 shr* DW_OP_swap?

Wasmtime GitHub notifications bot (Oct 07 2020 at 15:30):

yurydelendik submitted PR Review.

Wasmtime GitHub notifications bot (Oct 07 2020 at 15:30):

yurydelendik created PR Review Comment:

Can you expand on "normalisation" here? I recommend just include this PR description-like text here.

Wasmtime GitHub notifications bot (Oct 07 2020 at 15:30):

yurydelendik created PR Review Comment:

Do we need to include Shr here?

Wasmtime GitHub notifications bot (Oct 07 2020 at 16:03):

ggreif submitted PR Review.

Wasmtime GitHub notifications bot (Oct 07 2020 at 16:03):

ggreif created PR Review Comment:

I wanted to present it as clearly as possible in the first commit, for posterity :-)
I am preparing a commit, that adds an optimisation.

Wasmtime GitHub notifications bot (Oct 07 2020 at 16:05):

ggreif submitted PR Review.

Wasmtime GitHub notifications bot (Oct 07 2020 at 16:05):

ggreif created PR Review Comment:

Yes, I have seen dirt bits ending up in the semantic operand with lldb.

Wasmtime GitHub notifications bot (Oct 07 2020 at 16:15):

ggreif submitted PR Review.

Wasmtime GitHub notifications bot (Oct 07 2020 at 16:15):

ggreif created PR Review Comment:

Well, that may be in fact an indication that reading of WASM arguments/locals is not hygienic. :-( can you check that?

Wasmtime GitHub notifications bot (Oct 07 2020 at 16:18):

ggreif updated PR #2276 from main to main:

Due to the 32-bit WASM/64-bit wasmtime VM schism, the DW_OP_shr* operations end up with random bits frobbed from the upper portion of the 64-bit operand word. This PR fixes up the location translator such that it inserts (something akin to) DW_OP_lit32; DW_OP_shl; DW_OP_lit32; DW_OP_shrX in front of DW_OP_shrX. The rationale is that this will clean out the upper portion, respecting 32-bit signedness.

<!--

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 (Oct 07 2020 at 16:19):

ggreif updated PR #2276 from main to main:

Due to the 32-bit WASM/64-bit wasmtime VM schism, the DW_OP_shr* operations end up with random bits frobbed from the upper portion of the 64-bit operand word. This PR fixes up the location translator such that it inserts (something akin to) DW_OP_lit32; DW_OP_shl; DW_OP_lit32; DW_OP_shrX in front of DW_OP_shrX. The rationale is that this will clean out the upper portion, respecting 32-bit signedness.

<!--

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 (Oct 07 2020 at 16:20):

ggreif submitted PR Review.

Wasmtime GitHub notifications bot (Oct 07 2020 at 16:20):

ggreif created PR Review Comment:

Done: a806b96

Wasmtime GitHub notifications bot (Oct 07 2020 at 16:31):

ggreif submitted PR Review.

Wasmtime GitHub notifications bot (Oct 07 2020 at 16:31):

ggreif created PR Review Comment:

Mostly because I was not sure whether two shifts can be collapsed to a single one by the DWARF spec. But I now assume this is the case.

Wasmtime GitHub notifications bot (Oct 07 2020 at 16:46):

yurydelendik submitted PR Review.

Wasmtime GitHub notifications bot (Oct 07 2020 at 16:46):

yurydelendik created PR Review Comment:

it might be the case, though I expect DWARF expression not to know that, or the fact that register data is 16, 32 or 64 bit, and be proactive to mask only needed bits. We can keep Shr here for "symmetry".

Wasmtime GitHub notifications bot (Oct 07 2020 at 16:48):

ggreif updated PR #2276 from main to main:

Due to the 32-bit WASM/64-bit wasmtime VM schism, the DW_OP_shr* operations end up with random bits frobbed from the upper portion of the 64-bit operand word. This PR fixes up the location translator such that it inserts (something akin to) DW_OP_lit32; DW_OP_shl; DW_OP_lit32; DW_OP_shrX in front of DW_OP_shrX. The rationale is that this will clean out the upper portion, respecting 32-bit signedness.

<!--

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 (Oct 07 2020 at 16:49):

ggreif submitted PR Review.

Wasmtime GitHub notifications bot (Oct 07 2020 at 16:49):

ggreif created PR Review Comment:

This is now fe7f041.

Wasmtime GitHub notifications bot (Oct 07 2020 at 16:50):

ggreif submitted PR Review.

Wasmtime GitHub notifications bot (Oct 07 2020 at 16:50):

ggreif created PR Review Comment:

Okay, I'll keep an eye on this. If the _dirt issue_ happens to pop up elsewhere, I'll file a bug. Then we can revisit here.

Wasmtime GitHub notifications bot (Oct 07 2020 at 16:54):

ggreif submitted PR Review.

Wasmtime GitHub notifications bot (Oct 07 2020 at 16:54):

ggreif created PR Review Comment:

This normalisation is only needed for wasm32, it is not allowed for wasm64. Can we do it guardedly?

Wasmtime GitHub notifications bot (Oct 07 2020 at 16:57):

ggreif requested yurydelendik for a review on PR #2276.

Wasmtime GitHub notifications bot (Oct 07 2020 at 17:11):

yurydelendik submitted PR Review.

Wasmtime GitHub notifications bot (Oct 07 2020 at 17:11):

yurydelendik created PR Review Comment:

good question, but I think we don't have to answer it right now. we probably need to: a) start tracking values types that are coming from DW_OP_WASM_location (which can be 32 or 64-bit), b) and try to be proactive and track types and implicit bit expansion.

Wasmtime GitHub notifications bot (Oct 07 2020 at 17:42):

yurydelendik submitted PR Review.

Wasmtime GitHub notifications bot (Oct 07 2020 at 17:42):

yurydelendik merged PR #2276.

Last updated: Oct 23 2024 at 20:03 UTC