From 4f3a9881da89361358247ba58574b9eee37df4aa Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Sat, 29 Oct 2022 11:30:07 +0200 Subject: [PATCH 01/14] teach ./miri how to do Josh syncs --- src/tools/miri/CONTRIBUTING.md | 23 +++---- src/tools/miri/miri | 114 +++++++++++++++++++++++---------- 2 files changed, 91 insertions(+), 46 deletions(-) diff --git a/src/tools/miri/CONTRIBUTING.md b/src/tools/miri/CONTRIBUTING.md index b1e6b9c69d39..b18083849cf6 100644 --- a/src/tools/miri/CONTRIBUTING.md +++ b/src/tools/miri/CONTRIBUTING.md @@ -290,14 +290,12 @@ cargo run --release -p josh-proxy -- --local=$(pwd)/local --remote=https://githu ### Importing changes from the rustc repo +Josh needs to be running, as described above. We assume we start on an up-to-date master branch in the Miri repo. ```sh -# Fetch rustc side of the history. Takes ca 5 min the first time. -# Do NOT change that commit ID, it needs to be exactly this! -git fetch http://localhost:8000/rust-lang/rust.git:at_commit=75dd959a3a40eb5b4574f8d2e23aa6efbeb33573[:prefix=src/tools/miri]:/src/tools/miri.git master -# Include that history into ours. -git merge FETCH_HEAD -m "merge rustc history" +# Fetch and merge rustc side of the history. Takes ca 5 min the first time. +./miri rustc-pull # Update toolchain reference and apply formatting. ./rustup-toolchain HEAD && ./miri fmt git commit -am "rustup" @@ -310,16 +308,15 @@ needed. ### Exporting changes to the rustc repo -We will use the josh proxy to push to your fork of rustc. You need to make sure -that the master branch of your fork is up-to-date. Also make sure that there -exists no branch called `miri` in your fork. Then run the following in the Miri -repo, assuming we are on an up-to-date master branch: +Josh needs to be running, as described above. We will use the josh proxy to push +to your fork of rustc. Run the following in the Miri repo, assuming we are on an +up-to-date master branch: ```sh # Push the Miri changes to your rustc fork (substitute your github handle for YOUR_NAME). -# Do NOT change that commit ID, it needs to be exactly this! -git push http://localhost:8000/YOUR_NAME/rust.git:at_commit=75dd959a3a40eb5b4574f8d2e23aa6efbeb33573[:prefix=src/tools/miri]:/src/tools/miri.git -o base=master HEAD:miri +./miri rustc-push YOUR_NAME miri ``` -This will create a new branch in your fork, and the output should include a link -to create a rustc PR that will integrate those changes into the main repository. +This will create a new branch called 'miri' in your fork, and the output should +include a link to create a rustc PR that will integrate those changes into the +main repository. diff --git a/src/tools/miri/miri b/src/tools/miri/miri index e492308a62eb..662f2b8e6317 100755 --- a/src/tools/miri/miri +++ b/src/tools/miri/miri @@ -42,6 +42,15 @@ many different seeds. Runs the benchmarks from bench-cargo-miri in hyperfine. hyperfine needs to be installed. can explicitly list the benchmarks to run; by default, all of them are run. +./miri rustc-pull: +Pull and merge Miri changes from the rustc repo. + +./miri rustc-push : +Push Miri changes back to the rustc repo. This will update the 'master' branch +in the Rust fork of the given user to upstream. It will also pull a copy of the +rustc history into the Miri repo, unless you set the RUSTC_GIT env var to an +existing clone of the rustc repo. + ENVIRONMENT VARIABLES MIRI_SYSROOT: @@ -52,37 +61,60 @@ Pass extra flags to all cargo invocations. (Ignored by `./miri cargo`.) EOF ) -## We need to know where we are. +## We need to know which command to run and some global constants. +COMMAND="$1" +if [ -z "$COMMAND" ]; then + echo "$USAGE" + exit 1 +fi +shift # macOS does not have a useful readlink/realpath so we have to use Python instead... MIRIDIR=$(python3 -c 'import os, sys; print(os.path.dirname(os.path.realpath(sys.argv[1])))' "$0") +# Used for rustc syncs. +JOSH_FILTER=":at_commit=75dd959a3a40eb5b4574f8d2e23aa6efbeb33573[:prefix=src/tools/miri]:/src/tools/miri" -## Run the auto-things. -if [ -z "$MIRI_AUTO_OPS" ]; then - export MIRI_AUTO_OPS=42 - - # Run this first, so that the toolchain doesn't change after - # other code has run. - if [ -f "$MIRIDIR/.auto-everything" ] || [ -f "$MIRIDIR/.auto-toolchain" ] ; then - (cd "$MIRIDIR" && ./rustup-toolchain) - fi - - if [ -f "$MIRIDIR/.auto-everything" ] || [ -f "$MIRIDIR/.auto-fmt" ] ; then - $0 fmt - fi - - if [ -f "$MIRIDIR/.auto-everything" ] || [ -f "$MIRIDIR/.auto-clippy" ] ; then - $0 clippy -- -D warnings - fi -fi - -## Determine command and toolchain. -COMMAND="$1" -[ $# -gt 0 ] && shift -# Doing this *after* auto-toolchain logic above, since that might change the toolchain. -TOOLCHAIN=$(cd "$MIRIDIR"; rustup show active-toolchain | head -n 1 | cut -d ' ' -f 1) - -## Handle some commands early, since they should *not* alter the environment. +## Early commands, that don't do auto-things and don't want the environment-altering things happening below. case "$COMMAND" in +rustc-pull) + cd "$MIRIDIR" + git fetch http://localhost:8000/rust-lang/rust.git$JOSH_FILTER.git master + git merge FETCH_HEAD + exit 0 + ;; +rustc-push) + USER="$1" + BRANCH="$2" + if [ -z "$USER" ] || [ -z "$BRANCH" ]; then + echo "Usage: $0 rustc-push " + exit 1 + fi + if [ -n "$RUSTC_GIT" ]; then + # Use an existing fork for the branch updates. + cd "$RUSTC_GIT" + else + # Do this in the local Miri repo. + echo "This will pull a copy of the rust-lang/rust history into this Miri checkout, growing it by about 1GB." + read -r -p "To avoid that, abort now and set the RUSTC_GIT environment variable to an existing rustc checkout. Proceed? [y/N] " + if [[ ! $REPLY =~ ^[Yy]$ ]]; then + exit 1 + fi + cd "$MIRIDIR" + fi + # Prepare the branches. For reliable pushing we need to push to a non-existent branch + # and set `-o base` to a branch that holds current rustc master. + echo "Preparing $USER/rust..." + if git fetch https://github.com/$USER/rust $BRANCH &>/dev/null; then + echo "The '$BRANCH' seems to already exist in $USER/rust. Please delete it and try again." + exit 1 + fi + git fetch https://github.com/rust-lang/rust master + git push https://github.com/$USER/rust FETCH_HEAD:master + # Do the actual push. + cd "$MIRIDIR" + echo "Pushing Miri changes..." + git push http://localhost:8000/$USER/rust.git$JOSH_FILTER.git HEAD:$BRANCH -o base=master + exit 0 + ;; many-seeds) for SEED in $({ echo obase=16; seq 0 255; } | bc); do echo "Trying seed: $SEED" @@ -106,9 +138,28 @@ bench) ;; esac +## Run the auto-things. +if [ -z "$MIRI_AUTO_OPS" ]; then + export MIRI_AUTO_OPS=42 + + # Run this first, so that the toolchain doesn't change after + # other code has run. + if [ -f "$MIRIDIR/.auto-everything" ] || [ -f "$MIRIDIR/.auto-toolchain" ] ; then + (cd "$MIRIDIR" && ./rustup-toolchain) + fi + + if [ -f "$MIRIDIR/.auto-everything" ] || [ -f "$MIRIDIR/.auto-fmt" ] ; then + $0 fmt + fi + + if [ -f "$MIRIDIR/.auto-everything" ] || [ -f "$MIRIDIR/.auto-clippy" ] ; then + $0 clippy -- -D warnings + fi +fi + ## Prepare the environment # Determine some toolchain properties -# export the target so its available in miri +TOOLCHAIN=$(cd "$MIRIDIR"; rustup show active-toolchain | head -n 1 | cut -d ' ' -f 1) TARGET=$(rustc +$TOOLCHAIN --version --verbose | grep "^host:" | cut -d ' ' -f 2) SYSROOT=$(rustc +$TOOLCHAIN --print sysroot) LIBDIR=$SYSROOT/lib/rustlib/$TARGET/lib @@ -227,10 +278,7 @@ cargo) $CARGO "$@" ;; *) - if [ -n "$COMMAND" ]; then - echo "Unknown command: $COMMAND" - echo - fi - echo "$USAGE" + echo "Unknown command: $COMMAND" exit 1 + ;; esac From 39598e46f6777e0f5eee8177c347fec431d2b591 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Sat, 29 Oct 2022 12:17:04 +0200 Subject: [PATCH 02/14] merge rustup-toolchain into ./miri --- src/tools/miri/.github/workflows/ci.yml | 6 +-- src/tools/miri/.gitpod.yml | 4 +- src/tools/miri/CONTRIBUTING.md | 10 ++--- src/tools/miri/README.md | 6 +-- src/tools/miri/miri | 45 ++++++++++++++++++++- src/tools/miri/rust-version | 2 +- src/tools/miri/rustup-toolchain | 53 ------------------------- 7 files changed, 58 insertions(+), 68 deletions(-) delete mode 100755 src/tools/miri/rustup-toolchain diff --git a/src/tools/miri/.github/workflows/ci.yml b/src/tools/miri/.github/workflows/ci.yml index 3efb2d733d42..607ffe0cc59f 100644 --- a/src/tools/miri/.github/workflows/ci.yml +++ b/src/tools/miri/.github/workflows/ci.yml @@ -67,9 +67,9 @@ jobs: shell: bash run: | if [[ ${{ github.event_name }} == 'schedule' ]]; then - ./rustup-toolchain HEAD --host ${{ matrix.host_target }} + ./miri toolchain HEAD --host ${{ matrix.host_target }} else - ./rustup-toolchain "" --host ${{ matrix.host_target }} + ./miri toolchain "" --host ${{ matrix.host_target }} fi - name: Show Rust version @@ -118,7 +118,7 @@ jobs: - name: Install "master" toolchain shell: bash run: | - ./rustup-toolchain "" -c clippy + ./miri toolchain - name: Show Rust version run: | diff --git a/src/tools/miri/.gitpod.yml b/src/tools/miri/.gitpod.yml index 36bd991740a8..724cf26df2b9 100644 --- a/src/tools/miri/.gitpod.yml +++ b/src/tools/miri/.gitpod.yml @@ -4,6 +4,6 @@ tasks: - before: echo "..." init: | cargo install rustup-toolchain-install-master - ./rustup-toolchain + ./miri toolchain ./miri build - command: echo "Run tests with ./miri test" \ No newline at end of file + command: echo "Run tests with ./miri test" diff --git a/src/tools/miri/CONTRIBUTING.md b/src/tools/miri/CONTRIBUTING.md index b18083849cf6..3e36c2ea5f97 100644 --- a/src/tools/miri/CONTRIBUTING.md +++ b/src/tools/miri/CONTRIBUTING.md @@ -23,13 +23,13 @@ tested against. Other versions will likely not work. After installing [`rustup-toolchain-install-master`], you can run the following command to install that exact version of rustc as a toolchain: ``` -./rustup-toolchain +./miri toolchain ``` This will set up a rustup toolchain called `miri` and set it as an override for the current directory. You can also create a `.auto-everything` file (contents don't matter, can be empty), which -will cause any `./miri` command to automatically call `rustup-toolchain`, `clippy` and `rustfmt` +will cause any `./miri` command to automatically call `./miri toolchain`, `clippy` and `rustfmt` for you. If you don't want all of these to happen, you can add individual `.auto-toolchain`, `.auto-clippy` and `.auto-fmt` files respectively. @@ -132,7 +132,7 @@ development version of Miri using and then you can use it as if it was installed by `rustup`. Make sure you use the same toolchain when calling `cargo miri` that you used when installing Miri! Usually this means you have to write `cargo +miri miri ...` to select the `miri` -toolchain that was installed by `./rustup-toolchain`. +toolchain that was installed by `./miri toolchain`. There's a test for the cargo wrapper in the `test-cargo-miri` directory; run `./run-test.py` in there to execute it. Like `./miri test`, this respects the @@ -214,7 +214,7 @@ for changes in rustc. In both cases, `rustc-version` needs updating. To update the `rustc-version` file and install the latest rustc, you can run: ``` -./rustup-toolchain HEAD +./miri toolchain HEAD ``` Now edit Miri until `./miri test` passes, and submit a PR. Generally, it is @@ -297,7 +297,7 @@ We assume we start on an up-to-date master branch in the Miri repo. # Fetch and merge rustc side of the history. Takes ca 5 min the first time. ./miri rustc-pull # Update toolchain reference and apply formatting. -./rustup-toolchain HEAD && ./miri fmt +./miri toolchain HEAD && ./miri fmt git commit -am "rustup" ``` diff --git a/src/tools/miri/README.md b/src/tools/miri/README.md index 5803a88c0e75..1b84d188c55d 100644 --- a/src/tools/miri/README.md +++ b/src/tools/miri/README.md @@ -419,9 +419,9 @@ Some native rustc `-Z` flags are also very relevant for Miri: Moreover, Miri recognizes some environment variables: -* `MIRI_AUTO_OPS` indicates whether the automatic execution of rustfmt, clippy and rustup-toolchain - should be skipped. If it is set to any value, they are skipped. This is used for avoiding - infinite recursion in `./miri` and to allow automated IDE actions to avoid the auto ops. +* `MIRI_AUTO_OPS` indicates whether the automatic execution of rustfmt, clippy and toolchain setup + should be skipped. If it is set to any value, they are skipped. This is used for avoiding infinite + recursion in `./miri` and to allow automated IDE actions to avoid the auto ops. * `MIRI_LOG`, `MIRI_BACKTRACE` control logging and backtrace printing during Miri executions, also [see "Testing the Miri driver" in `CONTRIBUTING.md`][testing-miri]. * `MIRIFLAGS` (recognized by `cargo miri` and the test suite) defines extra diff --git a/src/tools/miri/miri b/src/tools/miri/miri index 662f2b8e6317..52902410a6b2 100755 --- a/src/tools/miri/miri +++ b/src/tools/miri/miri @@ -51,6 +51,13 @@ in the Rust fork of the given user to upstream. It will also pull a copy of the rustc history into the Miri repo, unless you set the RUSTC_GIT env var to an existing clone of the rustc repo. +./miri toolchain : +Update and activate the rustup toolchain 'miri'. If no commit is given, updates +to the commit given in the `rust-version` file. If the commit is `HEAD`, updates +to the latest upstream rustc commit. +`rustup-toolchain-install-master` must be installed for this to work. Any extra +flags are passed to `rustup-toolchain-install-master`. + ENVIRONMENT VARIABLES MIRI_SYSROOT: @@ -75,6 +82,42 @@ JOSH_FILTER=":at_commit=75dd959a3a40eb5b4574f8d2e23aa6efbeb33573[:prefix=src/too ## Early commands, that don't do auto-things and don't want the environment-altering things happening below. case "$COMMAND" in +toolchain) + cd "$MIRIDIR" + # Make sure rustup-toolchain-install-master is installed. + if ! which rustup-toolchain-install-master >/dev/null; then + echo "Please install rustup-toolchain-install-master by running 'cargo install rustup-toolchain-install-master'" + exit 1 + fi + # Determine new commit. + if [[ "$1" == "" ]]; then + NEW_COMMIT=$(cat rust-version) + elif [[ "$1" == "HEAD" ]]; then + NEW_COMMIT=$(git ls-remote https://github.com/rust-lang/rust/ HEAD | cut -f 1) + else + NEW_COMMIT="$1" + fi + echo "$NEW_COMMIT" > rust-version + shift || true # don't fail if shifting fails because no commit was given + # Check if we already are at that commit. + CUR_COMMIT=$(rustc +miri --version -v 2>/dev/null | grep "^commit-hash: " | cut -d " " -f 2) + if [[ "$CUR_COMMIT" == "$NEW_COMMIT" ]]; then + echo "miri toolchain is already at commit $CUR_COMMIT." + rustup override set miri + exit 0 + fi + # Install and setup new toolchain. + rustup toolchain uninstall miri + rustup-toolchain-install-master -n miri -c cargo -c rust-src -c rustc-dev -c llvm-tools -c rustfmt -c clippy "$@" -- "$NEW_COMMIT" + rustup override set miri + # Cleanup. + cargo clean + # Call 'cargo metadata' on the sources in case that changes the lockfile + # (which fails under some setups when it is done from inside vscode). + cargo metadata --format-version 1 --manifest-path "$(rustc --print sysroot)/lib/rustlib/rustc-src/rust/compiler/rustc/Cargo.toml" >/dev/null + # Done! + exit 0 + ;; rustc-pull) cd "$MIRIDIR" git fetch http://localhost:8000/rust-lang/rust.git$JOSH_FILTER.git master @@ -145,7 +188,7 @@ if [ -z "$MIRI_AUTO_OPS" ]; then # Run this first, so that the toolchain doesn't change after # other code has run. if [ -f "$MIRIDIR/.auto-everything" ] || [ -f "$MIRIDIR/.auto-toolchain" ] ; then - (cd "$MIRIDIR" && ./rustup-toolchain) + $0 toolchain fi if [ -f "$MIRIDIR/.auto-everything" ] || [ -f "$MIRIDIR/.auto-fmt" ] ; then diff --git a/src/tools/miri/rust-version b/src/tools/miri/rust-version index d0e98a8b0dba..e582bf35356d 100644 --- a/src/tools/miri/rust-version +++ b/src/tools/miri/rust-version @@ -1 +1 @@ -85d089b41e2a0c0f07ab34f6c5a7c451389f25e6 +607878d069267e1402ad792c9331b426e4c6d0f9 diff --git a/src/tools/miri/rustup-toolchain b/src/tools/miri/rustup-toolchain deleted file mode 100755 index d7730f2b06d3..000000000000 --- a/src/tools/miri/rustup-toolchain +++ /dev/null @@ -1,53 +0,0 @@ -#!/bin/bash -set -e -# Manages a rustup toolchain called "miri". -# -# All commands set "miri" as the override toolchain for the current directory, -# and make the `rust-version` file match that toolchain. -# -# USAGE: -# -# ./rustup-toolchain: Update "miri" toolchain to match `rust-version` (the known-good version for this commit). -# -# ./rustup-toolchain HEAD: Update "miri" toolchain and `rust-version` file to latest rustc HEAD. -# -# ./rustup-toolchain $COMMIT: Update "miri" toolchain and `rust-version` file to match that commit. -# -# Any extra parameters are passed to `rustup-toolchain-install-master`. - -# Make sure rustup-toolchain-install-master is installed. -if ! which rustup-toolchain-install-master >/dev/null; then - echo "Please install rustup-toolchain-install-master by running 'cargo install rustup-toolchain-install-master'" - exit 1 -fi - -# Determine new commit. -if [[ "$1" == "" ]]; then - NEW_COMMIT=$(cat rust-version) -elif [[ "$1" == "HEAD" ]]; then - NEW_COMMIT=$(git ls-remote https://github.com/rust-lang/rust/ HEAD | cut -f 1) -else - NEW_COMMIT="$1" -fi -echo "$NEW_COMMIT" > rust-version -shift || true # don't fail if shifting fails - -# Check if we already are at that commit. -CUR_COMMIT=$(rustc +miri --version -v 2>/dev/null | grep "^commit-hash: " | cut -d " " -f 2) -if [[ "$CUR_COMMIT" == "$NEW_COMMIT" ]]; then - echo "miri toolchain is already at commit $CUR_COMMIT." - rustup override set miri - exit 0 -fi - -# Install and setup new toolchain. -rustup toolchain uninstall miri -rustup-toolchain-install-master -n miri -c cargo -c rust-src -c rustc-dev -c llvm-tools -c rustfmt -c clippy "$@" -- "$NEW_COMMIT" -rustup override set miri - -# Cleanup. -cargo clean - -# Call 'cargo metadata' on the sources in case that changes the lockfile -# (which fails under some setups when it is done from inside vscode). -cargo metadata --format-version 1 --manifest-path "$(rustc --print sysroot)/lib/rustlib/rustc-src/rust/compiler/rustc/Cargo.toml" >/dev/null From 2a3a53b1514a5e87d4992175274d2599e63c0734 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Sat, 29 Oct 2022 12:36:31 +0200 Subject: [PATCH 03/14] explain how to go back to rustup-managed Miri --- src/tools/miri/CONTRIBUTING.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/tools/miri/CONTRIBUTING.md b/src/tools/miri/CONTRIBUTING.md index b1e6b9c69d39..a2c86c3a8b23 100644 --- a/src/tools/miri/CONTRIBUTING.md +++ b/src/tools/miri/CONTRIBUTING.md @@ -138,6 +138,9 @@ There's a test for the cargo wrapper in the `test-cargo-miri` directory; run `./run-test.py` in there to execute it. Like `./miri test`, this respects the `MIRI_TEST_TARGET` environment variable to execute the test for another target. +Note that installing Miri like this will "take away" Miri management from `rustup`. +If you want to later go back to a rustup-installed Miri, run `rustup update`. + ### Using a modified standard library Miri re-builds the standard library into a custom sysroot, so it is fairly easy From 1470e9924460b299ce597ef9001e5e3f7549a8b8 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Sat, 29 Oct 2022 16:11:03 +0200 Subject: [PATCH 04/14] Stacked Borrows: make scalar field retagging the default --- src/tools/miri/README.md | 7 ++++--- src/tools/miri/src/eval.rs | 2 +- .../stacked_borrows/newtype_pair_retagging.rs | 1 - .../fail/stacked_borrows/newtype_retagging.rs | 1 - .../return_invalid_mut_option.rs | 5 ++--- .../return_invalid_mut_option.stderr | 19 ++++++++++++------- .../return_invalid_mut_tuple.rs | 5 ++--- .../return_invalid_mut_tuple.stderr | 13 +++++++++---- .../return_invalid_shr_option.rs | 5 ++--- .../return_invalid_shr_option.stderr | 19 ++++++++++++------- .../return_invalid_shr_tuple.rs | 5 ++--- .../return_invalid_shr_tuple.stderr | 13 +++++++++---- .../stacked-borrows/no_field_retagging.rs | 19 +++++++++++++++++++ .../stacked-borrows/stack-printing.stdout | 4 ++-- 14 files changed, 76 insertions(+), 42 deletions(-) create mode 100644 src/tools/miri/tests/pass/stacked-borrows/no_field_retagging.rs diff --git a/src/tools/miri/README.md b/src/tools/miri/README.md index 1b84d188c55d..7b684d5df137 100644 --- a/src/tools/miri/README.md +++ b/src/tools/miri/README.md @@ -374,14 +374,15 @@ to Miri failing to detect cases of undefined behavior in a program. application instead of raising an error within the context of Miri (and halting execution). Note that code might not expect these operations to ever panic, so this flag can lead to strange (mis)behavior. -* `-Zmiri-retag-fields` changes Stacked Borrows retagging to recurse into fields. +* `-Zmiri-retag-fields` changes Stacked Borrows retagging to recurse into *all* fields. This means that references in fields of structs/enums/tuples/arrays/... are retagged, and in particular, they are protected when passed as function arguments. + (The default is to recurse only in cases where rustc would actually emit a `noalias` attribute.) * `-Zmiri-retag-fields=` controls when Stacked Borrows retagging recurses into fields. `all` means it always recurses (like `-Zmiri-retag-fields`), `none` means it never - recurses (the default), `scalar` means it only recurses for types where we would also emit + recurses, `scalar` (the default) means it only recurses for types where we would also emit `noalias` annotations in the generated LLVM IR (types passed as indivudal scalars or pairs of - scalars). + scalars). Setting this to `none` is **unsound**. * `-Zmiri-tag-gc=` configures how often the pointer tag garbage collector runs. The default is to search for and remove unreachable tags once every `10000` basic blocks. Setting this to `0` disables the garbage collector, which causes some programs to have explosive memory usage diff --git a/src/tools/miri/src/eval.rs b/src/tools/miri/src/eval.rs index a3fc343f8b67..81132db94cf1 100644 --- a/src/tools/miri/src/eval.rs +++ b/src/tools/miri/src/eval.rs @@ -163,7 +163,7 @@ impl Default for MiriConfig { mute_stdout_stderr: false, preemption_rate: 0.01, // 1% report_progress: None, - retag_fields: RetagFields::No, + retag_fields: RetagFields::OnlyScalar, external_so_file: None, gc_interval: 10_000, num_cpus: 1, diff --git a/src/tools/miri/tests/fail/stacked_borrows/newtype_pair_retagging.rs b/src/tools/miri/tests/fail/stacked_borrows/newtype_pair_retagging.rs index 5cefdb08e787..cc774500a3c6 100644 --- a/src/tools/miri/tests/fail/stacked_borrows/newtype_pair_retagging.rs +++ b/src/tools/miri/tests/fail/stacked_borrows/newtype_pair_retagging.rs @@ -1,4 +1,3 @@ -//@compile-flags: -Zmiri-retag-fields=scalar //@error-pattern: which is protected struct Newtype<'a>(&'a mut i32, i32); diff --git a/src/tools/miri/tests/fail/stacked_borrows/newtype_retagging.rs b/src/tools/miri/tests/fail/stacked_borrows/newtype_retagging.rs index bc3883575c33..1aa6e240e30f 100644 --- a/src/tools/miri/tests/fail/stacked_borrows/newtype_retagging.rs +++ b/src/tools/miri/tests/fail/stacked_borrows/newtype_retagging.rs @@ -1,4 +1,3 @@ -//@compile-flags: -Zmiri-retag-fields=scalar //@error-pattern: which is protected struct Newtype<'a>(&'a mut i32); diff --git a/src/tools/miri/tests/fail/stacked_borrows/return_invalid_mut_option.rs b/src/tools/miri/tests/fail/stacked_borrows/return_invalid_mut_option.rs index 7fa9cf77d44b..5a9dc6afba8d 100644 --- a/src/tools/miri/tests/fail/stacked_borrows/return_invalid_mut_option.rs +++ b/src/tools/miri/tests/fail/stacked_borrows/return_invalid_mut_option.rs @@ -1,16 +1,15 @@ // Make sure that we cannot return a `&mut` that got already invalidated, not even in an `Option`. -// Due to shallow reborrowing, the error only surfaces when we look into the `Option`. fn foo(x: &mut (i32, i32)) -> Option<&mut i32> { let xraw = x as *mut (i32, i32); let ret = unsafe { &mut (*xraw).1 }; // let-bind to avoid 2phase let ret = Some(ret); let _val = unsafe { *xraw }; // invalidate xref - ret + ret //~ ERROR: /retag .* tag does not exist in the borrow stack/ } fn main() { match foo(&mut (1, 2)) { - Some(_x) => {} //~ ERROR: /retag .* tag does not exist in the borrow stack/ + Some(_x) => {} None => {} } } diff --git a/src/tools/miri/tests/fail/stacked_borrows/return_invalid_mut_option.stderr b/src/tools/miri/tests/fail/stacked_borrows/return_invalid_mut_option.stderr index 1068c286c62f..c0ff35ebcde3 100644 --- a/src/tools/miri/tests/fail/stacked_borrows/return_invalid_mut_option.stderr +++ b/src/tools/miri/tests/fail/stacked_borrows/return_invalid_mut_option.stderr @@ -1,11 +1,11 @@ error: Undefined Behavior: trying to retag from for Unique permission at ALLOC[0x4], but that tag does not exist in the borrow stack for this location --> $DIR/return_invalid_mut_option.rs:LL:CC | -LL | Some(_x) => {} - | ^^ - | | - | trying to retag from for Unique permission at ALLOC[0x4], but that tag does not exist in the borrow stack for this location - | this error occurs as part of retag at ALLOC[0x4..0x8] +LL | ret + | ^^^ + | | + | trying to retag from for Unique permission at ALLOC[0x4], but that tag does not exist in the borrow stack for this location + | this error occurs as part of retag at ALLOC[0x4..0x8] | = help: this indicates a potential bug in the program: it performed an invalid operation, but the Stacked Borrows rules it violated are still experimental = help: see https://github.com/rust-lang/unsafe-code-guidelines/blob/master/wip/stacked-borrows.md for further information @@ -13,14 +13,19 @@ help: was created by a Unique retag at offsets [0x4..0x8] --> $DIR/return_invalid_mut_option.rs:LL:CC | LL | let ret = Some(ret); - | ^^^ + | ^^^^^^^^^ help: was later invalidated at offsets [0x0..0x8] by a read access --> $DIR/return_invalid_mut_option.rs:LL:CC | LL | let _val = unsafe { *xraw }; // invalidate xref | ^^^^^ = note: BACKTRACE: - = note: inside `main` at $DIR/return_invalid_mut_option.rs:LL:CC + = note: inside `foo` at $DIR/return_invalid_mut_option.rs:LL:CC +note: inside `main` at $DIR/return_invalid_mut_option.rs:LL:CC + --> $DIR/return_invalid_mut_option.rs:LL:CC + | +LL | match foo(&mut (1, 2)) { + | ^^^^^^^^^^^^^^^^ note: some details are omitted, run with `MIRIFLAGS=-Zmiri-backtrace=full` for a verbose backtrace diff --git a/src/tools/miri/tests/fail/stacked_borrows/return_invalid_mut_tuple.rs b/src/tools/miri/tests/fail/stacked_borrows/return_invalid_mut_tuple.rs index c94fef90542f..8fe7f15cab0c 100644 --- a/src/tools/miri/tests/fail/stacked_borrows/return_invalid_mut_tuple.rs +++ b/src/tools/miri/tests/fail/stacked_borrows/return_invalid_mut_tuple.rs @@ -1,12 +1,11 @@ // Make sure that we cannot return a `&mut` that got already invalidated, not even in a tuple. -// Due to shallow reborrowing, the error only surfaces when we look into the tuple. fn foo(x: &mut (i32, i32)) -> (&mut i32,) { let xraw = x as *mut (i32, i32); let ret = (unsafe { &mut (*xraw).1 },); let _val = unsafe { *xraw }; // invalidate xref - ret + ret //~ ERROR: /retag .* tag does not exist in the borrow stack/ } fn main() { - foo(&mut (1, 2)).0; //~ ERROR: /retag .* tag does not exist in the borrow stack/ + foo(&mut (1, 2)).0; } diff --git a/src/tools/miri/tests/fail/stacked_borrows/return_invalid_mut_tuple.stderr b/src/tools/miri/tests/fail/stacked_borrows/return_invalid_mut_tuple.stderr index 79de9b668cf2..9abf43c29f08 100644 --- a/src/tools/miri/tests/fail/stacked_borrows/return_invalid_mut_tuple.stderr +++ b/src/tools/miri/tests/fail/stacked_borrows/return_invalid_mut_tuple.stderr @@ -1,8 +1,8 @@ error: Undefined Behavior: trying to retag from for Unique permission at ALLOC[0x4], but that tag does not exist in the borrow stack for this location --> $DIR/return_invalid_mut_tuple.rs:LL:CC | -LL | foo(&mut (1, 2)).0; - | ^^^^^^^^^^^^^^^^^^ +LL | ret + | ^^^ | | | trying to retag from for Unique permission at ALLOC[0x4], but that tag does not exist in the borrow stack for this location | this error occurs as part of retag at ALLOC[0x4..0x8] @@ -13,14 +13,19 @@ help: was created by a Unique retag at offsets [0x4..0x8] --> $DIR/return_invalid_mut_tuple.rs:LL:CC | LL | let ret = (unsafe { &mut (*xraw).1 },); - | ^^^^^^^^^^^^^^ + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: was later invalidated at offsets [0x0..0x8] by a read access --> $DIR/return_invalid_mut_tuple.rs:LL:CC | LL | let _val = unsafe { *xraw }; // invalidate xref | ^^^^^ = note: BACKTRACE: - = note: inside `main` at $DIR/return_invalid_mut_tuple.rs:LL:CC + = note: inside `foo` at $DIR/return_invalid_mut_tuple.rs:LL:CC +note: inside `main` at $DIR/return_invalid_mut_tuple.rs:LL:CC + --> $DIR/return_invalid_mut_tuple.rs:LL:CC + | +LL | foo(&mut (1, 2)).0; + | ^^^^^^^^^^^^^^^^ note: some details are omitted, run with `MIRIFLAGS=-Zmiri-backtrace=full` for a verbose backtrace diff --git a/src/tools/miri/tests/fail/stacked_borrows/return_invalid_shr_option.rs b/src/tools/miri/tests/fail/stacked_borrows/return_invalid_shr_option.rs index 3a028ceed86a..094ce33b9c1f 100644 --- a/src/tools/miri/tests/fail/stacked_borrows/return_invalid_shr_option.rs +++ b/src/tools/miri/tests/fail/stacked_borrows/return_invalid_shr_option.rs @@ -1,15 +1,14 @@ // Make sure that we cannot return a `&` that got already invalidated, not even in an `Option`. -// Due to shallow reborrowing, the error only surfaces when we look into the `Option`. fn foo(x: &mut (i32, i32)) -> Option<&i32> { let xraw = x as *mut (i32, i32); let ret = Some(unsafe { &(*xraw).1 }); unsafe { *xraw = (42, 23) }; // unfreeze - ret + ret //~ ERROR: /retag .* tag does not exist in the borrow stack/ } fn main() { match foo(&mut (1, 2)) { - Some(_x) => {} //~ ERROR: /retag .* tag does not exist in the borrow stack/ + Some(_x) => {} None => {} } } diff --git a/src/tools/miri/tests/fail/stacked_borrows/return_invalid_shr_option.stderr b/src/tools/miri/tests/fail/stacked_borrows/return_invalid_shr_option.stderr index f45456305db2..6066bf89f5d0 100644 --- a/src/tools/miri/tests/fail/stacked_borrows/return_invalid_shr_option.stderr +++ b/src/tools/miri/tests/fail/stacked_borrows/return_invalid_shr_option.stderr @@ -1,11 +1,11 @@ error: Undefined Behavior: trying to retag from for SharedReadOnly permission at ALLOC[0x4], but that tag does not exist in the borrow stack for this location --> $DIR/return_invalid_shr_option.rs:LL:CC | -LL | Some(_x) => {} - | ^^ - | | - | trying to retag from for SharedReadOnly permission at ALLOC[0x4], but that tag does not exist in the borrow stack for this location - | this error occurs as part of retag at ALLOC[0x4..0x8] +LL | ret + | ^^^ + | | + | trying to retag from for SharedReadOnly permission at ALLOC[0x4], but that tag does not exist in the borrow stack for this location + | this error occurs as part of retag at ALLOC[0x4..0x8] | = help: this indicates a potential bug in the program: it performed an invalid operation, but the Stacked Borrows rules it violated are still experimental = help: see https://github.com/rust-lang/unsafe-code-guidelines/blob/master/wip/stacked-borrows.md for further information @@ -13,14 +13,19 @@ help: was created by a SharedReadOnly retag at offsets [0x4..0x8] --> $DIR/return_invalid_shr_option.rs:LL:CC | LL | let ret = Some(unsafe { &(*xraw).1 }); - | ^^^^^^^^^^ + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: was later invalidated at offsets [0x0..0x8] by a write access --> $DIR/return_invalid_shr_option.rs:LL:CC | LL | unsafe { *xraw = (42, 23) }; // unfreeze | ^^^^^^^^^^^^^^^^ = note: BACKTRACE: - = note: inside `main` at $DIR/return_invalid_shr_option.rs:LL:CC + = note: inside `foo` at $DIR/return_invalid_shr_option.rs:LL:CC +note: inside `main` at $DIR/return_invalid_shr_option.rs:LL:CC + --> $DIR/return_invalid_shr_option.rs:LL:CC + | +LL | match foo(&mut (1, 2)) { + | ^^^^^^^^^^^^^^^^ note: some details are omitted, run with `MIRIFLAGS=-Zmiri-backtrace=full` for a verbose backtrace diff --git a/src/tools/miri/tests/fail/stacked_borrows/return_invalid_shr_tuple.rs b/src/tools/miri/tests/fail/stacked_borrows/return_invalid_shr_tuple.rs index e4536626dbf2..d0fd53e06aa2 100644 --- a/src/tools/miri/tests/fail/stacked_borrows/return_invalid_shr_tuple.rs +++ b/src/tools/miri/tests/fail/stacked_borrows/return_invalid_shr_tuple.rs @@ -1,12 +1,11 @@ // Make sure that we cannot return a `&` that got already invalidated, not even in a tuple. -// Due to shallow reborrowing, the error only surfaces when we look into the tuple. fn foo(x: &mut (i32, i32)) -> (&i32,) { let xraw = x as *mut (i32, i32); let ret = (unsafe { &(*xraw).1 },); unsafe { *xraw = (42, 23) }; // unfreeze - ret + ret //~ ERROR: /retag .* tag does not exist in the borrow stack/ } fn main() { - foo(&mut (1, 2)).0; //~ ERROR: /retag .* tag does not exist in the borrow stack/ + foo(&mut (1, 2)).0; } diff --git a/src/tools/miri/tests/fail/stacked_borrows/return_invalid_shr_tuple.stderr b/src/tools/miri/tests/fail/stacked_borrows/return_invalid_shr_tuple.stderr index 2e41f505bb9d..52d365246a74 100644 --- a/src/tools/miri/tests/fail/stacked_borrows/return_invalid_shr_tuple.stderr +++ b/src/tools/miri/tests/fail/stacked_borrows/return_invalid_shr_tuple.stderr @@ -1,8 +1,8 @@ error: Undefined Behavior: trying to retag from for SharedReadOnly permission at ALLOC[0x4], but that tag does not exist in the borrow stack for this location --> $DIR/return_invalid_shr_tuple.rs:LL:CC | -LL | foo(&mut (1, 2)).0; - | ^^^^^^^^^^^^^^^^^^ +LL | ret + | ^^^ | | | trying to retag from for SharedReadOnly permission at ALLOC[0x4], but that tag does not exist in the borrow stack for this location | this error occurs as part of retag at ALLOC[0x4..0x8] @@ -13,14 +13,19 @@ help: was created by a SharedReadOnly retag at offsets [0x4..0x8] --> $DIR/return_invalid_shr_tuple.rs:LL:CC | LL | let ret = (unsafe { &(*xraw).1 },); - | ^^^^^^^^^^ + | ^^^^^^^^^^^^^^^^^^^^^^^^ help: was later invalidated at offsets [0x0..0x8] by a write access --> $DIR/return_invalid_shr_tuple.rs:LL:CC | LL | unsafe { *xraw = (42, 23) }; // unfreeze | ^^^^^^^^^^^^^^^^ = note: BACKTRACE: - = note: inside `main` at $DIR/return_invalid_shr_tuple.rs:LL:CC + = note: inside `foo` at $DIR/return_invalid_shr_tuple.rs:LL:CC +note: inside `main` at $DIR/return_invalid_shr_tuple.rs:LL:CC + --> $DIR/return_invalid_shr_tuple.rs:LL:CC + | +LL | foo(&mut (1, 2)).0; + | ^^^^^^^^^^^^^^^^ note: some details are omitted, run with `MIRIFLAGS=-Zmiri-backtrace=full` for a verbose backtrace diff --git a/src/tools/miri/tests/pass/stacked-borrows/no_field_retagging.rs b/src/tools/miri/tests/pass/stacked-borrows/no_field_retagging.rs new file mode 100644 index 000000000000..48fc8e8668ce --- /dev/null +++ b/src/tools/miri/tests/pass/stacked-borrows/no_field_retagging.rs @@ -0,0 +1,19 @@ +//@compile-flags: -Zmiri-retag-fields=none + +struct Newtype<'a>(&'a mut i32); + +fn dealloc_while_running(_n: Newtype<'_>, dealloc: impl FnOnce()) { + dealloc(); +} + +// Make sure that we do *not* retag the fields of `Newtype`. +fn main() { + let ptr = Box::into_raw(Box::new(0i32)); + #[rustfmt::skip] // I like my newlines + unsafe { + dealloc_while_running( + Newtype(&mut *ptr), + || drop(Box::from_raw(ptr)), + ) + }; +} diff --git a/src/tools/miri/tests/pass/stacked-borrows/stack-printing.stdout b/src/tools/miri/tests/pass/stacked-borrows/stack-printing.stdout index 838733078209..296339e73845 100644 --- a/src/tools/miri/tests/pass/stacked-borrows/stack-printing.stdout +++ b/src/tools/miri/tests/pass/stacked-borrows/stack-printing.stdout @@ -1,6 +1,6 @@ 0..1: [ SharedReadWrite ] 0..1: [ SharedReadWrite ] 0..1: [ SharedReadWrite ] -0..1: [ SharedReadWrite Unique Unique Unique Unique Unique ] -0..1: [ SharedReadWrite Disabled Disabled Disabled Disabled Disabled SharedReadOnly ] +0..1: [ SharedReadWrite Unique Unique Unique Unique Unique Unique Unique ] +0..1: [ SharedReadWrite Disabled Disabled Disabled Disabled Disabled Disabled Disabled SharedReadOnly ] 0..1: [ unknown-bottom(..) ] From 0b49a5d17354c3b0980c3148f8291e429fc9a194 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Sun, 30 Oct 2022 09:10:45 +0100 Subject: [PATCH 05/14] rustup --- src/tools/miri/miri | 2 +- src/tools/miri/rust-version | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/tools/miri/miri b/src/tools/miri/miri index 52902410a6b2..a0593deb08a3 100755 --- a/src/tools/miri/miri +++ b/src/tools/miri/miri @@ -121,7 +121,7 @@ toolchain) rustc-pull) cd "$MIRIDIR" git fetch http://localhost:8000/rust-lang/rust.git$JOSH_FILTER.git master - git merge FETCH_HEAD + git merge FETCH_HEAD --no-ff -m "Merge from rustc" exit 0 ;; rustc-push) diff --git a/src/tools/miri/rust-version b/src/tools/miri/rust-version index e582bf35356d..13492d183c99 100644 --- a/src/tools/miri/rust-version +++ b/src/tools/miri/rust-version @@ -1 +1 @@ -607878d069267e1402ad792c9331b426e4c6d0f9 +b03502b35d111bef0399a66ab3cc765f0802e8ba From 41c368ba8b0ca08c05ad7bf4a2c3dbbbfe9168ff Mon Sep 17 00:00:00 2001 From: Rageking8 Date: Mon, 31 Oct 2022 12:41:26 +0800 Subject: [PATCH 06/14] fix dupe word typos --- src/tools/miri/cargo-miri/src/phases.rs | 2 +- src/tools/miri/src/stacked_borrows/mod.rs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/tools/miri/cargo-miri/src/phases.rs b/src/tools/miri/cargo-miri/src/phases.rs index 22da80be9021..2c84165a0f0a 100644 --- a/src/tools/miri/cargo-miri/src/phases.rs +++ b/src/tools/miri/cargo-miri/src/phases.rs @@ -528,7 +528,7 @@ pub fn phase_runner(mut binary_args: impl Iterator, phase: Runner cmd.args(binary_args); // Make sure we use the build-time working directory for interpreting Miri/rustc arguments. - // But then we need to switch to the run-time one, which we instruct Miri do do by setting `MIRI_CWD`. + // But then we need to switch to the run-time one, which we instruct Miri do by setting `MIRI_CWD`. cmd.current_dir(info.current_dir); cmd.env("MIRI_CWD", env::current_dir().unwrap()); diff --git a/src/tools/miri/src/stacked_borrows/mod.rs b/src/tools/miri/src/stacked_borrows/mod.rs index 5ec787dd4411..7f18e5dbae05 100644 --- a/src/tools/miri/src/stacked_borrows/mod.rs +++ b/src/tools/miri/src/stacked_borrows/mod.rs @@ -252,7 +252,7 @@ pub fn err_sb_ub<'tcx>( /// We need to make at least the following things true: /// /// U1: After creating a `Uniq`, it is at the top. -/// U2: If the top is `Uniq`, accesses must be through that `Uniq` or remove it it. +/// U2: If the top is `Uniq`, accesses must be through that `Uniq` or remove it. /// U3: If an access happens with a `Uniq`, it requires the `Uniq` to be in the stack. /// /// F1: After creating a `&`, the parts outside `UnsafeCell` have our `SharedReadOnly` on top. From 224dff4e15f1195c1cdbd85d9080e13b14a151e2 Mon Sep 17 00:00:00 2001 From: DrMeepster <19316085+DrMeepster@users.noreply.github.com> Date: Sun, 30 Oct 2022 21:39:16 -0700 Subject: [PATCH 07/14] add acquire when init once is already complete --- src/tools/miri/src/concurrency/init_once.rs | 35 +++++++++-------- src/tools/miri/src/shims/windows/sync.rs | 6 ++- .../pass/concurrency/windows_init_once.rs | 38 +++++++++++++++++++ 3 files changed, 59 insertions(+), 20 deletions(-) diff --git a/src/tools/miri/src/concurrency/init_once.rs b/src/tools/miri/src/concurrency/init_once.rs index 791931901e2a..b1443662e2b7 100644 --- a/src/tools/miri/src/concurrency/init_once.rs +++ b/src/tools/miri/src/concurrency/init_once.rs @@ -141,18 +141,11 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> { // Wake up everyone. // need to take the queue to avoid having `this` be borrowed multiple times for waiter in std::mem::take(&mut init_once.waiters) { - // End of the wait happens-before woken-up thread. - if let Some(data_race) = &this.machine.data_race { - data_race.validate_lock_acquire( - &this.machine.threads.sync.init_onces[id].data_race, - waiter.thread, - ); - } - this.unblock_thread(waiter.thread); // Call callback, with the woken-up thread as `current`. this.set_active_thread(waiter.thread); + this.init_once_acquire(id); waiter.callback.call(this)?; this.set_active_thread(current_thread); } @@ -172,26 +165,17 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> { ); // Each complete happens-before the end of the wait - // FIXME: should this really induce synchronization? If we think of it as a lock, then yes, - // but the docs don't talk about such details. if let Some(data_race) = &this.machine.data_race { data_race.validate_lock_release(&mut init_once.data_race, current_thread); } // Wake up one waiting thread, so they can go ahead and try to init this. if let Some(waiter) = init_once.waiters.pop_front() { - // End of the wait happens-before woken-up thread. - if let Some(data_race) = &this.machine.data_race { - data_race.validate_lock_acquire( - &this.machine.threads.sync.init_onces[id].data_race, - waiter.thread, - ); - } - this.unblock_thread(waiter.thread); // Call callback, with the woken-up thread as `current`. this.set_active_thread(waiter.thread); + this.init_once_acquire(id); waiter.callback.call(this)?; this.set_active_thread(current_thread); } else { @@ -201,4 +185,19 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> { Ok(()) } + + /// Synchronize with the previous completion or failure of an InitOnce. + /// This is required to prevent data races. + #[inline] + fn init_once_acquire(&mut self, id: InitOnceId) { + let this = self.eval_context_mut(); + let current_thread = this.get_active_thread(); + + if let Some(data_race) = &this.machine.data_race { + data_race.validate_lock_acquire( + &this.machine.threads.sync.init_onces[id].data_race, + current_thread, + ); + } + } } diff --git a/src/tools/miri/src/shims/windows/sync.rs b/src/tools/miri/src/shims/windows/sync.rs index 8156ae8af1ef..f8980e188b45 100644 --- a/src/tools/miri/src/shims/windows/sync.rs +++ b/src/tools/miri/src/shims/windows/sync.rs @@ -177,8 +177,10 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> { Box::new(Callback { init_once_id: id, pending_place }), ) } - InitOnceStatus::Complete => - this.write_scalar(this.eval_windows("c", "FALSE")?, &pending_place)?, + InitOnceStatus::Complete => { + this.init_once_acquire(id); + this.write_scalar(this.eval_windows("c", "FALSE")?, &pending_place)?; + } } // This always succeeds (even if the thread is blocked, we will succeed if we ever unblock). diff --git a/src/tools/miri/tests/pass/concurrency/windows_init_once.rs b/src/tools/miri/tests/pass/concurrency/windows_init_once.rs index d3c72c3d028c..6e5129acaf85 100644 --- a/src/tools/miri/tests/pass/concurrency/windows_init_once.rs +++ b/src/tools/miri/tests/pass/concurrency/windows_init_once.rs @@ -131,8 +131,46 @@ fn retry_on_fail() { waiter2.join().unwrap(); } +fn no_data_race_after_complete() { + let mut init_once = null_mut(); + let mut pending = 0; + + unsafe { + assert_eq!(InitOnceBeginInitialize(&mut init_once, 0, &mut pending, null_mut()), TRUE); + assert_eq!(pending, TRUE); + } + + let init_once_ptr = SendPtr(&mut init_once); + + let mut place = 0; + let place_ptr = SendPtr(&mut place); + + let reader = thread::spawn(move || unsafe { + let mut pending = 0; + + assert_eq!(InitOnceBeginInitialize(init_once_ptr.0, 0, &mut pending, null_mut()), TRUE); + assert_eq!(pending, FALSE); + // this should not data race + place_ptr.0.read() + }); + + unsafe { + // this should not data race + place_ptr.0.write(1); + } + + unsafe { + assert_eq!(InitOnceComplete(init_once_ptr.0, 0, null_mut()), TRUE); + } + //println!("complete"); + + // run reader + assert_eq!(reader.join().unwrap(), 1); +} + fn main() { single_thread(); block_until_complete(); retry_on_fail(); + no_data_race_after_complete(); } From a1cd27906c04ea1331c921f0c115135128aadc55 Mon Sep 17 00:00:00 2001 From: Rageking8 Date: Mon, 31 Oct 2022 15:56:48 +0800 Subject: [PATCH 08/14] followup for pr 2640 --- src/tools/miri/cargo-miri/src/phases.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/tools/miri/cargo-miri/src/phases.rs b/src/tools/miri/cargo-miri/src/phases.rs index 2c84165a0f0a..df36041c75ed 100644 --- a/src/tools/miri/cargo-miri/src/phases.rs +++ b/src/tools/miri/cargo-miri/src/phases.rs @@ -528,7 +528,7 @@ pub fn phase_runner(mut binary_args: impl Iterator, phase: Runner cmd.args(binary_args); // Make sure we use the build-time working directory for interpreting Miri/rustc arguments. - // But then we need to switch to the run-time one, which we instruct Miri do by setting `MIRI_CWD`. + // But then we need to switch to the run-time one, which we instruct Miri to do by setting `MIRI_CWD`. cmd.current_dir(info.current_dir); cmd.env("MIRI_CWD", env::current_dir().unwrap()); From 95549078d7260dba327bf87cad38d9b87ba10a3a Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Wed, 2 Nov 2022 08:43:53 +0100 Subject: [PATCH 09/14] fix ./miri bench --- src/tools/miri/miri | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/tools/miri/miri b/src/tools/miri/miri index a0593deb08a3..f0986bfb1cdb 100755 --- a/src/tools/miri/miri +++ b/src/tools/miri/miri @@ -79,6 +79,8 @@ shift MIRIDIR=$(python3 -c 'import os, sys; print(os.path.dirname(os.path.realpath(sys.argv[1])))' "$0") # Used for rustc syncs. JOSH_FILTER=":at_commit=75dd959a3a40eb5b4574f8d2e23aa6efbeb33573[:prefix=src/tools/miri]:/src/tools/miri" +# Needed for `./miri bench`. +TOOLCHAIN=$(cd "$MIRIDIR"; rustup show active-toolchain | head -n 1 | cut -d ' ' -f 1) ## Early commands, that don't do auto-things and don't want the environment-altering things happening below. case "$COMMAND" in @@ -189,6 +191,8 @@ if [ -z "$MIRI_AUTO_OPS" ]; then # other code has run. if [ -f "$MIRIDIR/.auto-everything" ] || [ -f "$MIRIDIR/.auto-toolchain" ] ; then $0 toolchain + # Let's make sure to actually use that toolchain, too. + TOOLCHAIN=miri fi if [ -f "$MIRIDIR/.auto-everything" ] || [ -f "$MIRIDIR/.auto-fmt" ] ; then @@ -202,7 +206,6 @@ fi ## Prepare the environment # Determine some toolchain properties -TOOLCHAIN=$(cd "$MIRIDIR"; rustup show active-toolchain | head -n 1 | cut -d ' ' -f 1) TARGET=$(rustc +$TOOLCHAIN --version --verbose | grep "^host:" | cut -d ' ' -f 2) SYSROOT=$(rustc +$TOOLCHAIN --print sysroot) LIBDIR=$SYSROOT/lib/rustlib/$TARGET/lib From fa1b720cfc600ed09dd2a6310ece57291c5bae5b Mon Sep 17 00:00:00 2001 From: DrMeepster <19316085+DrMeepster@users.noreply.github.com> Date: Thu, 3 Nov 2022 18:13:53 -0700 Subject: [PATCH 10/14] refactor into private functions --- src/tools/miri/src/concurrency/init_once.rs | 75 ++++++++++++++------- src/tools/miri/src/shims/windows/sync.rs | 2 +- 2 files changed, 51 insertions(+), 26 deletions(-) diff --git a/src/tools/miri/src/concurrency/init_once.rs b/src/tools/miri/src/concurrency/init_once.rs index b1443662e2b7..eb42cdf80abb 100644 --- a/src/tools/miri/src/concurrency/init_once.rs +++ b/src/tools/miri/src/concurrency/init_once.rs @@ -3,7 +3,7 @@ use std::num::NonZeroU32; use rustc_index::vec::Idx; -use super::sync::EvalContextExtPriv; +use super::sync::EvalContextExtPriv as _; use super::thread::MachineCallback; use super::vector_clock::VClock; use crate::*; @@ -52,6 +52,43 @@ impl<'mir, 'tcx> VisitTags for InitOnce<'mir, 'tcx> { } } +impl<'mir, 'tcx: 'mir> EvalContextExtPriv<'mir, 'tcx> for crate::MiriInterpCx<'mir, 'tcx> {} +trait EvalContextExtPriv<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> { + /// Synchronize with the previous initialization attempt of an InitOnce. + #[inline] + fn init_once_observe_attempt(&mut self, id: InitOnceId) { + let this = self.eval_context_mut(); + let current_thread = this.get_active_thread(); + + if let Some(data_race) = &this.machine.data_race { + data_race.validate_lock_acquire( + &this.machine.threads.sync.init_onces[id].data_race, + current_thread, + ); + } + } + + #[inline] + fn init_once_wake_waiter( + &mut self, + id: InitOnceId, + waiter: InitOnceWaiter<'mir, 'tcx>, + ) -> InterpResult<'tcx> { + let this = self.eval_context_mut(); + let current_thread = this.get_active_thread(); + + this.unblock_thread(waiter.thread); + + // Call callback, with the woken-up thread as `current`. + this.set_active_thread(waiter.thread); + this.init_once_observe_attempt(id); + waiter.callback.call(this)?; + this.set_active_thread(current_thread); + + Ok(()) + } +} + impl<'mir, 'tcx: 'mir> EvalContextExt<'mir, 'tcx> for crate::MiriInterpCx<'mir, 'tcx> {} pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> { fn init_once_get_or_create_id( @@ -141,13 +178,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> { // Wake up everyone. // need to take the queue to avoid having `this` be borrowed multiple times for waiter in std::mem::take(&mut init_once.waiters) { - this.unblock_thread(waiter.thread); - - // Call callback, with the woken-up thread as `current`. - this.set_active_thread(waiter.thread); - this.init_once_acquire(id); - waiter.callback.call(this)?; - this.set_active_thread(current_thread); + this.init_once_wake_waiter(id, waiter)?; } Ok(()) @@ -171,13 +202,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> { // Wake up one waiting thread, so they can go ahead and try to init this. if let Some(waiter) = init_once.waiters.pop_front() { - this.unblock_thread(waiter.thread); - - // Call callback, with the woken-up thread as `current`. - this.set_active_thread(waiter.thread); - this.init_once_acquire(id); - waiter.callback.call(this)?; - this.set_active_thread(current_thread); + this.init_once_wake_waiter(id, waiter)?; } else { // Nobody there to take this, so go back to 'uninit' init_once.status = InitOnceStatus::Uninitialized; @@ -186,18 +211,18 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> { Ok(()) } - /// Synchronize with the previous completion or failure of an InitOnce. - /// This is required to prevent data races. + /// Synchronize with the previous completion of an InitOnce. + /// Must only be called after checking that it is complete. #[inline] - fn init_once_acquire(&mut self, id: InitOnceId) { + fn init_once_observe_completed(&mut self, id: InitOnceId) { let this = self.eval_context_mut(); - let current_thread = this.get_active_thread(); - if let Some(data_race) = &this.machine.data_race { - data_race.validate_lock_acquire( - &this.machine.threads.sync.init_onces[id].data_race, - current_thread, - ); - } + assert_eq!( + this.init_once_status(id), + InitOnceStatus::Complete, + "observing the completion of incomplete init once" + ); + + this.init_once_observe_attempt(id); } } diff --git a/src/tools/miri/src/shims/windows/sync.rs b/src/tools/miri/src/shims/windows/sync.rs index f8980e188b45..098804626f2f 100644 --- a/src/tools/miri/src/shims/windows/sync.rs +++ b/src/tools/miri/src/shims/windows/sync.rs @@ -178,7 +178,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> { ) } InitOnceStatus::Complete => { - this.init_once_acquire(id); + this.init_once_observe_completed(id); this.write_scalar(this.eval_windows("c", "FALSE")?, &pending_place)?; } } From cb6f7a6a84e3e808fa95a1206843a96f8b9ff44d Mon Sep 17 00:00:00 2001 From: DrMeepster <19316085+DrMeepster@users.noreply.github.com> Date: Thu, 3 Nov 2022 18:30:04 -0700 Subject: [PATCH 11/14] clarify no_data_race_after_complete test --- src/tools/miri/tests/pass/concurrency/windows_init_once.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/tools/miri/tests/pass/concurrency/windows_init_once.rs b/src/tools/miri/tests/pass/concurrency/windows_init_once.rs index 6e5129acaf85..4eb883796205 100644 --- a/src/tools/miri/tests/pass/concurrency/windows_init_once.rs +++ b/src/tools/miri/tests/pass/concurrency/windows_init_once.rs @@ -148,6 +148,7 @@ fn no_data_race_after_complete() { let reader = thread::spawn(move || unsafe { let mut pending = 0; + // this doesn't block because reader only executes after `InitOnceComplete` is called assert_eq!(InitOnceBeginInitialize(init_once_ptr.0, 0, &mut pending, null_mut()), TRUE); assert_eq!(pending, FALSE); // this should not data race @@ -162,9 +163,8 @@ fn no_data_race_after_complete() { unsafe { assert_eq!(InitOnceComplete(init_once_ptr.0, 0, null_mut()), TRUE); } - //println!("complete"); - // run reader + // run reader (without preemption, it has not taken a step yet) assert_eq!(reader.join().unwrap(), 1); } From a1d94d43ac876975e15b5069da1f9b7a81e29733 Mon Sep 17 00:00:00 2001 From: DrMeepster <19316085+DrMeepster@users.noreply.github.com> Date: Sat, 29 Oct 2022 22:58:34 -0700 Subject: [PATCH 12/14] impl condvars for windows --- src/tools/miri/src/concurrency/sync.rs | 14 +- src/tools/miri/src/shims/unix/sync.rs | 14 +- .../miri/src/shims/windows/foreign_items.rs | 19 +++ src/tools/miri/src/shims/windows/sync.rs | 146 ++++++++++++++++++ src/tools/miri/tests/pass/concurrency/sync.rs | 20 +-- .../tests/pass/concurrency/sync_nopreempt.rs | 1 - .../miri/tests/pass/panic/concurrent-panic.rs | 1 - 7 files changed, 185 insertions(+), 30 deletions(-) diff --git a/src/tools/miri/src/concurrency/sync.rs b/src/tools/miri/src/concurrency/sync.rs index e76610e73028..48f9e605276e 100644 --- a/src/tools/miri/src/concurrency/sync.rs +++ b/src/tools/miri/src/concurrency/sync.rs @@ -121,8 +121,10 @@ declare_id!(CondvarId); struct CondvarWaiter { /// The thread that is waiting on this variable. thread: ThreadId, - /// The mutex on which the thread is waiting. - mutex: MutexId, + /// The mutex or rwlock on which the thread is waiting. + lock: u32, + /// If the lock is shared or exclusive + shared: bool, } /// The conditional variable state. @@ -569,16 +571,16 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> { } /// Mark that the thread is waiting on the conditional variable. - fn condvar_wait(&mut self, id: CondvarId, thread: ThreadId, mutex: MutexId) { + fn condvar_wait(&mut self, id: CondvarId, thread: ThreadId, lock: u32, shared: bool) { let this = self.eval_context_mut(); let waiters = &mut this.machine.threads.sync.condvars[id].waiters; assert!(waiters.iter().all(|waiter| waiter.thread != thread), "thread is already waiting"); - waiters.push_back(CondvarWaiter { thread, mutex }); + waiters.push_back(CondvarWaiter { thread, lock, shared }); } /// Wake up some thread (if there is any) sleeping on the conditional /// variable. - fn condvar_signal(&mut self, id: CondvarId) -> Option<(ThreadId, MutexId)> { + fn condvar_signal(&mut self, id: CondvarId) -> Option<(ThreadId, u32, bool)> { let this = self.eval_context_mut(); let current_thread = this.get_active_thread(); let condvar = &mut this.machine.threads.sync.condvars[id]; @@ -592,7 +594,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> { if let Some(data_race) = data_race { data_race.validate_lock_acquire(&condvar.data_race, waiter.thread); } - (waiter.thread, waiter.mutex) + (waiter.thread, waiter.lock, waiter.shared) }) } diff --git a/src/tools/miri/src/shims/unix/sync.rs b/src/tools/miri/src/shims/unix/sync.rs index fcb006920794..d24e1a56bd52 100644 --- a/src/tools/miri/src/shims/unix/sync.rs +++ b/src/tools/miri/src/shims/unix/sync.rs @@ -696,8 +696,9 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> { fn pthread_cond_signal(&mut self, cond_op: &OpTy<'tcx, Provenance>) -> InterpResult<'tcx, i32> { let this = self.eval_context_mut(); let id = this.condvar_get_or_create_id(cond_op, CONDVAR_ID_OFFSET)?; - if let Some((thread, mutex)) = this.condvar_signal(id) { - post_cond_signal(this, thread, mutex)?; + if let Some((thread, mutex, shared)) = this.condvar_signal(id) { + assert!(!shared); + post_cond_signal(this, thread, MutexId::from_u32(mutex))?; } Ok(0) @@ -710,8 +711,9 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> { let this = self.eval_context_mut(); let id = this.condvar_get_or_create_id(cond_op, CONDVAR_ID_OFFSET)?; - while let Some((thread, mutex)) = this.condvar_signal(id) { - post_cond_signal(this, thread, mutex)?; + while let Some((thread, mutex, shared)) = this.condvar_signal(id) { + assert!(!shared); + post_cond_signal(this, thread, MutexId::from_u32(mutex))?; } Ok(0) @@ -729,7 +731,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> { let active_thread = this.get_active_thread(); release_cond_mutex_and_block(this, active_thread, mutex_id)?; - this.condvar_wait(id, active_thread, mutex_id); + this.condvar_wait(id, active_thread, mutex_id.to_u32(), false); Ok(0) } @@ -768,7 +770,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> { }; release_cond_mutex_and_block(this, active_thread, mutex_id)?; - this.condvar_wait(id, active_thread, mutex_id); + this.condvar_wait(id, active_thread, mutex_id.to_u32(), false); // We return success for now and override it in the timeout callback. this.write_scalar(Scalar::from_i32(0), dest)?; diff --git a/src/tools/miri/src/shims/windows/foreign_items.rs b/src/tools/miri/src/shims/windows/foreign_items.rs index 2a34a3a47bbb..e16749c986b1 100644 --- a/src/tools/miri/src/shims/windows/foreign_items.rs +++ b/src/tools/miri/src/shims/windows/foreign_items.rs @@ -273,6 +273,25 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> { let result = this.InitOnceComplete(ptr, flags, context)?; this.write_scalar(result, dest)?; } + "SleepConditionVariableSRW" => { + let [condvar, lock, timeout, flags] = + this.check_shim(abi, Abi::System { unwind: false }, link_name, args)?; + + let result = this.SleepConditionVariableSRW(condvar, lock, timeout, flags, dest)?; + this.write_scalar(result, dest)?; + } + "WakeConditionVariable" => { + let [condvar] = + this.check_shim(abi, Abi::System { unwind: false }, link_name, args)?; + + this.WakeConditionVariable(condvar)?; + } + "WakeAllConditionVariable" => { + let [condvar] = + this.check_shim(abi, Abi::System { unwind: false }, link_name, args)?; + + this.WakeAllConditionVariable(condvar)?; + } // Dynamic symbol loading "GetProcAddress" => { diff --git a/src/tools/miri/src/shims/windows/sync.rs b/src/tools/miri/src/shims/windows/sync.rs index 098804626f2f..2eab1794c4f4 100644 --- a/src/tools/miri/src/shims/windows/sync.rs +++ b/src/tools/miri/src/shims/windows/sync.rs @@ -8,6 +8,38 @@ use crate::*; const SRWLOCK_ID_OFFSET: u64 = 0; const INIT_ONCE_ID_OFFSET: u64 = 0; +const CONDVAR_ID_OFFSET: u64 = 0; + +impl<'mir, 'tcx> EvalContextExtPriv<'mir, 'tcx> for crate::MiriInterpCx<'mir, 'tcx> {} +pub trait EvalContextExtPriv<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> { + /// Try to reacquire the lock associated with the condition variable after we + /// were signaled. + fn reacquire_cond_lock( + &mut self, + thread: ThreadId, + lock: RwLockId, + shared: bool, + ) -> InterpResult<'tcx> { + let this = self.eval_context_mut(); + this.unblock_thread(thread); + + if shared { + if this.rwlock_is_locked(lock) { + this.rwlock_enqueue_and_block_reader(lock, thread); + } else { + this.rwlock_reader_lock(lock, thread); + } + } else { + if this.rwlock_is_write_locked(lock) { + this.rwlock_enqueue_and_block_writer(lock, thread); + } else { + this.rwlock_writer_lock(lock, thread); + } + } + + Ok(()) + } +} impl<'mir, 'tcx> EvalContextExt<'mir, 'tcx> for crate::MiriInterpCx<'mir, 'tcx> {} #[allow(non_snake_case)] @@ -327,4 +359,118 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> { Ok(()) } + + fn SleepConditionVariableSRW( + &mut self, + condvar_op: &OpTy<'tcx, Provenance>, + lock_op: &OpTy<'tcx, Provenance>, + timeout_op: &OpTy<'tcx, Provenance>, + flags_op: &OpTy<'tcx, Provenance>, + dest: &PlaceTy<'tcx, Provenance>, + ) -> InterpResult<'tcx, Scalar> { + let this = self.eval_context_mut(); + + let condvar_id = this.condvar_get_or_create_id(condvar_op, CONDVAR_ID_OFFSET)?; + let lock_id = this.rwlock_get_or_create_id(lock_op, SRWLOCK_ID_OFFSET)?; + let timeout_ms = this.read_scalar(timeout_op)?.to_u32()?; + let flags = this.read_scalar(flags_op)?.to_u32()?; + + let timeout_time = if timeout_ms == this.eval_windows("c", "INFINITE")?.to_u32()? { + None + } else { + let duration = Duration::from_millis(timeout_ms.into()); + Some(this.machine.clock.now().checked_add(duration).unwrap()) + }; + + let shared_mode = 0x1; // CONDITION_VARIABLE_LOCKMODE_SHARED is not in std + let shared = flags == shared_mode; + + let active_thread = this.get_active_thread(); + + let was_locked = if shared { + this.rwlock_reader_unlock(lock_id, active_thread) + } else { + this.rwlock_writer_unlock(lock_id, active_thread) + }; + + if !was_locked { + throw_ub_format!( + "calling SleepConditionVariableSRW with an SRWLock that is not locked by the current thread" + ); + } + + this.block_thread(active_thread); + this.condvar_wait(condvar_id, active_thread, lock_id.to_u32(), shared); + + if let Some(timeout_time) = timeout_time { + struct Callback<'tcx> { + thread: ThreadId, + condvar_id: CondvarId, + lock_id: RwLockId, + shared: bool, + dest: PlaceTy<'tcx, Provenance>, + } + + impl<'tcx> VisitTags for Callback<'tcx> { + fn visit_tags(&self, visit: &mut dyn FnMut(SbTag)) { + let Callback { thread: _, condvar_id: _, lock_id: _, shared: _, dest } = self; + dest.visit_tags(visit); + } + } + + impl<'mir, 'tcx: 'mir> MachineCallback<'mir, 'tcx> for Callback<'tcx> { + fn call(&self, this: &mut MiriInterpCx<'mir, 'tcx>) -> InterpResult<'tcx> { + this.reacquire_cond_lock(self.thread, self.lock_id, self.shared)?; + + this.condvar_remove_waiter(self.condvar_id, self.thread); + + let error_timeout = this.eval_windows("c", "ERROR_TIMEOUT")?; + this.set_last_error(error_timeout)?; + this.write_scalar(this.eval_windows("c", "FALSE")?, &self.dest)?; + Ok(()) + } + } + + this.register_timeout_callback( + active_thread, + Time::Monotonic(timeout_time), + Box::new(Callback { + thread: active_thread, + condvar_id, + lock_id, + shared, + dest: dest.clone(), + }), + ); + } + + this.eval_windows("c", "TRUE") + } + + fn WakeConditionVariable(&mut self, condvar_op: &OpTy<'tcx, Provenance>) -> InterpResult<'tcx> { + let this = self.eval_context_mut(); + let condvar_id = this.condvar_get_or_create_id(condvar_op, CONDVAR_ID_OFFSET)?; + + if let Some((thread, lock, shared)) = this.condvar_signal(condvar_id) { + this.reacquire_cond_lock(thread, RwLockId::from_u32(lock), shared)?; + this.unregister_timeout_callback_if_exists(thread); + } + + Ok(()) + } + + fn WakeAllConditionVariable( + &mut self, + condvar_op: &OpTy<'tcx, Provenance>, + ) -> InterpResult<'tcx> { + let this = self.eval_context_mut(); + let condvar_id = this.condvar_get_or_create_id(condvar_op, CONDVAR_ID_OFFSET)?; + + while let Some((thread, lock, shared)) = this.condvar_signal(condvar_id) { + this.reacquire_cond_lock(thread, RwLockId::from_u32(lock), shared)?; + this.unregister_timeout_callback_if_exists(thread); + } + + Ok(()) + } } diff --git a/src/tools/miri/tests/pass/concurrency/sync.rs b/src/tools/miri/tests/pass/concurrency/sync.rs index b1518a49fbb1..19ea6c130bdd 100644 --- a/src/tools/miri/tests/pass/concurrency/sync.rs +++ b/src/tools/miri/tests/pass/concurrency/sync.rs @@ -230,20 +230,8 @@ fn main() { check_once(); park_timeout(); park_unpark(); - - if !cfg!(windows) { - // ignore-target-windows: Condvars on Windows are not supported yet - check_barriers(); - check_conditional_variables_notify_one(); - check_conditional_variables_timed_wait_timeout(); - check_conditional_variables_timed_wait_notimeout(); - } else { - // We need to fake the same output... - for _ in 0..10 { - println!("before wait"); - } - for _ in 0..10 { - println!("after wait"); - } - } + check_barriers(); + check_conditional_variables_notify_one(); + check_conditional_variables_timed_wait_timeout(); + check_conditional_variables_timed_wait_notimeout(); } diff --git a/src/tools/miri/tests/pass/concurrency/sync_nopreempt.rs b/src/tools/miri/tests/pass/concurrency/sync_nopreempt.rs index 55206f4bfc52..c6cff038f81e 100644 --- a/src/tools/miri/tests/pass/concurrency/sync_nopreempt.rs +++ b/src/tools/miri/tests/pass/concurrency/sync_nopreempt.rs @@ -1,4 +1,3 @@ -//@ignore-target-windows: Condvars on Windows are not supported yet. // We are making scheduler assumptions here. //@compile-flags: -Zmiri-strict-provenance -Zmiri-preemption-rate=0 diff --git a/src/tools/miri/tests/pass/panic/concurrent-panic.rs b/src/tools/miri/tests/pass/panic/concurrent-panic.rs index 342269c6acbe..776bc2057f35 100644 --- a/src/tools/miri/tests/pass/panic/concurrent-panic.rs +++ b/src/tools/miri/tests/pass/panic/concurrent-panic.rs @@ -1,4 +1,3 @@ -//@ignore-target-windows: Condvars on Windows are not supported yet. // We are making scheduler assumptions here. //@compile-flags: -Zmiri-preemption-rate=0 From a2f7e8497e12e6d79d7963729ef6746b912a32eb Mon Sep 17 00:00:00 2001 From: DrMeepster <19316085+DrMeepster@users.noreply.github.com> Date: Sun, 30 Oct 2022 14:56:49 -0700 Subject: [PATCH 13/14] use enum for condvar locks --- src/tools/miri/src/concurrency/sync.rs | 24 +++++--- src/tools/miri/src/shims/unix/sync.rs | 23 +++++--- src/tools/miri/src/shims/windows/sync.rs | 73 ++++++++++++++---------- 3 files changed, 76 insertions(+), 44 deletions(-) diff --git a/src/tools/miri/src/concurrency/sync.rs b/src/tools/miri/src/concurrency/sync.rs index 48f9e605276e..ba5ae852c5a9 100644 --- a/src/tools/miri/src/concurrency/sync.rs +++ b/src/tools/miri/src/concurrency/sync.rs @@ -116,15 +116,25 @@ struct RwLock { declare_id!(CondvarId); +#[derive(Debug, Copy, Clone)] +pub enum RwLockMode { + Read, + Write, +} + +#[derive(Debug)] +pub enum CondvarLock { + Mutex(MutexId), + RwLock { id: RwLockId, mode: RwLockMode }, +} + /// A thread waiting on a conditional variable. #[derive(Debug)] struct CondvarWaiter { /// The thread that is waiting on this variable. thread: ThreadId, /// The mutex or rwlock on which the thread is waiting. - lock: u32, - /// If the lock is shared or exclusive - shared: bool, + lock: CondvarLock, } /// The conditional variable state. @@ -571,16 +581,16 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> { } /// Mark that the thread is waiting on the conditional variable. - fn condvar_wait(&mut self, id: CondvarId, thread: ThreadId, lock: u32, shared: bool) { + fn condvar_wait(&mut self, id: CondvarId, thread: ThreadId, lock: CondvarLock) { let this = self.eval_context_mut(); let waiters = &mut this.machine.threads.sync.condvars[id].waiters; assert!(waiters.iter().all(|waiter| waiter.thread != thread), "thread is already waiting"); - waiters.push_back(CondvarWaiter { thread, lock, shared }); + waiters.push_back(CondvarWaiter { thread, lock }); } /// Wake up some thread (if there is any) sleeping on the conditional /// variable. - fn condvar_signal(&mut self, id: CondvarId) -> Option<(ThreadId, u32, bool)> { + fn condvar_signal(&mut self, id: CondvarId) -> Option<(ThreadId, CondvarLock)> { let this = self.eval_context_mut(); let current_thread = this.get_active_thread(); let condvar = &mut this.machine.threads.sync.condvars[id]; @@ -594,7 +604,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> { if let Some(data_race) = data_race { data_race.validate_lock_acquire(&condvar.data_race, waiter.thread); } - (waiter.thread, waiter.lock, waiter.shared) + (waiter.thread, waiter.lock) }) } diff --git a/src/tools/miri/src/shims/unix/sync.rs b/src/tools/miri/src/shims/unix/sync.rs index d24e1a56bd52..a7275646847e 100644 --- a/src/tools/miri/src/shims/unix/sync.rs +++ b/src/tools/miri/src/shims/unix/sync.rs @@ -3,6 +3,7 @@ use std::time::SystemTime; use rustc_hir::LangItem; use rustc_middle::ty::{layout::TyAndLayout, query::TyCtxtAt, Ty}; +use crate::concurrency::sync::CondvarLock; use crate::concurrency::thread::{MachineCallback, Time}; use crate::*; @@ -696,9 +697,12 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> { fn pthread_cond_signal(&mut self, cond_op: &OpTy<'tcx, Provenance>) -> InterpResult<'tcx, i32> { let this = self.eval_context_mut(); let id = this.condvar_get_or_create_id(cond_op, CONDVAR_ID_OFFSET)?; - if let Some((thread, mutex, shared)) = this.condvar_signal(id) { - assert!(!shared); - post_cond_signal(this, thread, MutexId::from_u32(mutex))?; + if let Some((thread, lock)) = this.condvar_signal(id) { + if let CondvarLock::Mutex(mutex) = lock { + post_cond_signal(this, thread, mutex)?; + } else { + panic!("condvar should not have an rwlock on unix"); + } } Ok(0) @@ -711,9 +715,12 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> { let this = self.eval_context_mut(); let id = this.condvar_get_or_create_id(cond_op, CONDVAR_ID_OFFSET)?; - while let Some((thread, mutex, shared)) = this.condvar_signal(id) { - assert!(!shared); - post_cond_signal(this, thread, MutexId::from_u32(mutex))?; + while let Some((thread, lock)) = this.condvar_signal(id) { + if let CondvarLock::Mutex(mutex) = lock { + post_cond_signal(this, thread, mutex)?; + } else { + panic!("condvar should not have an rwlock on unix"); + } } Ok(0) @@ -731,7 +738,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> { let active_thread = this.get_active_thread(); release_cond_mutex_and_block(this, active_thread, mutex_id)?; - this.condvar_wait(id, active_thread, mutex_id.to_u32(), false); + this.condvar_wait(id, active_thread, CondvarLock::Mutex(mutex_id)); Ok(0) } @@ -770,7 +777,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> { }; release_cond_mutex_and_block(this, active_thread, mutex_id)?; - this.condvar_wait(id, active_thread, mutex_id.to_u32(), false); + this.condvar_wait(id, active_thread, CondvarLock::Mutex(mutex_id)); // We return success for now and override it in the timeout callback. this.write_scalar(Scalar::from_i32(0), dest)?; diff --git a/src/tools/miri/src/shims/windows/sync.rs b/src/tools/miri/src/shims/windows/sync.rs index 2eab1794c4f4..a34d142f4a5e 100644 --- a/src/tools/miri/src/shims/windows/sync.rs +++ b/src/tools/miri/src/shims/windows/sync.rs @@ -3,6 +3,7 @@ use std::time::Duration; use rustc_target::abi::Size; use crate::concurrency::init_once::InitOnceStatus; +use crate::concurrency::sync::{CondvarLock, RwLockMode}; use crate::concurrency::thread::MachineCallback; use crate::*; @@ -18,23 +19,24 @@ pub trait EvalContextExtPriv<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tc &mut self, thread: ThreadId, lock: RwLockId, - shared: bool, + mode: RwLockMode, ) -> InterpResult<'tcx> { let this = self.eval_context_mut(); this.unblock_thread(thread); - if shared { - if this.rwlock_is_locked(lock) { - this.rwlock_enqueue_and_block_reader(lock, thread); - } else { - this.rwlock_reader_lock(lock, thread); - } - } else { - if this.rwlock_is_write_locked(lock) { - this.rwlock_enqueue_and_block_writer(lock, thread); - } else { - this.rwlock_writer_lock(lock, thread); - } + match mode { + RwLockMode::Read => + if this.rwlock_is_locked(lock) { + this.rwlock_enqueue_and_block_reader(lock, thread); + } else { + this.rwlock_reader_lock(lock, thread); + }, + RwLockMode::Write => + if this.rwlock_is_write_locked(lock) { + this.rwlock_enqueue_and_block_writer(lock, thread); + } else { + this.rwlock_writer_lock(lock, thread); + }, } Ok(()) @@ -383,14 +385,19 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> { }; let shared_mode = 0x1; // CONDITION_VARIABLE_LOCKMODE_SHARED is not in std - let shared = flags == shared_mode; + let mode = if flags == 0 { + RwLockMode::Write + } else if flags == shared_mode { + RwLockMode::Read + } else { + throw_unsup_format!("unsupported `Flags` {flags} in `SleepConditionVariableSRW`"); + }; let active_thread = this.get_active_thread(); - let was_locked = if shared { - this.rwlock_reader_unlock(lock_id, active_thread) - } else { - this.rwlock_writer_unlock(lock_id, active_thread) + let was_locked = match mode { + RwLockMode::Read => this.rwlock_reader_unlock(lock_id, active_thread), + RwLockMode::Write => this.rwlock_writer_unlock(lock_id, active_thread), }; if !was_locked { @@ -400,27 +407,27 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> { } this.block_thread(active_thread); - this.condvar_wait(condvar_id, active_thread, lock_id.to_u32(), shared); + this.condvar_wait(condvar_id, active_thread, CondvarLock::RwLock { id: lock_id, mode }); if let Some(timeout_time) = timeout_time { struct Callback<'tcx> { thread: ThreadId, condvar_id: CondvarId, lock_id: RwLockId, - shared: bool, + mode: RwLockMode, dest: PlaceTy<'tcx, Provenance>, } impl<'tcx> VisitTags for Callback<'tcx> { fn visit_tags(&self, visit: &mut dyn FnMut(SbTag)) { - let Callback { thread: _, condvar_id: _, lock_id: _, shared: _, dest } = self; + let Callback { thread: _, condvar_id: _, lock_id: _, mode: _, dest } = self; dest.visit_tags(visit); } } impl<'mir, 'tcx: 'mir> MachineCallback<'mir, 'tcx> for Callback<'tcx> { fn call(&self, this: &mut MiriInterpCx<'mir, 'tcx>) -> InterpResult<'tcx> { - this.reacquire_cond_lock(self.thread, self.lock_id, self.shared)?; + this.reacquire_cond_lock(self.thread, self.lock_id, self.mode)?; this.condvar_remove_waiter(self.condvar_id, self.thread); @@ -438,7 +445,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> { thread: active_thread, condvar_id, lock_id, - shared, + mode, dest: dest.clone(), }), ); @@ -451,9 +458,13 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> { let this = self.eval_context_mut(); let condvar_id = this.condvar_get_or_create_id(condvar_op, CONDVAR_ID_OFFSET)?; - if let Some((thread, lock, shared)) = this.condvar_signal(condvar_id) { - this.reacquire_cond_lock(thread, RwLockId::from_u32(lock), shared)?; - this.unregister_timeout_callback_if_exists(thread); + if let Some((thread, lock)) = this.condvar_signal(condvar_id) { + if let CondvarLock::RwLock { id, mode } = lock { + this.reacquire_cond_lock(thread, id, mode)?; + this.unregister_timeout_callback_if_exists(thread); + } else { + panic!("mutexes should not exist on windows"); + } } Ok(()) @@ -466,9 +477,13 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> { let this = self.eval_context_mut(); let condvar_id = this.condvar_get_or_create_id(condvar_op, CONDVAR_ID_OFFSET)?; - while let Some((thread, lock, shared)) = this.condvar_signal(condvar_id) { - this.reacquire_cond_lock(thread, RwLockId::from_u32(lock), shared)?; - this.unregister_timeout_callback_if_exists(thread); + while let Some((thread, lock)) = this.condvar_signal(condvar_id) { + if let CondvarLock::RwLock { id, mode } = lock { + this.reacquire_cond_lock(thread, id, mode)?; + this.unregister_timeout_callback_if_exists(thread); + } else { + panic!("mutexes should not exist on windows"); + } } Ok(()) From 2eb07a05a3fa1e604f4243ddd5659f294d6b69b9 Mon Sep 17 00:00:00 2001 From: DrMeepster <19316085+DrMeepster@users.noreply.github.com> Date: Sun, 30 Oct 2022 19:14:45 -0700 Subject: [PATCH 14/14] fix shared behavior and add tests --- src/tools/miri/src/shims/windows/sync.rs | 6 +- .../concurrency/windows_condvar_shared.rs | 227 ++++++++++++++++++ .../concurrency/windows_condvar_shared.stdout | 60 +++++ 3 files changed, 290 insertions(+), 3 deletions(-) create mode 100644 src/tools/miri/tests/pass/concurrency/windows_condvar_shared.rs create mode 100644 src/tools/miri/tests/pass/concurrency/windows_condvar_shared.stdout diff --git a/src/tools/miri/src/shims/windows/sync.rs b/src/tools/miri/src/shims/windows/sync.rs index a34d142f4a5e..8f414d98dba5 100644 --- a/src/tools/miri/src/shims/windows/sync.rs +++ b/src/tools/miri/src/shims/windows/sync.rs @@ -12,7 +12,7 @@ const INIT_ONCE_ID_OFFSET: u64 = 0; const CONDVAR_ID_OFFSET: u64 = 0; impl<'mir, 'tcx> EvalContextExtPriv<'mir, 'tcx> for crate::MiriInterpCx<'mir, 'tcx> {} -pub trait EvalContextExtPriv<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> { +trait EvalContextExtPriv<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> { /// Try to reacquire the lock associated with the condition variable after we /// were signaled. fn reacquire_cond_lock( @@ -26,13 +26,13 @@ pub trait EvalContextExtPriv<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tc match mode { RwLockMode::Read => - if this.rwlock_is_locked(lock) { + if this.rwlock_is_write_locked(lock) { this.rwlock_enqueue_and_block_reader(lock, thread); } else { this.rwlock_reader_lock(lock, thread); }, RwLockMode::Write => - if this.rwlock_is_write_locked(lock) { + if this.rwlock_is_locked(lock) { this.rwlock_enqueue_and_block_writer(lock, thread); } else { this.rwlock_writer_lock(lock, thread); diff --git a/src/tools/miri/tests/pass/concurrency/windows_condvar_shared.rs b/src/tools/miri/tests/pass/concurrency/windows_condvar_shared.rs new file mode 100644 index 000000000000..d89320bfe597 --- /dev/null +++ b/src/tools/miri/tests/pass/concurrency/windows_condvar_shared.rs @@ -0,0 +1,227 @@ +//@only-target-windows: Uses win32 api functions +// We are making scheduler assumptions here. +//@compile-flags: -Zmiri-preemption-rate=0 + +use std::ffi::c_void; +use std::ptr::null_mut; +use std::thread; + +#[derive(Copy, Clone)] +struct SendPtr(*mut T); + +unsafe impl Send for SendPtr {} + +extern "system" { + fn SleepConditionVariableSRW( + condvar: *mut *mut c_void, + lock: *mut *mut c_void, + timeout: u32, + flags: u32, + ) -> i32; + fn WakeAllConditionVariable(condvar: *mut *mut c_void); + + fn AcquireSRWLockExclusive(lock: *mut *mut c_void); + fn AcquireSRWLockShared(lock: *mut *mut c_void); + fn ReleaseSRWLockExclusive(lock: *mut *mut c_void); + fn ReleaseSRWLockShared(lock: *mut *mut c_void); +} + +const CONDITION_VARIABLE_LOCKMODE_SHARED: u32 = 1; +const INFINITE: u32 = u32::MAX; + +/// threads should be able to reacquire the lock while it is locked by multiple other threads in shared mode +fn all_shared() { + println!("all_shared"); + + let mut lock = null_mut(); + let mut condvar = null_mut(); + + let lock_ptr = SendPtr(&mut lock); + let condvar_ptr = SendPtr(&mut condvar); + + let mut handles = Vec::with_capacity(10); + + // waiters + for i in 0..5 { + handles.push(thread::spawn(move || { + unsafe { + AcquireSRWLockShared(lock_ptr.0); + } + println!("exclusive waiter {i} locked"); + + let r = unsafe { + SleepConditionVariableSRW( + condvar_ptr.0, + lock_ptr.0, + INFINITE, + CONDITION_VARIABLE_LOCKMODE_SHARED, + ) + }; + assert_ne!(r, 0); + + println!("exclusive waiter {i} reacquired lock"); + + // unlocking is unnecessary because the lock is never used again + })); + } + + // ensures each waiter is waiting by this point + thread::yield_now(); + + // readers + for i in 0..5 { + handles.push(thread::spawn(move || { + unsafe { + AcquireSRWLockShared(lock_ptr.0); + } + println!("reader {i} locked"); + + // switch to next reader or main thread + thread::yield_now(); + + unsafe { + ReleaseSRWLockShared(lock_ptr.0); + } + println!("reader {i} unlocked"); + })); + } + + // ensures each reader has acquired the lock + thread::yield_now(); + + unsafe { + WakeAllConditionVariable(condvar_ptr.0); + } + + for handle in handles { + handle.join().unwrap(); + } +} + +// reacquiring a lock should wait until the lock is not exclusively locked +fn shared_sleep_and_exclusive_lock() { + println!("shared_sleep_and_exclusive_lock"); + + let mut lock = null_mut(); + let mut condvar = null_mut(); + + let lock_ptr = SendPtr(&mut lock); + let condvar_ptr = SendPtr(&mut condvar); + + let mut waiters = Vec::with_capacity(5); + for i in 0..5 { + waiters.push(thread::spawn(move || { + unsafe { + AcquireSRWLockShared(lock_ptr.0); + } + println!("shared waiter {i} locked"); + + let r = unsafe { + SleepConditionVariableSRW( + condvar_ptr.0, + lock_ptr.0, + INFINITE, + CONDITION_VARIABLE_LOCKMODE_SHARED, + ) + }; + assert_ne!(r, 0); + + println!("shared waiter {i} reacquired lock"); + + // unlocking is unnecessary because the lock is never used again + })); + } + + // ensures each waiter is waiting by this point + thread::yield_now(); + + unsafe { + AcquireSRWLockExclusive(lock_ptr.0); + } + println!("main locked"); + + unsafe { + WakeAllConditionVariable(condvar_ptr.0); + } + + // waiters are now waiting for the lock to be unlocked + thread::yield_now(); + + unsafe { + ReleaseSRWLockExclusive(lock_ptr.0); + } + println!("main unlocked"); + + for handle in waiters { + handle.join().unwrap(); + } +} + +// threads reacquiring locks should wait for all locks to be released first +fn exclusive_sleep_and_shared_lock() { + println!("exclusive_sleep_and_shared_lock"); + + let mut lock = null_mut(); + let mut condvar = null_mut(); + + let lock_ptr = SendPtr(&mut lock); + let condvar_ptr = SendPtr(&mut condvar); + + let mut handles = Vec::with_capacity(10); + for i in 0..5 { + handles.push(thread::spawn(move || { + unsafe { + AcquireSRWLockExclusive(lock_ptr.0); + } + + println!("exclusive waiter {i} locked"); + + let r = unsafe { SleepConditionVariableSRW(condvar_ptr.0, lock_ptr.0, INFINITE, 0) }; + assert_ne!(r, 0); + + println!("exclusive waiter {i} reacquired lock"); + + // switch to next waiter or main thread + thread::yield_now(); + + unsafe { + ReleaseSRWLockExclusive(lock_ptr.0); + } + println!("exclusive waiter {i} unlocked"); + })); + } + + for i in 0..5 { + handles.push(thread::spawn(move || { + unsafe { + AcquireSRWLockShared(lock_ptr.0); + } + println!("reader {i} locked"); + + // switch to next reader or main thread + thread::yield_now(); + + unsafe { + ReleaseSRWLockShared(lock_ptr.0); + } + println!("reader {i} unlocked"); + })); + } + + // ensures each reader has acquired the lock + thread::yield_now(); + + unsafe { + WakeAllConditionVariable(condvar_ptr.0); + } + + for handle in handles { + handle.join().unwrap(); + } +} + +fn main() { + all_shared(); + shared_sleep_and_exclusive_lock(); + exclusive_sleep_and_shared_lock(); +} diff --git a/src/tools/miri/tests/pass/concurrency/windows_condvar_shared.stdout b/src/tools/miri/tests/pass/concurrency/windows_condvar_shared.stdout new file mode 100644 index 000000000000..918b54668f20 --- /dev/null +++ b/src/tools/miri/tests/pass/concurrency/windows_condvar_shared.stdout @@ -0,0 +1,60 @@ +all_shared +exclusive waiter 0 locked +exclusive waiter 1 locked +exclusive waiter 2 locked +exclusive waiter 3 locked +exclusive waiter 4 locked +reader 0 locked +reader 1 locked +reader 2 locked +reader 3 locked +reader 4 locked +exclusive waiter 0 reacquired lock +exclusive waiter 1 reacquired lock +exclusive waiter 2 reacquired lock +exclusive waiter 3 reacquired lock +exclusive waiter 4 reacquired lock +reader 0 unlocked +reader 1 unlocked +reader 2 unlocked +reader 3 unlocked +reader 4 unlocked +shared_sleep_and_exclusive_lock +shared waiter 0 locked +shared waiter 1 locked +shared waiter 2 locked +shared waiter 3 locked +shared waiter 4 locked +main locked +main unlocked +shared waiter 0 reacquired lock +shared waiter 1 reacquired lock +shared waiter 2 reacquired lock +shared waiter 3 reacquired lock +shared waiter 4 reacquired lock +exclusive_sleep_and_shared_lock +exclusive waiter 0 locked +exclusive waiter 1 locked +exclusive waiter 2 locked +exclusive waiter 3 locked +exclusive waiter 4 locked +reader 0 locked +reader 1 locked +reader 2 locked +reader 3 locked +reader 4 locked +reader 0 unlocked +reader 1 unlocked +reader 2 unlocked +reader 3 unlocked +reader 4 unlocked +exclusive waiter 0 reacquired lock +exclusive waiter 0 unlocked +exclusive waiter 1 reacquired lock +exclusive waiter 1 unlocked +exclusive waiter 2 reacquired lock +exclusive waiter 2 unlocked +exclusive waiter 3 reacquired lock +exclusive waiter 3 unlocked +exclusive waiter 4 reacquired lock +exclusive waiter 4 unlocked