Skip to content

Conversation

@eira-fransham
Copy link

Small PR to ensure that the range information is persisted when getting the value. This allows bounded-integer to be used not just for correctness, but for optimisation too.

@Kestrer
Copy link
Owner

Kestrer commented Sep 23, 2025

This seems like a good change. But is there any way we can add a test for this, something that can run in CI? Maybe with iai, or with cargo asm or something? Unfortunately I don’t know of a tool that makes this sort of thing easy…

For some inspiration, maybe look at what is done here: https://github.com/taiki-e/atomic-memcpy/tree/main/tests/asm-test (obviously, we don’t need so many architectures, just do one like aarch64-unknown-none and it should be fine) I came up with a better solution, see below

@Kestrer
Copy link
Owner

Kestrer commented Sep 23, 2025

Btw, I fixed the CI failures, so you can rebase on/merge with main.

@Kestrer
Copy link
Owner

Kestrer commented Sep 23, 2025

Nevermind, someone pointed out to me a better solution. Make another library crate in the workspace and put in its Cargo.toml:

[lib]
crate-type = ["cdylib"]

Add this to its lib.rs

unsafe extern "C" {
     safe fn should_be_optimized_out() -> !;
}
fn assert(b: bool) {
    if !b {
        should_be_optimized_out();
    }
}

In CI, run the command RUSTFLAGS="-Clink-arg=-Wl,--no-undefined" cargo build --release. Then you can add tests here; for example:

#[unsafe(no_mangle)]
pub fn simple_range(i: &BoundedU8<3, 5>) {
    assert(3 <= i.get());
    assert(i.get() <= 5);
}

@clarfonthey
Copy link

Also worth noting that I think this statement is only necessary for the struct definitions, as the enum versions should have these sorts of assumptions built-in by default. But it's probably not harmful to use it for both cases.

@eira-fransham
Copy link
Author

eira-fransham commented Sep 24, 2025

I like the idea! I ran the automated tests under Miri, which is usually pretty good at finding undef. Great idea to use linker failures to handle this though!

EDIT: I also quickly manually tested that the asserts are optimised out using cargo disasm.

@eira-fransham eira-fransham force-pushed the maintain-range-information-on-get branch from fc453b0 to 7364647 Compare September 24, 2025 07:30
…1.90.0, even though it seems like it should
…ilds, add `miri test` for additional checks for undef
@eira-fransham
Copy link
Author

Ok, I added tests to ensure that the range checks are optimized out as expected.

"uses": "actions-rs/cargo@v1",
"with": {
"command": "miri",
# `miri` does not support the `macros` feature as it uses IO.
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wait, really? cargo +nightly miri test --workspace --all-features works for me.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting! I’ll investigate tomorrow

@eira-fransham
Copy link
Author

Made some changes based on your review. The name I went with in the end was optimizer_assert_guaranteed, flipping the condition to be the same as assert - i.e. it fails if the condition is (possible to be) false, rather than the prior implementation that failed if the condition was (possible to be) true. It was frankly a little tough to find a way to talk about const folding without risking confusion with the Rust concept of const but that's the best I could come up with.

@eira-fransham
Copy link
Author

@Kestrer Hey, any movement on this?

@Kestrer Kestrer merged commit 06295dc into Kestrer:main Oct 17, 2025
@Kestrer
Copy link
Owner

Kestrer commented Oct 17, 2025

Sorry for the delay. I didn’t forget but I’ve been rather sick…

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

Successfully merging this pull request may close these issues.

3 participants