Stream: git-wasmtime

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


view this post on Zulip 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.

Please ensure all communication adheres to the code of conduct.
-->

view this post on Zulip 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.

Please ensure all communication adheres to the code of conduct.
-->

view this post on Zulip 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.

Please ensure all communication adheres to the code of conduct.
-->

view this post on Zulip Wasmtime GitHub notifications bot (Oct 07 2020 at 15:30):

yurydelendik submitted PR Review.

view this post on Zulip 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?

view this post on Zulip Wasmtime GitHub notifications bot (Oct 07 2020 at 15:30):

yurydelendik submitted PR Review.

view this post on Zulip 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.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 07 2020 at 15:30):

yurydelendik created PR Review Comment:

Do we need to include Shr here?

view this post on Zulip Wasmtime GitHub notifications bot (Oct 07 2020 at 16:03):

ggreif submitted PR Review.

view this post on Zulip 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.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 07 2020 at 16:05):

ggreif submitted PR Review.

view this post on Zulip 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.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 07 2020 at 16:15):

ggreif submitted PR Review.

view this post on Zulip 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?

view this post on Zulip 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.

Please ensure all communication adheres to the code of conduct.
-->

view this post on Zulip 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.

Please ensure all communication adheres to the code of conduct.
-->

view this post on Zulip Wasmtime GitHub notifications bot (Oct 07 2020 at 16:20):

ggreif submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 07 2020 at 16:20):

ggreif created PR Review Comment:

Done: a806b96

view this post on Zulip Wasmtime GitHub notifications bot (Oct 07 2020 at 16:31):

ggreif submitted PR Review.

view this post on Zulip 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.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 07 2020 at 16:46):

yurydelendik submitted PR Review.

view this post on Zulip 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".

view this post on Zulip 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.

Please ensure all communication adheres to the code of conduct.
-->

view this post on Zulip Wasmtime GitHub notifications bot (Oct 07 2020 at 16:49):

ggreif submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 07 2020 at 16:49):

ggreif created PR Review Comment:

This is now fe7f041.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 07 2020 at 16:50):

ggreif submitted PR Review.

view this post on Zulip 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.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 07 2020 at 16:54):

ggreif submitted PR Review.

view this post on Zulip 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?

view this post on Zulip Wasmtime GitHub notifications bot (Oct 07 2020 at 16:57):

ggreif requested yurydelendik for a review on PR #2276.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 07 2020 at 17:11):

yurydelendik submitted PR Review.

view this post on Zulip 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.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 07 2020 at 17:42):

yurydelendik submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 07 2020 at 17:42):

yurydelendik merged PR #2276.


Last updated: Dec 23 2024 at 12:05 UTC