Created attachment 430269 [details]
The attached program triggers an assertion failure when running jsc in 32 bits and debug mode.
In 32 bits, asCell() returns a pointer to the payload and using reinterpret_cast will sign extend asNumber to FFFF FFFF + <value-address>, which will fail the isImpureNaN in jsNumber.
So first cast the address to uint32_t then to uint64_t to prevent the tag corruption.
Created attachment 430271 [details]
Created attachment 430274 [details]
Comment on attachment 430274 [details]
View in context: https://bugs.webkit.org/attachment.cgi?id=430274&action=review
> +#if USE(JSVALUE32_64)
> + // In 32 bits, asCell() returns a pointer to the payload and using reinterpret_cast
> + // will sign extend asNumber to FFFF FFFF + <value-address>, which will fail the
> + // isImpureNaN check in jsNumber. To prevent that, first reinterpret_cast the address
> + // to uint32_t then cast it to uint64_t. AddressOf in 32 bits will return the same
> + // address returned by describe() but in double
> + uint64_t asNumber = static_cast<uint64_t>(reinterpret_cast<uint32_t>(value.asCell()));
> // Need to cast to uint64_t so bitwise_cast will play along.
> uint64_t asNumber = reinterpret_cast<uint64_t>(value.asCell());
Better fix is to just replace the incorrect <uint64_t> with the correct <uintptr_t> instead of adding code and comments. No need to use static_cast to expand a 32-bit unsigned int to 64-bit.
Indeed, <uintptr_t> sounds like a better solution here, but we would still need the static_cast to 64 bits because of the bitcast<double>.
In 32 bits <uintptr_t> is 32 bits long and the bitcast will fail.
(In reply to Mikhail R. Gadelha from comment #4)
> Indeed, <uintptr_t> sounds like a better solution here, but we would still
> need the static_cast to 64 bits because of the bitcast<double>.
No static_cast needed. Write this:
uint64_t asNumber = reinterpret_cast<uintptr_t>(value.asCell());
There’s no need for a static_cast on this line of code. The unsigned integer conversion, with possible promotion, can be done without any casting.
I got what you mean now!
I'll send an updated patch shortly
Created attachment 430302 [details]
Comment on attachment 430302 [details]
I suspect that other callers of reinterpret_cast<uint64_t> throughout WebKit could have the same issue on 32-bit. There are 20 of them at least, and many are likely converting from pointers to integers; the same fix would likely apply.
Committed r278660 (238642@main): <https://commits.webkit.org/238642@main>
All reviewed patches have been landed. Closing bug and clearing flags on attachment 430302 [details].