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

Inaccuracies in the reference about bitshifts and overflows #23421

Closed
simias opened this issue Mar 16, 2015 · 8 comments · Fixed by #23662
Closed

Inaccuracies in the reference about bitshifts and overflows #23421

simias opened this issue Mar 16, 2015 · 8 comments · Fixed by #23662

Comments

@simias
Copy link

simias commented Mar 16, 2015

The references states that >> is a logical right shift, however it seems that it behaves like an arithmetic right shift for signed integers. For instance the following snippet prints -2:

let i = -4i32;

println!("{}", i >> 1);

A logical right shift should produce 2147483646 (assuming 2's complement representation which I think rust guarantees?).

In the same spirit the reference does not say if shifting outside the range of an integer is defined. From my quick test it doesn't appear to be:

let i = 10u32;
let j = 200u32;

println!("{}", i << j);

This prints garbage on my computer and on the playpen, so I assume it's UB or at least UV (it doesn't trigger the overflow check in debug builds though, maybe it should?)

On a similar topic the reference also states that the following behaviours are safe:

  • Unsigned integer overflow (well-defined as wrapping)
  • Signed integer overflow (well-defined as two's complement representation wrapping)

I believe that's not true anymore.

@simias
Copy link
Author

simias commented Mar 17, 2015

I see that checking for bitshift overflows is going to be implemented in #22020 so it seems to confirm that it's UB (UV?) for now.

@pnkfelix
Copy link
Member

see PR #23536 for the checks. We can go either way on the issue of whether overflow is UB or UV or neither. (That is, it is easy to adapt the PR accordingly; just a matter of deciding, and perhaps measuring the impact on code size, maybe.)

@steveklabnik
Copy link
Member

@pnkfelix what's the process by which we decide that? I'd like to knock this issue out.

@pnkfelix
Copy link
Member

@steveklabnik heh, I already made an "executive decision" (though admittedly it was one that @nikomatsakis was already in favor of), of making overflow panic when checks are on, and when the checks are off, we forcibly mask the RHS to ensure that we do not encounter weirdness such as that documented in #10183 and #23551.

(Though I still need to put in tests for that fallback behavior.)

@simias
Copy link
Author

simias commented Mar 24, 2015

That sounds very reasonable, thanks.

Right shifts of signed values will remain arithmetic though, right? It's probably what most people will expect since that's how it's done in C/C++ and we'd have to simple way of doing arithmetic shifting otherwise.

@pnkfelix
Copy link
Member

@simias right-shift of signed values are still arithmetic shifts, but that is independent of the overflow checking (which is based solely on the value of the right-hand-side compared to the bitwidth of the type of the left-hand-side)

@simias
Copy link
Author

simias commented Mar 24, 2015

Okay, that makes sense. I wanted to make sure that was the intended behaviour since the reference seems to say otherwise.

Thank you!

@pnkfelix
Copy link
Member

@simias oh I see; yes that part of the reference has probably been incorrect for a long time.

steveklabnik added a commit to steveklabnik/rust that referenced this issue Mar 24, 2015
alexcrichton added a commit to alexcrichton/rust that referenced this issue Mar 24, 2015
I assume since both shifts say the same thing, I should fix both of them, but then I realized I don't strictly know about left shift.

Fixes rust-lang#23421

r? @pnkfelix
dlrobertson pushed a commit to dlrobertson/rust that referenced this issue Nov 29, 2018
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 a pull request may close this issue.

4 participants