Stream: git-wasmtime

Topic: wasmtime / PR #4790 x64: Ensure that constants are always...

Wasmtime GitHub notifications bot (Aug 26 2022 at 18:57):

elliottt opened PR #4790 from trevor/xmm-mem-alignment-bug to main:

Ensure that constants generated for the memory case of XmmMem values are always 16 bytes, ensuring that we don't accidantally perform an unaligned load.

Fixes #4761
<!--

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 (Aug 26 2022 at 19:01):

elliottt has marked PR #4790 as ready for review.

Wasmtime GitHub notifications bot (Aug 26 2022 at 19:02):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Aug 26 2022 at 19:02):

cfallin created PR review comment:

Can we delegate to put_in_xmm_mem at this point to share logic (then lift the XmmMem back to XmmMemImm)?

Wasmtime GitHub notifications bot (Aug 26 2022 at 19:02):

cfallin created PR review comment:

We can probably use a u128 here to avoid the awkward iter-chaining below? Then we can do a zero-extend cast when calling this rather than giving 0 as upper.

Wasmtime GitHub notifications bot (Aug 26 2022 at 19:02):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Aug 26 2022 at 19:06):

jameysharp submitted PR review.

Wasmtime GitHub notifications bot (Aug 26 2022 at 19:06):

jameysharp submitted PR review.

Wasmtime GitHub notifications bot (Aug 26 2022 at 19:06):

jameysharp created PR review comment:

Absolutely unimportant nit: I like to use copied instead of cloned when I'm expecting it to be cheap, so the compiler can tell me if I've tried to do it to a type that isn't Copy.

Wasmtime GitHub notifications bot (Aug 26 2022 at 19:06):

jameysharp created PR review comment:

Should this comment say "put_in_reg_mem_imm"?

Wasmtime GitHub notifications bot (Aug 26 2022 at 19:06):

jameysharp created PR review comment:

Should we just run this on all architectures as well as the interpreter? The bug was x64-specific but the test should work everywhere and it would be nice to know if it didn't.

Wasmtime GitHub notifications bot (Aug 26 2022 at 19:10):

elliottt updated PR #4790 from trevor/xmm-mem-alignment-bug to main.

Wasmtime GitHub notifications bot (Aug 26 2022 at 19:10):

elliottt requested cfallin for a review on PR #4790.

Wasmtime GitHub notifications bot (Aug 26 2022 at 19:11):

elliottt updated PR #4790 from trevor/xmm-mem-alignment-bug to main.

Wasmtime GitHub notifications bot (Aug 26 2022 at 19:12):

elliottt created PR review comment:

Fixed this by delegating to a different function here :)

Wasmtime GitHub notifications bot (Aug 26 2022 at 19:12):

elliottt submitted PR review.

Wasmtime GitHub notifications bot (Aug 26 2022 at 19:16):

jameysharp created PR review comment:

Oh, after Chris' suggestion, now you can simplify this to value.to_le_bytes().into(), I think.

Wasmtime GitHub notifications bot (Aug 26 2022 at 19:16):

jameysharp submitted PR review.

Wasmtime GitHub notifications bot (Aug 26 2022 at 19:19):

jameysharp submitted PR review.

Wasmtime GitHub notifications bot (Aug 26 2022 at 19:19):

jameysharp created PR review comment:

I guess ConstantData doesn't have a From implementation for arrays, but .as_slice().into() ought to work at least.

Wasmtime GitHub notifications bot (Aug 26 2022 at 19:20):

jameysharp submitted PR review.

Wasmtime GitHub notifications bot (Aug 26 2022 at 19:20):

jameysharp created PR review comment:

Yeah, delegating to a different function worked out very nicely!

Wasmtime GitHub notifications bot (Aug 26 2022 at 19:23):

elliottt submitted PR review.

Wasmtime GitHub notifications bot (Aug 26 2022 at 19:23):

elliottt created PR review comment:

Looks like we're stuck with my verbose monstrosity:

809 |             let data = VCodeConstantData::Generated(value.to_le_bytes().iter().into());
    |                                                                                ^^^^ the trait `From<core::slice::Iter<'_, u8>>` is not implemented for `ConstantData`

Wasmtime GitHub notifications bot (Aug 26 2022 at 19:24):

jameysharp created PR review comment:

The problem there is the .iter().

Wasmtime GitHub notifications bot (Aug 26 2022 at 19:24):

jameysharp submitted PR review.

Wasmtime GitHub notifications bot (Aug 26 2022 at 19:26):

elliottt submitted PR review.

Wasmtime GitHub notifications bot (Aug 26 2022 at 19:26):

elliottt created PR review comment:

Yeah, I get a similar error without it. I had been experimenting :)

809 |             let data = VCodeConstantData::Generated(value.to_le_bytes().into());
    |                                                                         ^^^^ the trait `From<[u8; 16]>` is not implemented for `ConstantData`

Wasmtime GitHub notifications bot (Aug 26 2022 at 19:28):

jameysharp submitted PR review.

Wasmtime GitHub notifications bot (Aug 26 2022 at 19:28):

jameysharp created PR review comment:

I'd figured out that just .into() wouldn't work after I posted my first comment. Did you try my second suggestion, of .as_slice().into()?

Wasmtime GitHub notifications bot (Aug 26 2022 at 19:29):

elliottt updated PR #4790 from trevor/xmm-mem-alignment-bug to main.

Wasmtime GitHub notifications bot (Aug 26 2022 at 19:30):

elliottt submitted PR review.

Wasmtime GitHub notifications bot (Aug 26 2022 at 19:30):

elliottt created PR review comment:

Ah! Totally missed your second comment, that works :+1:

Wasmtime GitHub notifications bot (Aug 26 2022 at 19:33):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Aug 26 2022 at 19:33):

jameysharp submitted PR review.

Wasmtime GitHub notifications bot (Aug 26 2022 at 19:33):

jameysharp created PR review comment:

Hooray! It'd probably be nice if ConstantData actually had a From<[T; n]> implementation too, but I didn't want to suggest doing that in this PR. :sweat_smile:

Wasmtime GitHub notifications bot (Aug 26 2022 at 19:49):

elliottt has enabled auto merge for PR #4790.

Wasmtime GitHub notifications bot (Aug 26 2022 at 20:04):

elliottt merged PR #4790.

Wasmtime GitHub notifications bot (Aug 27 2022 at 06:32):

afonso360 submitted PR review.

Wasmtime GitHub notifications bot (Aug 27 2022 at 06:32):

afonso360 created PR review comment:

As a further suggestion, we can also remove the set enable_llvm_abi_extensions since we no longer have i128's in this new minimized test case.

Last updated: Nov 22 2024 at 16:03 UTC