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

Shift exponent 32 is too large for 32-bit type 'KUInt32' #54

Closed
panicsteve opened this issue May 30, 2018 · 3 comments
Closed

Shift exponent 32 is too large for 32-bit type 'KUInt32' #54

panicsteve opened this issue May 30, 2018 · 3 comments

Comments

@panicsteve
Copy link
Collaborator

This may not be an actual problem in the real world (it seems to run fine on all our supported platforms), but I'm reporting it since it was flagged by the Undefined Behavior Sanitizer on macOS.

In line 413 of Emulator/JIT/Generic/TJITGeneric_Macros.h, when amount has the value 32, it attempts to shift the 32-bit KUInt32 theResult right by 32 bits.

The sanitizer flags this as "Invalid shift exponent" with the explanation "Shift exponent 32 is too large for 32-bit type 'KUInt32'".

There's a similar situation in line 389 of Monitor/UDisasm.cp where a 32-bit value is shifted left by 32 bits, when rotate has the value 0, but this line of code is only generating text for the monitor, and is not part of the emulation.

@pguyot
Copy link
Owner

pguyot commented May 7, 2020

This is a dangerous bug. Indeed, shifting 32 bits a 32 bit value is an undefined behavior, and I remember there was an issue when supported platforms were 32 bits only, PowerPC and x86 behaved differently. The emulator seemed to work fine (on one platform), however there were little glitches that were not present on the other platform.

@panicsteve
Copy link
Collaborator Author

Although the behavior is officially undefined, is it correct to say that NewtonOS as it is written expects a 32-bit rotate to be effectively a nop? (It seems to be the case?) If so, we should just check if the rotate amount is 32, and do nothing if so, rather than leaving it up to the compiler.

@pguyot
Copy link
Owner

pguyot commented May 10, 2020

The code seems to say so and recent ARM documentation confirms indeed that ROR by 32 bits is a NOP (except for the carry).

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