Closed Bug 921364 Opened 6 years ago Closed 6 years ago

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

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla27

People

(Reporter: briansmith, Assigned: mz_mhs-ctb)

Details

Attachments

(1 file, 3 obsolete files)

// 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());
Attached patch Patch (obsolete) — Splinter Review
Attachment #811469 - Flags: review?(luke)
Attachment #811469 - Flags: review?(luke)
Luke, will modifying this code to emit signed return values have any effect on JS compilation?
Attached patch Patch (obsolete) — Splinter Review
This doesn't change the return per se, but rather gets rid of the negation operator to silence that error.
Attachment #811469 - Attachment is obsolete: true
Attachment #811744 - Flags: review?(luke)
Attachment #811744 - Attachment description: bug921364.patch → Patch
Attachment #811744 - Attachment filename: bug921364.patch → Patch
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.
(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.
Attachment #811744 - Attachment is obsolete: true
Attachment #811744 - Flags: review?(luke)
Attachment #811469 - Attachment is obsolete: false
Attachment #811469 - Flags: review?(luke)
All right, changed that.
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).
Attachment #811469 - Flags: review?(luke) → review+
Attached patch Patch (v2) (obsolete) — Splinter Review
Like this or without the '-'?
Attachment #811469 - Attachment is obsolete: true
Attachment #813667 - Flags: review?(luke)
Comment on attachment 813667 [details] [diff] [review]
Patch (v2)

Yes, exactly.  (The - is necessary)
Attachment #813667 - Flags: review?(luke) → review+
Attachment #813805 - Flags: checkin?
Attachment #813667 - Attachment is obsolete: true
Attachment #813805 - Flags: checkin? → checkin+
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! :)
Assignee: nobody → mz_mhs-ctb
https://hg.mozilla.org/mozilla-central/rev/650952735c8b
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
You need to log in before you can comment on or make changes to this bug.