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

Miscompilation of code in or around Iterator::position #161

Closed
benhansen-io opened this issue Feb 16, 2023 · 4 comments
Closed

Miscompilation of code in or around Iterator::position #161

benhansen-io opened this issue Feb 16, 2023 · 4 comments

Comments

@benhansen-io
Copy link

I tried this code (also available at https://github.com/benhansen-io/esp-miscompile-test)

use esp_idf_sys as _; // If using the `libstart` feature of `esp-idf-sys`, always keep this module imported

#[derive(Copy, Clone, Debug, Default, Eq, PartialEq, Ord, PartialOrd, Hash)]
pub struct RGB {
    r: u8,
    g: u8,
    b: u8,
}

impl RGB {
    fn new(r: u8, g: u8, b: u8) -> RGB {
        RGB { r, g, b }
    }
}

fn next_color(curr: RGB, order: &[RGB]) -> RGB {
    let curr_idx = order
        .iter()
        .position(|c| {
            // println!("UNCOMMENT TO STOP PANIC");
            *c == curr
        })
        .unwrap();
    let next_idx = if curr_idx == order.len() - 1 {
        0
    } else {
        curr_idx + 1
    };
    order[next_idx]
}

#[no_mangle]
extern "C" fn rust_main() -> i32 {
    // Temporary. Will disappear once ESP-IDF 4.4 is released, but for now it is necessary to call this function once,
    // or else some patches to the runtime implemented by esp-idf-sys might not link properly.
    esp_idf_sys::link_patches();

    let green: RGB = RGB::new(0, 255, 0);
    let blue: RGB = RGB::new(0, 0, 255);
    let yellow: RGB = RGB::new(255, 255, 0);

    let color_order = [yellow, blue, green];
    let mut curr = yellow;
    println!("Starting");
    loop {
        println!("curr: {:?}", curr);
        curr = next_color(curr, &color_order);
        println!("next: {:?}", curr);
        std::thread::sleep(std::time::Duration::from_millis(500));
    }
}

I expected to see this happen:

2023-02-15 20:44:15 Hello world from C!
2023-02-15 20:44:15 Starting
2023-02-15 20:44:15 curr: RGB { r: 255, g: 255, b: 0 }
2023-02-15 20:44:15 next: RGB { r: 0, g: 0, b: 255 }
2023-02-15 20:44:15 curr: RGB { r: 0, g: 0, b: 255 }
2023-02-15 20:44:16 next: RGB { r: 0, g: 255, b: 0 }
2023-02-15 20:44:16 curr: RGB { r: 0, g: 255, b: 0 }
2023-02-15 20:44:16 next: RGB { r: 255, g: 255, b: 0 }
2023-02-15 20:44:16 curr: RGB { r: 255, g: 255, b: 0 }
...

Instead, this happened:

2023-02-15 20:43:28 Hello world from C!                                                                                                                         
2023-02-15 20:43:28 Starting                                                                                                                                    
2023-02-15 20:43:28 curr: RGB { r: 255, g: 255, b: 0 }                                                                                                          
2023-02-15 20:43:28 next: RGB { r: 0, g: 0, b: 255 }                                                                                                            
2023-02-15 20:43:28 curr: RGB { r: 0, g: 0, b: 255 }                                                                                                            
2023-02-15 20:43:28 ESP-ROM:esp32s3-20210327
2023-02-15 20:43:29 curr: RGB { r: 0, g: 0, b: 255 }
2023-02-15 20:43:29 ESP-ROM:esp32s3-20210327
2023-02-15 20:43:30 curr: RGB { r: 0, g: 0, b: 255 }
2023-02-15 20:43:30 ESP-ROM:esp32s3-20210327
...

Without panic abort immediate I also see:

2023-02-15 20:49:27 thread '<unnamed>' panicked at 'called `Option::unwrap()` on a `None` value', src/lib.rs:23:10

Adding the commented out println! or removing the opt-level = "s" both cause the correct output to be displayed.

Meta

rustc --version --verbose:

rustc 1.65.0 (897e37553 2022-11-02)
binary: rustc
commit-hash: 897e37553bba8b42751c67658967889d11ecd120
commit-date: 2022-11-02
host: x86_64-unknown-linux-gnu
release: 1.65.0
LLVM version: 15.0.0

(tested on 1.67.0 too)

@MabezDev
Copy link
Member

Thanks for the report! What are the results if you add -Ctarget-feature=-loop to rustflags and do a clear build?

@benhansen-io
Copy link
Author

If I add "-Ctarget-feature=-loop" to RUSTFLAGS then the results are as expected (no mis-compilation). Verified on the reproduction repo.

@MabezDev
Copy link
Member

Sorry about that, we have an ongoing issue with the Xtensa backend around the loop optimization feature of the ESP32-S3 and ESP32. I don't think we have a tracking issue for it on esp-rs/rust so we can use this one until its resolved :)

@MabezDev
Copy link
Member

With the 1.69 release, this should now be fixed. Please let me know if you still run into issues!

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

2 participants