Closed Bug 1229196 Opened 4 years ago Closed 4 years ago

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

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla45
Tracking Status
firefox45 --- fixed

People

(Reporter: cpeterson, Assigned: cpeterson)

References

(Blocks 2 open bugs)

Details

Attachments

(1 file)

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?)
Attachment #8693815 - Flags: review?(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.
Attachment #8693815 - Flags: review?(sunfish) → review+
Thanks. I'll use uintptr_t(1) instead of 1ULL.
https://hg.mozilla.org/mozilla-central/rev/ca9261497284
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
You need to log in before you can comment on or make changes to this bug.