search

rust-vmm / vm-memory

Virtual machine's guest memory crate
Apache License 2.0
305 stars 98 forks source link

CI: Run miri on each pull request #232

Closed roypat closed 1 year ago

roypat commented 1 year ago

Summary of the PR

This PR adds miri to the CI for vm-memory, to ensure that we do not regress on recently fixed memory unsoundnesses (#228, #217). It fixes some more unsoundness issues in doctests, as well as some alignments issues that arise from miri's mock allocator.

Note that this blocked on ~https://github.com/rust-vmm/rust-vmm-ci/pull/126~ https://github.com/rust-vmm/vm-memory/pull/233.

Requirements

Before submitting your PR, please make sure you addressed the following requirements:

andreeaflorescu commented 1 year ago

@roypat it looks like it in the CI, we will need to publish a new container version that also has rust-src:

$ docker run -t -i --rm --init --volume /var/lib/buildkite-agent/builds/ip-172-31-29-42-4/rust-vmm/vm-memory-ci:/workdir --workdir /workdir --label com.buildkite.job-id=018842d7-a20d-4948-8ef8-41a4dc30eac1 rustvmm/dev:v23 /bin/sh -e -c RUST_BACKTRACE=1\ MIRIFLAGS=\'-Zmiri-disable-isolation\ -Zmiri-panic-on-unsupported\ -Zmiri-backtrace=full\'\ cargo\ +nightly\ miri\ test\ --features\ backend-mmap
--
  | I will run `"rustup" "component" "add" "rust-src"` to install the `rust-src` component for the selected toolchain. Proceed? [Y/n]

Can you test it with a local container build to make sure we have all the required tools?

roypat commented 1 year ago

@roypat it looks like it in the CI, we will need to publish a new container version that also has rust-src:

$ docker run -t -i --rm --init --volume /var/lib/buildkite-agent/builds/ip-172-31-29-42-4/rust-vmm/vm-memory-ci:/workdir --workdir /workdir --label com.buildkite.job-id=018842d7-a20d-4948-8ef8-41a4dc30eac1 rustvmm/dev:v23 /bin/sh -e -c RUST_BACKTRACE=1\ MIRIFLAGS=\'-Zmiri-disable-isolation\ -Zmiri-panic-on-unsupported\ -Zmiri-backtrace=full\'\ cargo\ +nightly\ miri\ test\ --features\ backend-mmap
--
  | I will run `"rustup" "component" "add" "rust-src"` to install the `rust-src` component for the selected toolchain. Proceed? [Y/n]

Can you test it with a local container build to make sure we have all the required tools?

Yep, I noticed that too, sorry >.>

I added a temporary commit (f7805ad) that installs "rust-src" before executing miri and that makes the CI pass, so I went back to rust-vmm-container and added rust-src to it (https://github.com/rust-vmm/rust-vmm-container/pull/84)

andreeaflorescu commented 1 year ago

Yep, I noticed that too, sorry >.>

I added a temporary commit (f7805ad) that installs "rust-src" before executing miri and that makes the CI pass, so I went back to rust-vmm-container and added rust-src to it (rust-vmm/rust-vmm-container#84)

I just saw that afterwards, I approved the PR. But I see that it's currently stuck in testing.

roypat commented 1 year ago

@andreeaflorescu everything worked this time :partying_face:

alxiord commented 1 year ago

Same issue with miri missing from the container image, this time on aarch64 though.

roypat commented 1 year ago

Same issue with miri missing from the container image, this time on aarch64 though.

huh, that's bizarre... it was in there earlier :thinking: did we build another container since then?

roypat commented 1 year ago

It seems like its running version 18 of the container, but it should be running version 24 (the newest)

docker run -t -i --rm --init --volume /var/lib/buildkite-agent/builds/ip-172-31-72-62-1/rust-vmm/vm-memory-ci:/workdir --workdir /workdir --label com.buildkite.job-id=01885865-d0c8-4c71-930f-98389dd9c872 rustvmm/dev:v18 /bin/sh -e -c RUST_BACKTRACE=1\ MIRIFLAGS=\'-Zmiri-disable-isolation\ -Zmiri-panic-on-unsupported\ -Zmiri-backtrace=full\'\ cargo\ +nightly\ miri\ test\ --features\ backend-mmap

roypat commented 1 year ago

Ah, it seems like https://github.com/rust-vmm/vm-memory/pull/223 accidentally downgraded the container version - I'll fix it up