Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Modifying locals potentially broken with recent Rust #236

Open
netzdoktor opened this issue May 13, 2020 · 7 comments
Open

Modifying locals potentially broken with recent Rust #236

netzdoktor opened this issue May 13, 2020 · 7 comments

Comments

@netzdoktor
Copy link

I'm working through the discovery book and just did the gdb session in this chapter.

What I found is that while info locals properly reports my change to half_period, the behaviour did not change. So I went through calls to the delay_ms methods again and found that the calling parameter is still the original value of 500.

What I am wondering is if this is due to Rust recent changes with respect to constant value optimizations. Could it be that the half_period (which is a primitive type and only read twice and never written) is optimized away? I can imagine that it would be sufficient to write the value somewhere to the binary and let delay_ms access it directly without doing the usual stack-frame handling.

My questions are:

  • Can you give me hints on how to validate my hypothesis?
  • Can we (jointly) fix this by modifying the usage or declaration of half_period so that it is really modifiable interactively in gdb?

I guess fixing this would save others from trying out this example and wondering why it does not work.

@wupdiduh
Copy link

wupdiduh commented May 21, 2020

I have the same issue.
Stepping through the program in disassemble view (as described in 5.3. Debug it) I found the instructions mov.w r0, #500 and strh.w r0, [r7, #-2], which I think store the value of half_period in memory.
However, right before the subroutine for function delay.delay_ms is called the raw value is moved to a register mov.w r1, #500.

I am pretty new to the topic, but as far as I understand my findings, I think your assumption is correct that the compiler is too smart in this case.
So far I haven't tried to find a solution or workaround, but I agree that fixing this would help prevent some confusion.

@wupdiduh
Copy link

I played around with it some more and found that declaring half_period as mutable and reassigning its value once solves the issue.

let mut half_period = 0_u16;
half_period = 500_u16;

As much as I think playing around with this feature at that point in the tutorial is very interesting, I am not sure it justifies using strange code like that - unless it is explained in some detail what is happening without it.

@netzdoktor
Copy link
Author

Thanks for looking into assembly and confirming my hypothesis. Your code makes sense but is indeed "strange".

I wondered if one could make the variable volatile as it is also done in the register section of the book.

I found the volatile crate, which could be used for that. Still, a developer would not per-se make this variable volatile, as it actually is not expected to change without doing live-rewrite in the debugger. But that would at least provide similar "changability" while having less contrived code.

Maybe leveraging volatile and explaining the details in a "note"-box would educate the reader and make the example work again.

@yerke
Copy link
Contributor

yerke commented Jul 1, 2020

I also just ran into this issue. @wupdiduh thanks for finding a workaround. I had to modify it a bit:

let mut half_period = 500_u16;
delay.delay_ms(half_period);
half_period = 100;

loop {

I had to use half_period before changing the value, so the compiler wouldn't optimize it away.

@mbrubeck
Copy link

From a thread in the Rust forum:

I believe this is a result of MIR constant propagation, which is done by rustc before it generates LLVM IR and runs LLVM's optimization passes. In recent versions of rustc, MIR constant propagation is enabled by default in debug mode because it reduces compile times. You can disable it using the -Zmir-opt-level=0 rustc flag (requires a nightly build of the Rust toolchain).

@migueloller
Copy link

Unfortunately using -Zmir-opt-level doesn't work. I've only been able to get around the optimization by doing what @yerke did.

@winksaville
Copy link
Contributor

Here are a couple other solutions I came up with.

  1. Use the volatile crate:

Cargo.toml to add volatile = "0.4.2" as a dependency:

[dependencies]
aux5 = { path = "auxiliary" }
volatile = "0.4.2"

And the main.rs is now:

#![deny(unsafe_code)]
#![no_main]
#![no_std]

use aux5::{entry, prelude::*, Delay, Leds};
use volatile::Volatile;

#[entry]
fn main() -> ! {
    let (mut delay, mut leds): (Delay, Leds) = aux5::init();

    let mut half_period = 2000_u16;
    let v_half_period = Volatile::new(&mut half_period);

    loop {
        leds[0].on();
        delay.delay_ms(v_half_period.read());

        leds[0].off();
        delay.delay_ms(v_half_period.read());
    }
}
  1. Use Atomics:
#![deny(unsafe_code)]
#![no_main]
#![no_std]

use aux5::{entry, prelude::*, Delay, Leds};
use core::sync::atomic::{AtomicU16, Ordering};

#[entry]
fn main() -> ! {
    let (mut delay, mut leds): (Delay, Leds) = aux5::init();

    let half_period = AtomicU16::new(2000_u16);

    loop {
        leds[0].on();
        delay.delay_ms(half_period.load(Ordering::Relaxed));

        leds[0].off();
        delay.delay_ms(half_period.load(Ordering::Relaxed));
    }
}

Change half_period with:

(gdb) set half_period = core::sync::atomic::AtomicU16::new(100)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants