`if` ranching condition variable masked if `else` leg empty (AVR ?)

[Frankkkkk]

Hello,

The code is here.

Basically, it consists of:

static mut CTR: RefCell<u8> = RefCell::new(0);


#[avr_device::interrupt(atmega328p)]
fn TIMER0_OVF() {
    interrupt::free(|_cs| unsafe {
        let ctr = CTR.get_mut();
        let mut inc = *ctr + 1;
        if inc > 254 {
            inc = 1;
        }
        *ctr = inc
    });
}


#[avr_device::entry]
fn main() -> ! {
    ....
    unsafe { asm!("444444:"); }
    loop {
        let wait_qty: u8;
        unsafe { wait_qty = *CTR.get_mut(); };

        if wait_qty > 0 {
            dp.PORTD.portd.modify(|_, w| w.pd2().set_bit());
        }
        else {
            //unsafe { asm!("nop") }
        }
    }
}

I expected to see this happen:
Basically, at the start of the program, CTR is 0 and thus the else branch is executed inside the loop.
After some time, an interrupt is raised, and CTR is set to 1, and the if branch should be run (and an LED turned on).

Instead, this happened:
The LED never lights up. However, if I add an instruction in the else branch (like a nop), then the code runs as expected (the LED turns on afterwards)

Meta

rustc --version --verbose:

   Compiling mega328-test v0.1.0 (/home/frank/src/github.com/Rahix/avr-device/examples/atmega328p)
rustc 1.77.0-nightly (e51e98dde 2023-12-31)
binary: rustc
commit-hash: e51e98dde6a60637b6a71b8105245b629ac3fe77
commit-date: 2023-12-31
host: x86_64-unknown-linux-gnu
release: 1.77.0-nightly
LLVM version: 17.0.6
    Finished dev [optimized + debuginfo] target(s) in 0.06s

For the sake of simplicity, I created two examples:

• GOOD, with a nop in the else branch -> LED turns on.
• BAD with nothing on the else branch -> LED stays off.

I tried to debug the compilation, and I see that:

MIR

The runtime optimized after MIR (006.000) looks okay on both cases:

GOOD:

MIR excerpt

    bb14: {
        StorageLive(_25);
        StorageLive(_26);
        StorageLive(_27);
        _27 = const {alloc1: *mut RefCell<u8>};
        _26 = &mut (*_27);
        _25 = RefCell::<u8>::get_mut(move _26) -> [return: bb15, unwind unreachable];
    }

    bb15: {
        StorageDead(_26);
        _24 = (*_25);
        StorageDead(_27);
        StorageDead(_25);
        StorageLive(_28);
        _28 = Gt(_24, const 0_u8);
        switchInt(move _28) -> [0: bb19, otherwise: bb16];
    }

    bb16: {
        StorageLive(_30);
        StorageLive(_31);
        StorageLive(_32);
        _32 = &(_1.9: avr_device::atmega328p::PORTD);
        _31 = <PORTD as Deref>::deref(move _32) -> [return: bb17, unwind unreachable];
    }

    bb17: {
        StorageDead(_32);
        _30 = &((*_31).2: avr_device::generic::Reg<avr_device::atmega328p::portd::portd::PORTD_SPEC>);
        _29 = Reg::<PORTD_SPEC>::modify::<[closure@src/main.rs:58:35: 58:41]>(move _30, const ZeroSized: [closure@src/main.rs:58:35: 58:41]) -> [return: bb18, unwind unreachable];
    }

    bb18: {
        StorageDead(_30);
        StorageDead(_31);
        goto -> bb20;
    }

    bb19: {
        asm!("444444:", options((empty))) -> [return: bb20, unwind unreachable];
    }

    bb20: {
        StorageDead(_28);
        goto -> bb14;
    }

BAD:

MIR excerpt

   bb14: {
        StorageLive(_25);
        StorageLive(_26);
        StorageLive(_27);
        _27 = const {alloc1: *mut RefCell<u8>};
        _26 = &mut (*_27);
        _25 = RefCell::<u8>::get_mut(move _26) -> [return: bb15, unwind unreachable];
    }

    bb15: {
        StorageDead(_26);
        _24 = (*_25);
        StorageDead(_27);
        StorageDead(_25);
        StorageLive(_28);
        _28 = Gt(_24, const 0_u8);
        switchInt(move _28) -> [0: bb19, otherwise: bb16];
    }

    bb16: {
        StorageLive(_30);
        StorageLive(_31);
        StorageLive(_32);
        _32 = &(_1.9: avr_device::atmega328p::PORTD);
        _31 = <PORTD as Deref>::deref(move _32) -> [return: bb17, unwind unreachable];
    }

    bb17: {
        StorageDead(_32);
        _30 = &((*_31).2: avr_device::generic::Reg<avr_device::atmega328p::portd::portd::PORTD_SPEC>);
        _29 = Reg::<PORTD_SPEC>::modify::<[closure@src/main.rs:58:35: 58:41]>(move _30, const ZeroSized: [closure@src/main.rs:58:35: 58:41]) -> [return: bb18, unwind unreachable];
    }

    bb18: {
        StorageDead(_30);
        StorageDead(_31);
        goto -> bb19;
    }

    bb19: {
        StorageDead(_28);
        goto -> bb14;
    }

Basically, the diff is in bb18/bb19

LLVM-IR:

GOOD:

LLVM-IR excerpt

  br label %bb14, !dbg !3347
bb14:                                             ; preds = %bb19, %bb16, %"_ZN4core6option15Option$LT$T$GT$6unwrap17h450249f892692cf5E.exit"
  %wait_qty = load i8, ptr @_ZN12mega328_test3CTR17hd094fa9ca712b679E.0, align 1, !dbg !3349, !noundef !358
  call addrspace(1) void @llvm.dbg.value(metadata i8 %wait_qty, metadata !2947, metadata !DIExpression()), !dbg !3350
  %_28.not = icmp eq i8 %wait_qty, 0, !dbg !3351
  br i1 %_28.not, label %bb19, label %bb16, !dbg !3351

BAD:

LLVM-IR excerpt

  %wait_qty.pre1 = load i8, ptr @_ZN12mega328_test3CTR17hd094fa9ca712b679E.0, align 1, !dbg !3349
  br label %bb14, !dbg !3347

bb14:                                             ; preds = %bb14, %bb16, %"_ZN4core6option15Option$LT$T$GT$6unwrap17h450249f892692cf5E.exit"
  %wait_qty = phi i8 [ %wait_qty.pre1, %"_ZN4core6option15Option$LT$T$GT$6unwrap17h450249f892692cf5E.exit" ], [ %wait_qty.pre, %bb16 ], [ 0, %bb14 ], !dbg !3349
  call addrspace(1) void @llvm.dbg.value(metadata i8 %wait_qty, metadata !2947, metadata !DIExpression()), !dbg !3350
  %_28.not = icmp eq i8 %wait_qty, 0, !dbg !3351
  br i1 %_28.not, label %bb14, label %bb16, !dbg !3351

I don't really know LLVM, but the phi i8 that uses [0, %bb14] looks strange to me, making the condition after always true ?

Assembly

BAD:

    loop {
        let wait_qty: u8;
        unsafe { wait_qty = *CTR.get_mut(); };
  24:   80 91 00 00     lds     r24, 0x0000     ; 0x800000 <__SREG__+0x7fffc1>

        if wait_qty > 0 {
  28:   80 30           cpi     r24, 0x00       ; 0
  2a:   81 2d           mov     r24, r1
  2c:   01 f0           breq    .+0             ; 0x2e <_ZN12mega328_test20__avr_device_rt_main17h8ca9fc0fd7a09522E+0x2e>
  2e:   5a 9a           sbi     0x0b, 2 ; 11
  30:   00 c0           rjmp    .+0             ; 0x32 <_ZN12mega328_test20__avr_device_rt_main17h8ca9fc0fd7a09522E+0x32>

To be honest I don't really understand instruction 24; why would it try to load from 0x0000 ?

What do you think ?

Many thanks for your help !

[krtab]

Hi,
I believe your code is unsound. You are taking &mut to a static mut in two different and concurrent execution paths (the interrupt and the main). The compiler is allowed to assume that CTR won't change inside your main loop and hence read it only once, which give the else elision.
In general, you should really avoid static mut. What you want here is likely AtomicU8.

More info here and in other liked issues: #114447

[Frankkkkk]

Makes sense; thanks !