Clippy suggests optimizing volatile operations away; Shoudln't do so?

[andre-richter]

Hi,

given the following code sample, we read from an MMIO timer that has high and low parts.
If a wrap in low occurred soon after we read it, read the timers again.

extern crate volatile_register;

use volatile_register::RW;

#[repr(C)]
pub struct MMIO {
    pub high: RW<u32>,
    pub low: RW<u32>,
}

fn main() {
    let timer = 0x1337 as *const MMIO;

    unsafe {
        let mut hi = (*timer).high.read();
        let mut lo = (*timer).low.read();

        // Repeat if high word changed during read.
        if hi != (*timer).high.read() {
            hi = (*timer).high.read();
            lo = (*timer).low.read();
        }

        println!("{}", (u64::from(hi) << 32) | u64::from(lo));
    }
}

Clippy suggest alternative code in this case.

    Checking vcell v0.1.0                                                                           
    Checking volatile-register v0.2.0
    Checking volatile_clippy v0.1.0 (file:///home/andre/repos/volatile_clippy)
warning: `if _ { .. } else { .. }` is an expression
  --> src/main.rs:16:9
   |
16 | /         let mut lo = (*timer).low.read();
17 | |
18 | |         // Repeat if high word changed during read.
19 | |         if hi != (*timer).high.read() {
20 | |             hi = (*timer).high.read();
21 | |             lo = (*timer).low.read();
22 | |         }
   | |_________^ help: it is more idiomatic to write: `let <mut> lo = if hi != (*timer).high.read() { ..; (*timer).low.read() } else { (*timer).low.read() };`
   |
   = note: #[warn(useless_let_if_seq)] on by default
   = note: you might not need `mut` at all
   = help: for further information visit https://rust-lang-nursery.github.io/rust-clippy/v0.0.212/index.html#useless_let_if_seq

Using this suggestion would actually remove the first MMIO read of low. Since volatile instructs the compiler to not reorder volatile operations across each other and/or elide volatile operations, clippy shouldn't do so as well?

I understand this case might be difficult because the volatile hides in an extern crate.

[oli-obk]

We should probably disable this lint for values with interior mutability

[ghost]

How do you tell if a type has interior mutability?

[oli-obk]

there's an is_freeze function on Ty: https://doc.rust-lang.org/nightly/nightly-rustc/rustc/ty/struct.TyS.html#method.is_freeze

if false, the type has interior mutability. There might still be values like Option::<Cell<u32>>::None which have no interior mutability, but we can ignore that.

[JoshMcguigan]

I tried to reproduce this with a RefCell in the playground, and this lint was not activated.

https://play.rust-lang.org/?gist=2667edcc6275927a006ac361a45e4448&version=nightly&mode=debug&edition=2015

@oli-obk Can you post a playground link illustrating what you were thinking?

[oli-obk]

The minimal example I was thinking about is https://play.rust-lang.org/?gist=eda67f7012d15ec25d0baaaddb275973&version=nightly&mode=debug&edition=2015

[JoshMcguigan]

I'll take a shot at this. Hopefully I'll have a PR ready in the next few days.

[andre-richter]

Hi @JoshMcguigan,

I just tested your code with https://github.com/rust-embedded/rust-raspi3-OS-tutorials/tree/master/09_delays using make clippy. You may need some tools installed as described in https://github.com/rust-embedded/rust-raspi3-OS-tutorials#prerequisites to make it work.
This code was the reason for me opening this issue originally. I still get the warning:

~/repos/rust-raspi3-OS-tutorials/09_delays [master*] » cargo clippy -V
clippy 0.0.212 (aeadf15 2019-09-03)
~/repos/rust-raspi3-OS-tutorials/09_delays [master*] » make clippy
cargo xclippy --target=aarch64-unknown-none
...
warning: `if _ { .. } else { .. }` is an expression
  --> src/delays.rs:69:9
   |
69 | /         let mut lo = self.SYSTMR_LO.get();
70 | |
71 | |         // We have to repeat if high word changed during read. This
72 | |         // will emit a clippy warning that needs be ignored, or you
...  |
76 | |             lo = self.SYSTMR_LO.get();
77 | |         }
   | |_________^ help: it is more idiomatic to write: `let <mut> lo = if hi != self.SYSTMR_HI.get() { ..; self.SYSTMR_LO.get() } else { self.SYSTMR_LO.get() };`
   |
   = note: `#[warn(clippy::useless_let_if_seq)]` on by default
   = note: you might not need `mut` at all
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#useless_let_if_seq

    Finished dev [unoptimized + debuginfo] target(s) in 1.01s

Could you have a look again? Please note that the voltaile nature of reading SYSTMR_ hides between extern crates and macros that might be difficult to follow at first.

[JoshMcguigan]

Hi @andre-richter,

Sorry this wasn't able to resolve your issue. It seems we must have missed something when turning your example into a minimal example which could be included as a test in the clippy code base, but I don't know enough about the compiler internals to know what that would be.

If you can provide an example using only the standard library (no external libs) then we can add that to the clippy test suite to be sure it solves your problem.

Alternatively perhaps @oli-obk has some insight. Or maybe this issue should just be re-opened?

[andre-richter]

Hi @JoshMcguigan,

I think this playground should do.

Thanks for having a look 😊

[flip1995]

Thanks for the minimal reproducer! Reopening the issue.

[phansch]

Triage: The lint now is allow by default but this particular issue seems to have been fixed at some point: https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=b60090a798f6723d1a08d44d2b58bc50

I'm going to go ahead and close this issue but feel free to re-open it, should the issue persist for you.