1173908 - Fix an MSVC warning in MacroAssembler

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Terrence Cole [:terrence]]

Attachment: 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.

Comment 1

[Daniel Holbert [:dholbert]]

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.

Comment 2

[Terrence Cole [:terrence]]

Good point. I was thinking this was a platform specific part of the code. It should use INT32_MAX, regardless.

Comment 3

[Terrence Cole [:terrence]]

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.

Comment 4

[Terrence Cole [:terrence]]

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 5

[Nicolas B. Pierron [:nbp]]

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 6

[Nicolas B. Pierron [:nbp]]

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.

Comment 7

[Terrence Cole [:terrence]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=225e25d0f903

Comment 8

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/50f91f9d8383

Comment 9

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/50f91f9d8383