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

Fix sign-extension in Isolate Data #213

Merged
merged 1 commit into from Jul 7, 2021

Conversation

hackmud-dtr
Copy link
Contributor

While helping @seanmakesgames debug #212, we discovered an issue with the bitshifting on MEM_SOFTLIMIT_MAX sometimes causing sign-extension on values.

When performing any arithmetic on a bitfield member, C++ will implicitly convert the value to an int (if the bitfield member is narrower than int) or unsigned int (if the bitfield member is the same width as int). In this case, a 22 bit-wide bitfield member is narrower than 32 bit ints, so the bitshift is performed on ints even though the underlying bitfield member is declared as uintptr_t (which is both wider than an int, and unsigned). And a left shift of 10 means that the high bit of the bitfield ends up in the sign bit of the resulting (signed) int. So if the high-bit is set in the bitfield, the high bit is set in the int after the left shift. That int then is converted to a uintptr_t, which sign-extends the int's sign bit into the 32 new bits of the uintptr_t.

Upshot: Any MEM_SOFTLIMIT_MAX between 2 and 4 gigs will store properly, but be fetched with 0xFFFFFFFF00000000 added to the value. Which is probably more RAM than exists. An explicit cast to uintptr_t before the shift ensures the shift works on the proper sized/signed value. (The right shift when setting is fine -- the data was always stored properly, it just was mangled on retrieval)

This also prevents a potential future issue -- if at some later point it was decided to store MEM_SOFTLIMIT_MAX shifted by 20 instead of 10 (since megabytes probably make more sense here), you'd run a real risk of shifting bits off the left end of the 32-bit int before converting to uintptr_t when fetching the value. Casting first means you have the full 64 bits to work with in the shift.

Relevant spec quotes
(for bit shifts)
"The integer promotions are performed on each of the operands. The type of the result is that of the promoted left operand."

(for integer promotion)
"If an int can represent all values of the original type (as restricted by the width, for a bit-field), the value is converted to an int; otherwise, it is converted to an unsigned int. These are called the integer promotions (f:58) All other types are unchanged by the integer promotions."

C++ will 'promote' a bitfield member that is narrower than an int into an int on arithmetic, leading to sign extension when converted back to a wider type.
@seanmakesgames
Copy link
Contributor

tldr:
I was using a value for softlimit that hit this condition which tapped into a deep cavern of unfortunate cpp specification and was unable to use printf to debug the number, because the getter mangled the number. This is a fix for that weirdness.

For our usage, our memlimit is low enough it doesn't hit this issue, but this is relevant for others.

@SamSaffron
Copy link
Collaborator

wow, what an edge case! Thanks heaps for the fix.

@SamSaffron SamSaffron merged commit 496f829 into rubyjs:master Jul 7, 2021
@seanmakesgames
Copy link
Contributor

wow, what an edge case! Thanks heaps for the fix.

Edge cases are all we have! 😛

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 this pull request may close these issues.

None yet

3 participants