Fix sign-extension in Isolate Data #213
Merged
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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."