Fix an MSVC warning in MacroAssembler

RESOLVED FIXED in Firefox 41

Status

()

Core
JavaScript Engine
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: terrence, Assigned: terrence)

Tracking

unspecified
mozilla41
Points:
---

Firefox Tracking Flags

(firefox41 fixed)

Details

Attachments

(1 attachment)

(Assignee)

Description

3 years ago
Created attachment 8621143 [details] [diff] [review]
warning_fix_macroassembler-v0.diff

If |bytes| is larger than INT_MAX, the conversion from uint32_t to int followed by unary- will make the number positive and the stack will grow in the wrong direction. Of course the frame size is never going to be larger than INT_MAX here (or we wouldn't be compiling it), but we might as well silence the warning and add some robustness.
Attachment #8621143 - Flags: review?(nicolas.b.pierron)
While you're here: perhaps we should really be using int32_t instead of "int" here, for adjustFrame's "int value" arg?  It seems slightly strange to be doing math with a platform-defined "int" type and a fixed-size uint32_t value, framePushed_.

This is relevant to this bug because if "int" is 64-bit, then your INT_MAX assertion won't really be useful, because INT_MAX will be way beyond what a uint32_t can represent.
(Assignee)

Comment 2

3 years ago
Good point. I was thinking this was a platform specific part of the code. It should use INT32_MAX, regardless.
(Assignee)

Comment 3

3 years ago
Err, no, that's not right. We're casting from uint32_t -> int before doing the inversion, so as long as the value is less than INT_MAX, we should be fine because INT_MAX >= UINT32_MAX on all platforms.
(Assignee)

Comment 4

3 years ago
Err, no, again. The assertion won't fire ever on x64 because INT_MAX > UINT32_MAX, but that's fine because the conversion can't fail in that case. On 32bit, INT_MAX == INT32_MAX, so it doesn't matter there either.
Comment on attachment 8621143 [details] [diff] [review]
warning_fix_macroassembler-v0.diff

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

::: js/src/jit/MacroAssembler-inl.h
@@ +32,2 @@
>  {
>      setFramePushed(framePushed_ + value);

nit: Assert we do not underflow, when converting this result to an uint32_t.

@@ +37,5 @@
>  MacroAssembler::implicitPop(uint32_t bytes)
>  {
>      MOZ_ASSERT(bytes % sizeof(intptr_t) == 0);
> +    MOZ_ASSERT(bytes <= INT_MAX);
> +    adjustFrame(-int(bytes));

nit: use int32_t, here and above, and update the macro assembler as well.
Comment on attachment 8621143 [details] [diff] [review]
warning_fix_macroassembler-v0.diff

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

::: js/src/jit/MacroAssembler-inl.h
@@ +37,5 @@
>  MacroAssembler::implicitPop(uint32_t bytes)
>  {
>      MOZ_ASSERT(bytes % sizeof(intptr_t) == 0);
> +    MOZ_ASSERT(bytes <= INT_MAX);
> +    adjustFrame(-int(bytes));

indeed int or int32 are both fine, as long as the MAX value correspond.
Attachment #8621143 - Flags: review?(nicolas.b.pierron) → review+
https://hg.mozilla.org/mozilla-central/rev/50f91f9d8383
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox41: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
You need to log in before you can comment on or make changes to this bug.