921364 - ToStackIndex applies negation operator to unsigned type -> warning C4146 result still unsigned

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

// see OffsetOfFrameSlot
static inline int32_t
ToStackIndex(LAllocation *a)
{
    if (a->isStackSlot()) {
        JS_ASSERT(a->toStackSlot()->slot() >= 1);
        return a->toStackSlot()->slot();
    }
    JS_ASSERT(-int32_t(sizeof(IonJSFrameLayout)) <= a->toArgument()->index());
    return -(sizeof(IonJSFrameLayout) + a->toArgument()->index());
}

The return statement probably needs to be:

    return -static_cast<int32_t>(sizeof(IonJSFrameLayout) + 
                                 a->toArgument()->index());

Comment 1

[Michael]

Attachment: Patch

Comment 2

[Michael]

Luke, will modifying this code to emit signed return values have any effect on JS compilation?

Comment 3

[Michael]

Attachment: Patch

This doesn't change the return per se, but rather gets rid of the negation operator to silence that error.

Comment 4

[Luke Wagner [:luke]]

Maybe I'm missing something, but negation does have an effect: -x === (~x + 1) so, even if it is undefined per-spec, I expect this is what the compiler is doing.  I think the fix is to cast to int32_t before negating.  To verify your fix, you can build a JS shell (https://developer.mozilla.org/en-US/docs/SpiderMonkey/Build_Documentation) and run
  js/src/jit-test/jit_test.py $(PATH_TO_SHELL) --tbpl
which I expect would fail with the above patch.

Comment 5

[Michael]

(In reply to Luke Wagner [:luke] from comment #4)
> Maybe I'm missing something, but negation does have an effect: -x === (~x +
> 1) so, even if it is undefined per-spec, I expect this is what the compiler
> is doing.  I think the fix is to cast to int32_t before negating.  To verify
> your fix, you can build a JS shell
> (https://developer.mozilla.org/en-US/docs/SpiderMonkey/Build_Documentation)
> and run
>   js/src/jit-test/jit_test.py $(PATH_TO_SHELL) --tbpl
> which I expect would fail with the above patch.

Ah yes, but what I was worried about was a signed return type affecting the other functions that depend on the value.

Comment 6

[Michael]

All right, changed that.

Comment 7

[Luke Wagner [:luke]]

Comment on attachment 811469 [details] [diff] [review]
Patch

Thanks.  Could write the cast int32_t(sizeof(IonJSFrameLayout) + a->toArgument()->index)?  JS engine doesn't usually use static_cast for int casts except to draw attention to something weird (see, e.g., the line right above the one you are chaing).

Comment 8

[Michael]

Attachment: Patch (v2)

Like this or without the '-'?

Comment 9

[Luke Wagner [:luke]]

Comment on attachment 813667 [details] [diff] [review]
Patch (v2)

Yes, exactly.  (The - is necessary)

Comment 10

[Michael]

Attachment: Patch for checkin; r=luke

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/650952735c8b

FYI, there's an extra space in front of your email address in your Mercurial settings. Also, you can just set the checkin-needed bug keyword in the future to request landing. It's a bit more preferred for bugs like this where there aren't multiple patches landing at different times. Thanks! :)

Comment 12

[Ekanan Ketunuti]

https://hg.mozilla.org/mozilla-central/rev/650952735c8b