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

Comparison with Instant of u32::MAX is unsound #35

Closed
henrikssn opened this issue Jul 1, 2022 · 7 comments
Closed

Comparison with Instant of u32::MAX is unsound #35

henrikssn opened this issue Jul 1, 2022 · 7 comments

Comments

@henrikssn
Copy link

henrikssn commented Jul 1, 2022

I try this code on a thumbv7em-none-eabihf target:

defmt::println!("{:?}", TimerInstantU32::<100_000_000>::from_ticks(u32::MAX) > TimerInstantU32::<100_000_000>::from_ticks(1000));

Output:

0.099983 false

Since u32::MAX > 1000, something seems to be wrong inside of const_cmp?

@korken89
Copy link
Owner

korken89 commented Jul 2, 2022

Hi, this looks correct to me.
Instant supports time which can roll over, so the maximum range is MAX/2 - 1.

I think the documentation needs clarification here

@korken89
Copy link
Owner

korken89 commented Jul 2, 2022

And thanks for bringing this to my attention!

@henrikssn
Copy link
Author

Interesting, thanks for the explanation! What do you think about adding an Instant::MAX constant? :)

@korken89
Copy link
Owner

korken89 commented Jul 2, 2022

I see what you want, but it would be difficult.
As every instant is relative another instant there is no absolute MAX value, it's simply MAX/2 - 1 that is the maximum relative offset that is valid without overflow.

Does that make sense?

@henrikssn
Copy link
Author

Hmm, I think so. If we would define u32::MAX/2 - 1 as MAX, would it then be true that

Instant::MAX + Instant::from_ticks(1) > Instant::MAX

@korken89
Copy link
Owner

korken89 commented Jul 3, 2022

That is correct.

@korken89
Copy link
Owner

korken89 commented Jul 3, 2022

You can run this test to verify:

    #[test]
    fn yolo() {
        let max = Instant::<u32, 1, 1>::from_ticks(core::u32::MAX / 2 - 1);
        let maxpone = Instant::<u32, 1, 1>::from_ticks(core::u32::MAX / 2);

        assert!(maxpone > max);
    }

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