Rahix / avr-device

Register access crate for AVR microcontrollers
Apache License 2.0
168 stars 67 forks source link

Possible rust compilation issue ? #145

Closed Frankkkkk closed 6 months ago

Frankkkkk commented 6 months ago

Hi,

I'm posting here first because I have an unexpected result on some code I wrote. It looks like a compiler issue but as I'm learning Rust, maybe it's just me doing dumb things :D. Could you please tell me is this is a misusage on my end before I raise the issue upstream ?

The code is here.

Basically, it consists of a RefCell that is written in an interrupt, and read on the main loop. The initial value is 0, and set to > 0 on the interrup. The main loop turns on the LED if the value is >0. So it starts off and then turns on.

(sorry for the rather strange code; I started removing logic to make a minimum MVP)

However, when there is no code on the else branch, the LED never turns on. If we put some code (like a nop, or something else) then it works as expected.

For the sake of simplicity, I created two examples:

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

MIR

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

GOOD:

    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:

   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:

  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:

  %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 !

Rahix commented 6 months ago

Hey, I think you should post this issue on the rustc repository, it looks like a compiler topic indeed.

Frankkkkk commented 6 months ago

Thanks ! I've created the issue https://github.com/rust-lang/rust/issues/119572 . I'll close this one for now then :-)

Cheers