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

Bitwise operators OR and AND are not working with -(2^63 - 1) in 3.2.3 [regression] #42289

Closed
rui-curado opened this issue Sep 24, 2020 · 6 comments · Fixed by #73363
Closed

Comments

@rui-curado
Copy link

rui-curado commented Sep 24, 2020

Godot version:

3.2.3

OS/device including version:

Windows 10, 2004

Issue description:

Bitwise operators OR and AND aren't working 100% correctly in version 3.2.3, but were working in 3.2.2.

I didn't test XOR.

Steps to reproduce:

If I want to set bit 63 on a 64-bit int using:
var value:int = 0 | -9223372036854775808
I get -9223372036854775807 which is wrong
Godot just set bit 63 and bit 0!

Likewise, if I want to test for bit 63:
var value:int = -1 & -9223372036854775808
I get again -9223372036854775807 which is wrong
Godot tested bit 63 and bit 0!

(I also tested with Python, just to be sure)

Possibly related, I repeatedly see on the console while editing:

ERROR: Cannot represent 9223372036854775808 as 64-bit integer, provided value is too big.
At: core/ustring.cpp:1821

Which is actually correct. However, I don't have that number anywhere in code. Well, I do have that number, but negative. The parser seems to be ignoring the minus sign.

My hunch is that "-9223372036854775808" is being incorrectly parsed, leading to incorrect bitwise operations.

Minimal reproduction project:

@akien-mga akien-mga changed the title Bitwise operators OR and AND are not working in 3.2.3. Were working in 3.2.2. Bitwise operators OR and AND are not working with -(2^63 - 1) in 3.2.3. Were working in 3.2.2. Sep 24, 2020
@akien-mga
Copy link
Member

Minimal reproduction project:

Bitwise huge values.zip

Indeed the issue seems to be that -9223372036854775808 is parsed as -(9223372036854775808), and 9223372036854775808 is out of bounds for a signed 64-bit integer.

The same issue happens in 3.2.2 too if you use actual hexadecimal notation, which makes those magic numbers somewhat easier to parse. -0x8000000000000000 fails as hex_to_int64 is called on 0x8000000000000000.

@akien-mga akien-mga changed the title Bitwise operators OR and AND are not working with -(2^63 - 1) in 3.2.3. Were working in 3.2.2. Bitwise operators OR and AND are not working with -(2^63 - 1) in 3.2.3 [regression] Sep 24, 2020
@akien-mga akien-mga added this to the 3.2 milestone Sep 24, 2020
@akien-mga akien-mga modified the milestones: 3.2, 4.0 Sep 24, 2020
@akien-mga
Copy link
Member

It's also reproducible in the master branch.

In 3.2, the regression happened between 3.2.2-stable and 3.2.3-beta1 (89f57ae).

Probably caused by #40009 / #40149 (@mrushyendra).

The same issue happens in 3.2.2 too if you use actual hexadecimal notation, which makes those magic numbers somewhat easier to parse. -0x8000000000000000 fails as hex_to_int64 is called on 0x8000000000000000.

To clarify, that one is a pre-existing bug not caused by the above PRs. But that's also something that should be fixed eventually.

@mrushyendra
Copy link
Contributor

If I'm understanding correctly, it seems like the issue is not with the changes in #40009 itself, but due to String::to_int64 being called with the sign omitted when parsing? In that case, it seems like the choices are (i) doing away with the overflow checks and leaving such behavior undefined, or (ii) modifying the calls to to_int64 when parsing

@akien-mga
Copy link
Member

Yeah I think this might need to be fixed in the GDScript parser so that it calls String::to_int64("-9223372036854775808") instead of String::to_int64("9223372036854775808") and then making it negative.

This would fix both the current "regression" and the pre-existing bug when using -0x8000000000000000 (with hex_to_int64).

CC @godotengine/gdscript

@bruvzg
Copy link
Member

bruvzg commented Sep 24, 2020

Yeah I think this might need to be fixed in the GDScript parser

Expressions also have the same behavior, e.g this crashing test case have to use (-9223372036854775807 - 1) for the same reason - #40955 (comment)

@dalexeev
Copy link
Member

dalexeev commented Feb 9, 2023

My hunch is that "-9223372036854775808" is being incorrectly parsed, leading to incorrect bitwise operations.

-9223372036854775808 is not a literal, it's a unary minus of 9223372036854775808 which is not representable, the maximum int value is 9223372036854775807. For the same reason, negative numbers in annotations did not work (#41183), before support for constant expressions was added.

This is somewhat of an argument in favor of handling negative number literals in the tokenizer, but it can lead to the problem of parsing expressions like 1-1 (binary minus without spaces). I'm not sure if it's worth it, since only one valid integer has the representability problem. You can use 1 << 63 instead.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants