Stream: git-wasmtime

Topic: wasmtime / PR #2428 wiggle: support overlapping immutable...

Wasmtime GitHub notifications bot (Nov 18 2020 at 23:08):

pchickey opened PR #2428 from pch/wiggle_immut_borrows to main:

This PR renames the existing GuestSlice and GuestStr (which impl both Deref and DerefMut) to GuestSliceMut and GuestStrMut, and makes an immutable version of each called GuestSlice and GuestStr, which only impl Deref.

The borrow checker now distinguishes between mutable and immutable borrows, allowing overlapping immutable borrows, while still requiring mutable borrows be exclusive.

This also moves the borrow checker implementation, which was duplicated under wiggle/wasmtime and wiggle/test-helpers (and additionally over in Lucet), into its own crate. Users can still choose whether to use it to impl GuestMemory or not, but putting it in a crate gets rid of some code duplication, which I care about now that the code actually changed.

<!--

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 (Nov 18 2020 at 23:08):

pchickey requested alexcrichton for a review on PR #2428.

Wasmtime GitHub notifications bot (Nov 18 2020 at 23:08):

pchickey edited PR #2428 from pch/wiggle_immut_borrows to main:

This PR renames the existing GuestSlice and GuestStr (which impl both Deref and DerefMut) to GuestSliceMut and GuestStrMut, and makes an immutable version of each called GuestSlice and GuestStr, which only impl Deref.

The borrow checker now distinguishes between mutable and immutable borrows, allowing overlapping immutable borrows, while still requiring mutable borrows be exclusive. Tests have been updated to exercise this, and wasi-common has been ported to borrow mutable strs and slices where necessary.

This also moves the borrow checker implementation, which was duplicated under wiggle/wasmtime and wiggle/test-helpers (and additionally over in Lucet), into its own crate. Users can still choose whether to use it to impl GuestMemory or not, but putting it in a crate gets rid of some code duplication, which I care about now that the code actually changed.

<!--

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 (Nov 18 2020 at 23:26):

pchickey edited PR #2428 from pch/wiggle_immut_borrows to main:

This PR renames the existing GuestSlice and GuestStr (which impl both Deref and DerefMut) to GuestSliceMut and GuestStrMut, and makes an immutable version of each called GuestSlice and GuestStr, which only impl Deref.

The borrow checker now distinguishes between mutable and immutable borrows, allowing overlapping immutable borrows, while still requiring mutable borrows be exclusive. Tests have been updated to exercise this, and wasi-common has been ported to borrow mutable strs and slices where necessary.

This also moves the borrow checker implementation, which was duplicated under wiggle/wasmtime and wiggle/test-helpers (and additionally over in Lucet), into its own crate wiggle-borrow at wiggle/borrow. Users can still choose whether to use it to impl GuestMemory or not, but putting it in a crate gets rid of some code duplication, which I care about now that the code actually changed.

<!--

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 (Nov 19 2020 at 17:36):

alexcrichton submitted PR Review.

Wasmtime GitHub notifications bot (Nov 19 2020 at 17:36):

alexcrichton created PR Review Comment:

This looks like it's probably the same as as_slice_mut (for the most part), so could some of the internal bits be refactored into a helper called from both functions?

Wasmtime GitHub notifications bot (Nov 19 2020 at 17:36):

alexcrichton submitted PR Review.

Wasmtime GitHub notifications bot (Nov 19 2020 at 17:36):

alexcrichton created PR Review Comment:

Perhaps eliding the ! and switching to any?

Wasmtime GitHub notifications bot (Nov 19 2020 at 17:36):

alexcrichton created PR Review Comment:

(same thoughts as above for slices)

Wasmtime GitHub notifications bot (Nov 19 2020 at 17:36):

alexcrichton created PR Review Comment:

We technically statically know whether this is a shared or mutable borrow, so could there be two unborrow methods instead of one that checks both maps?

Wasmtime GitHub notifications bot (Nov 19 2020 at 17:36):

alexcrichton created PR Review Comment:

FWIW this is excluded from publication so it's fine to avoid bumping the version here.

Wasmtime GitHub notifications bot (Nov 19 2020 at 18:24):

sunfishcode submitted PR Review.

Wasmtime GitHub notifications bot (Nov 19 2020 at 18:24):

sunfishcode created PR Review Comment:

The comments for mut_borrow can now refer to GuestSliceMut and GuestStrMut. And this comment can be adapted for immut_borrow below to refer to GuestSlice and GuestStr, & access, and to drop the "at most one" language.

Wasmtime GitHub notifications bot (Nov 19 2020 at 18:24):

sunfishcode created PR Review Comment:

This unsafe could use a coment similar to above; SAFETY: iff there are no overlapping borrows it is ok to construct a &mut str.

Wasmtime GitHub notifications bot (Nov 19 2020 at 18:24):

sunfishcode created PR Review Comment:

Should this say "no overlapping mut borrows it is ok to construct a &str`?

Wasmtime GitHub notifications bot (Nov 19 2020 at 18:24):

sunfishcode submitted PR Review.

Wasmtime GitHub notifications bot (Nov 19 2020 at 23:29):

pchickey updated PR #2428 from pch/wiggle_immut_borrows to main:

This PR renames the existing GuestSlice and GuestStr (which impl both Deref and DerefMut) to GuestSliceMut and GuestStrMut, and makes an immutable version of each called GuestSlice and GuestStr, which only impl Deref.

The borrow checker now distinguishes between mutable and immutable borrows, allowing overlapping immutable borrows, while still requiring mutable borrows be exclusive. Tests have been updated to exercise this, and wasi-common has been ported to borrow mutable strs and slices where necessary.

This also moves the borrow checker implementation, which was duplicated under wiggle/wasmtime and wiggle/test-helpers (and additionally over in Lucet), into its own crate wiggle-borrow at wiggle/borrow. Users can still choose whether to use it to impl GuestMemory or not, but putting it in a crate gets rid of some code duplication, which I care about now that the code actually changed.

<!--

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 (Nov 20 2020 at 01:14):

abrown created PR Review Comment:

:+1: This is a helpful change for users who may want to reuse this script to install OpenVINO; not sure why the paths seem messed up in the CI (I usually use a simpler version of "find the directory this script lives in," FWIW). If you want, you could use Bash variables defaults for the version, ${1:-"..."}.

Wasmtime GitHub notifications bot (Nov 20 2020 at 01:14):

abrown submitted PR Review.

Wasmtime GitHub notifications bot (Nov 20 2020 at 16:11):

alexcrichton submitted PR Review.

Wasmtime GitHub notifications bot (Nov 20 2020 at 16:11):

alexcrichton created PR Review Comment:

Is it possible to debug_assert! that this did a removal?

Wasmtime GitHub notifications bot (Nov 20 2020 at 18:12):

pchickey submitted PR Review.

Wasmtime GitHub notifications bot (Nov 20 2020 at 18:12):

pchickey created PR Review Comment:

I don't know bash very well, this is just a copypaste of what stackoverflow says to do... I'll see what went wrong in CI

Wasmtime GitHub notifications bot (Nov 20 2020 at 18:13):

pchickey submitted PR Review.

Wasmtime GitHub notifications bot (Nov 20 2020 at 18:13):

pchickey created PR Review Comment:

Done!

Wasmtime GitHub notifications bot (Nov 20 2020 at 18:13):

pchickey submitted PR Review.

Wasmtime GitHub notifications bot (Nov 20 2020 at 18:13):

pchickey created PR Review Comment:

Great idea

Wasmtime GitHub notifications bot (Nov 20 2020 at 19:18):

pchickey submitted PR Review.

Wasmtime GitHub notifications bot (Nov 20 2020 at 19:18):

pchickey created PR Review Comment:

I looked into this and because of the interleaving of the mut/shared with validation theres really only opportunity to factor out the top two statements into a func, which I don't think is worth the readability hit.

Wasmtime GitHub notifications bot (Nov 20 2020 at 19:20):

pchickey updated PR #2428 from pch/wiggle_immut_borrows to main:

This PR renames the existing GuestSlice and GuestStr (which impl both Deref and DerefMut) to GuestSliceMut and GuestStrMut, and makes an immutable version of each called GuestSlice and GuestStr, which only impl Deref.

The borrow checker now distinguishes between mutable and immutable borrows, allowing overlapping immutable borrows, while still requiring mutable borrows be exclusive. Tests have been updated to exercise this, and wasi-common has been ported to borrow mutable strs and slices where necessary.

This also moves the borrow checker implementation, which was duplicated under wiggle/wasmtime and wiggle/test-helpers (and additionally over in Lucet), into its own crate wiggle-borrow at wiggle/borrow. Users can still choose whether to use it to impl GuestMemory or not, but putting it in a crate gets rid of some code duplication, which I care about now that the code actually changed.

<!--

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 (Nov 20 2020 at 19:23):

pchickey updated PR #2428 from pch/wiggle_immut_borrows to main:

This PR renames the existing GuestSlice and GuestStr (which impl both Deref and DerefMut) to GuestSliceMut and GuestStrMut, and makes an immutable version of each called GuestSlice and GuestStr, which only impl Deref.

The borrow checker now distinguishes between mutable and immutable borrows, allowing overlapping immutable borrows, while still requiring mutable borrows be exclusive. Tests have been updated to exercise this, and wasi-common has been ported to borrow mutable strs and slices where necessary.

This also moves the borrow checker implementation, which was duplicated under wiggle/wasmtime and wiggle/test-helpers (and additionally over in Lucet), into its own crate wiggle-borrow at wiggle/borrow. Users can still choose whether to use it to impl GuestMemory or not, but putting it in a crate gets rid of some code duplication, which I care about now that the code actually changed.

<!--

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 (Nov 20 2020 at 19:23):

pchickey submitted PR Review.

Wasmtime GitHub notifications bot (Nov 20 2020 at 19:23):

pchickey created PR Review Comment:

The script error was a simple typo, should be fixed now

Wasmtime GitHub notifications bot (Nov 20 2020 at 19:42):

pchickey updated PR #2428 from pch/wiggle_immut_borrows to main:

This PR renames the existing GuestSlice and GuestStr (which impl both Deref and DerefMut) to GuestSliceMut and GuestStrMut, and makes an immutable version of each called GuestSlice and GuestStr, which only impl Deref.

The borrow checker now distinguishes between mutable and immutable borrows, allowing overlapping immutable borrows, while still requiring mutable borrows be exclusive. Tests have been updated to exercise this, and wasi-common has been ported to borrow mutable strs and slices where necessary.

This also moves the borrow checker implementation, which was duplicated under wiggle/wasmtime and wiggle/test-helpers (and additionally over in Lucet), into its own crate wiggle-borrow at wiggle/borrow. Users can still choose whether to use it to impl GuestMemory or not, but putting it in a crate gets rid of some code duplication, which I care about now that the code actually changed.

<!--

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 (Nov 20 2020 at 19:46):

pchickey updated PR #2428 from pch/wiggle_immut_borrows to main:

This PR renames the existing GuestSlice and GuestStr (which impl both Deref and DerefMut) to GuestSliceMut and GuestStrMut, and makes an immutable version of each called GuestSlice and GuestStr, which only impl Deref.

The borrow checker now distinguishes between mutable and immutable borrows, allowing overlapping immutable borrows, while still requiring mutable borrows be exclusive. Tests have been updated to exercise this, and wasi-common has been ported to borrow mutable strs and slices where necessary.

This also moves the borrow checker implementation, which was duplicated under wiggle/wasmtime and wiggle/test-helpers (and additionally over in Lucet), into its own crate wiggle-borrow at wiggle/borrow. Users can still choose whether to use it to impl GuestMemory or not, but putting it in a crate gets rid of some code duplication, which I care about now that the code actually changed.

<!--

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 (Nov 20 2020 at 21:56):

pchickey updated PR #2428 from pch/wiggle_immut_borrows to main:

This PR renames the existing GuestSlice and GuestStr (which impl both Deref and DerefMut) to GuestSliceMut and GuestStrMut, and makes an immutable version of each called GuestSlice and GuestStr, which only impl Deref.

The borrow checker now distinguishes between mutable and immutable borrows, allowing overlapping immutable borrows, while still requiring mutable borrows be exclusive. Tests have been updated to exercise this, and wasi-common has been ported to borrow mutable strs and slices where necessary.

This also moves the borrow checker implementation, which was duplicated under wiggle/wasmtime and wiggle/test-helpers (and additionally over in Lucet), into its own crate wiggle-borrow at wiggle/borrow. Users can still choose whether to use it to impl GuestMemory or not, but putting it in a crate gets rid of some code duplication, which I care about now that the code actually changed.

<!--

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 (Nov 20 2020 at 23:58):

pchickey merged PR #2428.

Last updated: Nov 22 2024 at 16:03 UTC