1229196 - Fix MSVC C4334 "was 64-bit shift intended" warning in js/src/asmjs/AsmJSSignalHandlers.cpp

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Chris Peterson [:cpeterson]]

Attachment: js-fix-C4334.patch

Dan, this warning came from your patch for addresses with constant offsets (bug 986981). Is it safe to assume address.scale() is always shifting fewer than 32 bits?

js/src/asmjs/AsmJSSignalHandlers.cpp(594) : warning C4334: '<<' : result of 32-bit shift implicitly converted to 64 bits (was 64-bit shift intended?)

Comment 1

[Dan Gohman [:sunfish]]

Comment on attachment 8693815 [details] [diff] [review]
js-fix-C4334.patch

Review of attachment 8693815 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/asmjs/AsmJSSignalHandlers.cpp
@@ +591,5 @@
>          uintptr_t index;
>          StoreValueFromGPReg(SharedMem<void*>::unshared(&index), sizeof(uintptr_t),
>                              AddressOfGPRegisterSlot(context, address.index()));
> +        MOZ_ASSERT(address.scale() < 32, "address shift overflow?");
> +        result += index * (1ULL << address.scale());

This will work, but uintptr_t(1) would be slightly nicer than 1ULL, because on 32-bit platforms unsigned long long is wider than uintptr_t, which is the width of the rest of the computation here, so some other compiler might warn about the implicit truncating conversion.

Comment 2

[Chris Peterson [:cpeterson]]

Thanks. I'll use uintptr_t(1) instead of 1ULL.

Comment 3

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ca9261497284

Comment 4

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/ca9261497284