Last Comment Bug 763883 - IonMonkey: (ARM) -x does not deal with INT_MIN properly
: IonMonkey: (ARM) -x does not deal with INT_MIN properly
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: ARM Linux
: -- normal (vote)
: ---
Assigned To: general
:
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-06-12 04:50 PDT by Marty Rosenberg [:mjrosenb]
Modified: 2012-07-27 17:47 PDT (History)
1 user (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
/home/mrosenberg/patches/fixNegate-r0.patch (2.90 KB, patch)
2012-06-12 04:50 PDT, Marty Rosenberg [:mjrosenb]
no flags Details | Diff | Splinter Review
/home/mrosenberg/patches/fixNegate-r1.patch (2.90 KB, patch)
2012-06-12 04:51 PDT, Marty Rosenberg [:mjrosenb]
Jacob.Bramley: review+
Details | Diff | Splinter Review

Description Marty Rosenberg [:mjrosenb] 2012-06-12 04:50:29 PDT
Created attachment 632205 [details] [diff] [review]
/home/mrosenberg/patches/fixNegate-r0.patch

It currently produces INT_MIN rather than -INT_MIN.
Comment 1 Marty Rosenberg [:mjrosenb] 2012-06-12 04:51:59 PDT
Created attachment 632206 [details] [diff] [review]
/home/mrosenberg/patches/fixNegate-r1.patch

with more qrefresh
Comment 2 Jacob Bramley [:jbramley] 2012-06-13 02:12:57 PDT
Comment on attachment 632206 [details] [diff] [review]
/home/mrosenberg/patches/fixNegate-r1.patch

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

r+ with the simple nits fixed.

::: js/src/ion/arm/CodeGenerator-arm.cpp
@@ +350,5 @@
>  {
>      const LAllocation *lhs = ins->getOperand(0);
>      const LAllocation *rhs = ins->getOperand(1);
>      const LDefinition *dest = ins->getDef(0);
> +    fprintf(stderr, "subtracting...\n");

This is surely debug code!

@@ +388,5 @@
> +            if (mul->canOverflow()) {
> +                masm.ma_cmp(ToRegister(dest), Imm32(INT_MIN));
> +                c = Assembler::Equal;
> +            }
> +            masm.ma_rsb(ToRegister(lhs), Imm32(0), ToRegister(dest));

Oh, it's simpler than that. I don't know why I didn't think of it when we discussed this on IRC, but you can simply do this:

rsbs dest, lhs, #0
bvs bailout

That is, the operation dest=0-lhs results in (signed) overflow iff lhs=INT_MIN, so you don't need a separate comparison.
Comment 3 Marty Rosenberg [:mjrosenb] 2012-07-27 17:47:36 PDT
Landed: http://hg.mozilla.org/projects/ionmonkey/rev/15afceb8e292

Note You need to log in before you can comment on or make changes to this bug.