discovery icon indicating copy to clipboard operation
discovery copied to clipboard

Modifying locals potentially broken with recent Rust

Open netzdoktor opened this issue 4 years ago • 7 comments

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.

netzdoktor avatar May 13 '20 08:05 netzdoktor

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 avatar May 21 '20 15:05 wupdiduh

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.

wupdiduh avatar May 22 '20 12:05 wupdiduh

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.

netzdoktor avatar May 22 '20 13:05 netzdoktor

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.

yerke avatar Jul 01 '20 05:07 yerke

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).

mbrubeck avatar Nov 29 '20 20:11 mbrubeck

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

migueloller avatar Dec 01 '20 22:12 migueloller

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)

winksaville avatar Dec 23 '20 06:12 winksaville