Stream: git-wasmtime

Topic: wasmtime / PR #9620 Add HostAlignedByteCount to enforce a...

view this post on Zulip Wasmtime GitHub notifications bot (Nov 19 2024 at 02:29):

sunshowers requested wasmtime-core-reviewers for a review on PR #9620.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 19 2024 at 02:29):

sunshowers requested alexcrichton for a review on PR #9620.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 19 2024 at 02:29):

sunshowers opened PR #9620 from sunshowers:host-aligned to bytecodealliance:main:

As part of the work to allow mmaps to be backed by other implementations (#9544), I realized that we didn't have any way to track whether a particular usize is host-page-aligned at compile time.

Add a HostAlignedByteCount which tracks that a particular usize is aligned to the host page size. This also does not expose safe unchecked arithmetic operations, to ensure that overflows always error out.

With HostAlignedByteCount, a lot of runtime checks can go away thanks to the type-level assertion.

In the interest of keeping the diff relatively small, I haven't converted everything over yet. More code can be converted over as we go along.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 19 2024 at 02:30):

sunshowers submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 19 2024 at 02:30):

sunshowers created PR review comment:

This code and the similar section below would be good to convert over, but I decided to do this in a followup.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 19 2024 at 02:32):

sunshowers submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 19 2024 at 02:32):

sunshowers created PR review comment:

I first named this AlignedByteCount, but then realized that the VM page size might not be the same as the host page size, so named it HostAlignedByteCount instead. Happy to rename this to something else if it makes sense.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 19 2024 at 02:38):

sunshowers updated PR #9620.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 19 2024 at 02:41):

sunshowers updated PR #9620.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 19 2024 at 02:47):

sunshowers submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 19 2024 at 02:47):

sunshowers created PR review comment:

Should this print the size in hex or as a decimal?

view this post on Zulip Wasmtime GitHub notifications bot (Nov 19 2024 at 02:49):

sunshowers edited PR #9620:

As part of the work to allow mmaps to be backed by other implementations (#9544), I realized that we didn't have any way to track whether a particular usize is host-page-aligned at compile time.

Add a HostAlignedByteCount which tracks that a particular usize is aligned to the host page size. This also does not expose safe unchecked arithmetic operations, to ensure that overflows are always handled.

With HostAlignedByteCount, a lot of runtime checks can go away thanks to the type-level assertion.

In the interest of keeping the diff relatively small, I haven't converted everything over yet. More code can be converted over as we go along.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 19 2024 at 06:57):

sunshowers commented on PR #9620:

I started working on a followup as well: https://github.com/bytecodealliance/wasmtime/commit/03efe42fb4d8fdd760d945ce6ceb95b8ca338974

Unfortunately GitHub's review flow does not let you manage stacked PRs unless you have push access to the destination repo, so I'll have to put these up one at a time. Sigh :(

view this post on Zulip Wasmtime GitHub notifications bot (Nov 19 2024 at 16:09):

alexcrichton submitted PR review:

Thanks for this!

view this post on Zulip Wasmtime GitHub notifications bot (Nov 19 2024 at 16:09):

alexcrichton created PR review comment:

For here I think it's probably ok to discard the precise values that were being operated on as otherwise that makes the errors here pretty large for a low-level type such as this.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 19 2024 at 16:09):

alexcrichton created PR review comment:

Mind putting this type/impls/errors/etc in a dedicated submodule of the vm module?

view this post on Zulip Wasmtime GitHub notifications bot (Nov 19 2024 at 16:09):

alexcrichton created PR review comment:

Same seems reasonable to me :+1:

view this post on Zulip Wasmtime GitHub notifications bot (Nov 19 2024 at 16:09):

alexcrichton created PR review comment:

Let's go with hex perhaps to clearly show that the low bits are all zero

view this post on Zulip Wasmtime GitHub notifications bot (Nov 19 2024 at 18:35):

alexcrichton submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 19 2024 at 18:35):

alexcrichton created PR review comment:

Reviewing some other code elsewhere, I think that the from_file constructor may cause problems here because file lengths aren't always page-aligned.

To help catch this mind adding a debug_assert! to new_unchecked? That I think should make the test suite panic

view this post on Zulip Wasmtime GitHub notifications bot (Nov 19 2024 at 19:03):

alexcrichton commented on PR #9620:

As a heads up I'm adding https://github.com/bytecodealliance/wasmtime/pull/9614 to the merge queue which is going to have a number of conflicts with this. It should (hopefully) be just a few #[cfg(feature = "signals-based-traps")] in a few places but if anything is overly difficult or onerous lemme know and I can help out

view this post on Zulip Wasmtime GitHub notifications bot (Nov 20 2024 at 21:49):

sunshowers updated PR #9620.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 20 2024 at 21:50):

sunshowers submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 20 2024 at 21:50):

sunshowers created PR review comment:

Done.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 20 2024 at 21:51):

sunshowers submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 20 2024 at 21:51):

sunshowers created PR review comment:

This ended up becoming #9639.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 20 2024 at 21:58):

sunshowers created PR review comment:

Hmm good point, done.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 20 2024 at 21:58):

sunshowers submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 20 2024 at 22:47):

sunshowers updated PR #9620.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 20 2024 at 22:49):

sunshowers commented on PR #9620:

(Apologies for the force push -- had to do it as part of the rebase. GH makes me cry)

view this post on Zulip Wasmtime GitHub notifications bot (Nov 21 2024 at 00:42):

alexcrichton submitted PR review:

Thanks again for this!

view this post on Zulip Wasmtime GitHub notifications bot (Nov 21 2024 at 04:19):

sunshowers updated PR #9620.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 21 2024 at 04:19):

sunshowers commented on PR #9620:

All right, think I fixed the Miri issue.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 21 2024 at 16:05):

alexcrichton submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 21 2024 at 16:05):

alexcrichton created PR review comment:

I think this may be the source of the bug, albeit it's a bit indirect. Previously the > condition is now morally becoming >= since new_accessible == self.accessible() didn't run this block before but it's now running this block with difference == 0. I think then it may be the case that mprotect for 0 bytes is succeeding on unix but failing on Windows.

On Linux I see mprotect(0x740d36001000, 0, PROT_READ|PROT_WRITE) = 0 in strace with this patch when running the failing spec test and while I haven't executed this on Windows this is what I would suspect is going on.

Another option is to update the Mmap type to skip make_accessible when the byte size is zero which I think would also be reasonable.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 21 2024 at 23:03):

sunshowers submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 21 2024 at 23:03):

sunshowers created PR review comment:

Oh that's terrifying -- I'm going to fix this (probably by making make_accessible ignore zero-sized regions) and go through this code again to make sure I didn't miss any other sign changes. Thanks for diagnosing this!

view this post on Zulip Wasmtime GitHub notifications bot (Nov 22 2024 at 00:00):

sunshowers updated PR #9620.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 22 2024 at 00:04):

sunshowers created PR review comment:

Made make_accessible a no-op on zero-length allocations, as well as the other mprotect calls (make_executable and make_readonly). They also had this as a lurking bug because they had assertions that range.start <= range.end, not range.start < range.end.

I've also added tests for zero-length calls, and re-reviewed everything -- didn't see any other changes.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 22 2024 at 00:04):

sunshowers submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 22 2024 at 00:05):

sunshowers edited PR review comment.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 22 2024 at 00:39):

alexcrichton merged PR #9620.

Last updated: Dec 23 2024 at 12:05 UTC