Add atomic min and max

[henryboisdequin]

Closes #1718
Previous attempt: #1653

TODO:

• Merge atomic_op and atomic_min_max functions
• Fix CI

Note: this PR also removes arbitrary trailing whitespace and generally formats the affected files

[RalfJung]

Thanks a lot for the PR. :)

this PR also removes arbitrary trailing whitespace and generally formats the affected files

This causes quite a lot of noise, which makes review harder. Can you make the formatting changes a separate commit in the PR, or (even better) create a separate format-only PR? (Please don't rustfmt the entire repository, only these two files. There are some other files where rustfmt does a bad job, so I am waiting for some rustfmt fixes before formatting the entire repository.)

[henryboisdequin]

@RalfJung As you suggested in this comment, I've merged the atomic_op and atomic_min_max functions by using this enum:

enum AtomicOp {
    MirOp(mir::BinOp, bool),
    Max,
    Min,
}

Also, how do I fix the CI errors (I'm not sure how to should I implement the trait the diagnostic suggested -the code with errors was from the previous PR-)?

[RalfJung]

(I'm afraid I will probably not be able to take a look at the CI errors at least until the weekend, I am quite busy until next Tuesday.)

[henryboisdequin]

@oli-obk Do you know what the problem is?

Error from CI:

normalized stderr:
thread 'main' panicked at 'assertion failed: `(left == right)`
  left: `8192`,
 right: `4096`', $DIR/atomic.rs:88:5
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace


expected stderr:


diff of stderr:

+thread 'main' panicked at 'assertion failed: `(left == right)`
+  left: `8192`,
+ right: `4096`', $DIR/atomic.rs:88:5
+note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
+

The actual stderr differed from the expected stderr.
Actual stderr saved to /tmp/compiletestswsV4Q/atomic.stderr
To update references, run this command from build directory:
tests/run-pass/update-references.sh '/tmp/compiletestswsV4Q' 'atomic.rs'

error: 1 errors occurred comparing output.
status: exit code: 101
command: "/home/runner/work/miri/miri/target/x86_64-unknown-linux-gnu/release/miri" "tests/run-pass/atomic.rs" "-L" "/tmp/compiletestswsV4Q" "--target=x86_64-unknown-linux-gnu" "--error-format" "json" "-C" "prefer-dynamic" "-o" "/tmp/compiletestswsV4Q/atomic.stage-id" "-A" "unused" "--edition" "2018" "-Dwarnings" "-Dunused" "--sysroot" "/home/runner/.cache/miri/HOST" "-L" "/tmp/compiletestswsV4Q/atomic.stage-id.aux"
stdout:
------------------------------------------

------------------------------------------
stderr:
------------------------------------------
thread 'main' panicked at 'assertion failed: `(left == right)`
  left: `8192`,
 right: `4096`', tests/run-pass/atomic.rs:88:5
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

------------------------------------------

test [ui] run-pass/atomic.rs ... FAILED

[henryboisdequin]

@oli-obk CI still fails though...

[oli-obk]

Yea, that's a regular build error, I approved the general code. Looks like you have references when a noon-reference was expected and an ImmTy instead of an Immedidate. If you deref an ImmTy you get an Immediate.

[henryboisdequin]

@oli-obk Understood! I didn't know that, thanks. I've changed the code accordingly.

[oli-obk]

Just out of curiosity: you are not building locally at all?

[henryboisdequin]

Just out of curiosity: you are not building locally at all?

@oli-obk No not really, I will next time though. Not sure why I'm not.

Edit:

Well, I kind of do have a reason: it takes more time to build locally than to look at the CI.

[RalfJung]

this PR also removes arbitrary trailing whitespace and generally formats the affected files

This causes quite a lot of noise, which makes review harder. Can you make the formatting changes a separate commit in the PR, or (even better) create a separate format-only PR? (Please don't rustfmt the entire repository, only these two files. There are some other files where rustfmt does a bad job, so I am waiting for some rustfmt fixes before formatting the entire repository.)

This concern is still open, isn't it? Review is made difficult by having formatting changes and actual behavior changes in the same commit.

[oli-obk]

Well, I kind of do have a reason: it takes more time to build locally than to look at the CI.

from scratch I understand that, but if you have that on every build, we should look into that.

Also, you still have some compilation failures.

[RalfJung]

@henryboisdequin We described in our contributors guide how to set up an environment for local builds. If you have any questions about that, feel free to ask. :)

[henryboisdequin]

this PR also removes arbitrary trailing whitespace and generally formats the affected files

This causes quite a lot of noise, which makes review harder. Can you make the formatting changes a separate commit in the PR, or (even better) create a separate format-only PR? (Please don't rustfmt the entire repository, only these two files. There are some other files where rustfmt does a bad job, so I am waiting for some rustfmt fixes before formatting the entire repository.)

This concern is still open, isn't it? Review is made difficult by having formatting changes and actual behavior changes in the same commit.

@RalfJung Do you the best way to put all the formatting changes into one commit efficently?

[RalfJung]

You could try starting with master, doing the formatting, committing that, and then rebasing the remaining changes on top of that and hope that git conflict resolution works well. There's more advanced alternatives but they are harder to explain.^^

[henryboisdequin]

@RalfJung Done, this is ready for review

[oli-obk]

The build is still failing, maybe the rebase caused some match arms to get duplicated?

[henryboisdequin]

@oli-obk Thanks for the reminder, I was wondering why the diff was larger than before but didn't look into it 🤔

[henryboisdequin]

Hmm, tests still fail @RalfJung @oli-obk. Same problem as at first

[oli-obk]

I think you can squash all commits after the second one into the second one, as they are just going back and forth

[henryboisdequin]

@oli-obk Commits squashed ✅

[oli-obk]

@bors r+

[bors]

📌 Commit f8440d6 has been approved by oli-obk

[bors]

⌛ Testing commit f8440d6 with merge 2670839...

[bors]

☀️ Test successful - checks-actions
Approved by: oli-obk
Pushing 2670839 to master...