dart-lang / sdk

The Dart SDK, including the VM, JS and Wasm compilers, analysis, core libraries, and more.
https://dart.dev
BSD 3-Clause "New" or "Revised" License
10.26k stars 1.58k forks source link

Handling of >> and << with negative shift values is not specified/wrong in dart2js? #1186

Closed DartBot closed 9 years ago

DartBot commented 12 years ago

This issue was originally filed by jimhug@google.com


These two cases behave differently between frog/dartc and the vm in production mode. I'd like to confirm that this is a bug an frog and also decide if this needs to be in the Editor milestone or not. Kasper - This is assigned to you to confirm the correct behavior and to assign it to that milestone if you choose. I am separating this out from Issue #596 to track the important differences.

void main() {   var x = 77;   print(x >> (-1)); // frog,dartc: 0, VM: exception   print(foo() << (-1)); // frog,dartc: -2147483648, VM: exception }

The part of this bug that is critical is resolving the expected behavior and whether or not this needs to be moved into the Editor milestone.

dgrove commented 12 years ago

The fact that these aren't well-defined is a dup of issue #1136.

I believe that the milestone issue here is that frog should be calling a function rather than using a straight <</>> unless it can prove both of the following:   1. lhs of the operation is an integer   2. rhs of the operation is an integer that is >= 0;


Added label.

DartBot commented 12 years ago

This comment was originally written by jimhug@google.com


I'm sorry, but the statement that "frog should be calling a function unless..." is not a useful way to specify expected behavior. For this to be a frog bug, it needs to explain what the expect output of the program is. When I went to fix this bug, I compared results to dartc in order to understand what our current best specification of correct behavior of a system with the JS numeric exemption was.

Your rule is one interesting possibility for the correct behavior, but it seems like an awful lot of work for very little value to me. There is a broad range of values for which frog will currently generate different results on the << and >> operators than the VM will.

For x << y, these include y < 0 as you point out. However, they also include y > 30 (unless x == 0) as well as x > 2^30ish for any values of y - including 0. For each value of y between 0 and 30, there is a different range of values of x that will and will not match the VM.

In order to fix this bug in frog I need someone to clearly specify the correct behavior - as I'm hoping to get out of issue #1136. Until that is resolved, I would be prefer to just mark this as blocked on that issue. However, I'm waiting to see if Kasper feels differently and would like to give me a precise specification for how these operators should work on a Dart implementation that compiles to JS if he feels it needs to be fixed as part of the editor milestone.

kasperl commented 12 years ago

Jim has already started working on improving the testing situation in this CL: http://codereview.chromium.org/9149005/.

kasperl commented 12 years ago

Moving this out of Area-Frog and putting it into Area-Language for the lack of a better place. One thing we could say is that we'll use JavaScript semantics for shifts when both the LHS and the RHS are numbers.


Removed Area-Frog label. Added Area-Language label.

anders-sandholm commented 12 years ago

Added this to the M1 milestone.

gbracha commented 12 years ago

Moving out of language per discussion with Kasper.


Removed Area-Language label. Added Area-Dart2JS label.

kasperl commented 12 years ago

We've decided to stick with unsigned 32-bit int output from bitwise operation and no range checks for the int inputs for M1.


Removed this from the M1 milestone. Added this to the Later milestone. Removed Priority-Critical label. Added Priority-Medium label. Changed the title to: "Handling of >> and << with negative shift values is not specified/wrong in dart2js?".

kasperl commented 12 years ago

Removed this from the Later milestone.

kasperl commented 12 years ago

Added this to the Later milestone.

lrhn commented 11 years ago

Issue #9400 has been merged into this issue.

kasperl commented 11 years ago

Added Triaged label.

kasperl commented 11 years ago

Added TriageForM5 label.

kasperl commented 11 years ago

Removed TriageForM5 label.

kasperl commented 11 years ago

Issue #11521 has been merged into this issue.

lrhn commented 11 years ago

Shift is now specified (or will soon be) and implemented to not allow negative shifts.


Added Fixed label.

DartBot commented 10 years ago

This comment was originally written by jdav...@pcprogramming.com


what version does this roll out in?

floitschG commented 10 years ago

The negative shift check has been implemented a long time ago.