Last Comment Bug 771871 - [TI] Wrong result with bound Function.prototype.call saved as an object property
: [TI] Wrong result with bound Function.prototype.call saved as an object property
Status: RESOLVED FIXED
[js:p2]
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: x86_64 Linux
: -- normal (vote)
: mozilla17
Assigned To: Shu-yu Guo [:shu]
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-07-07 22:28 PDT by Shu-yu Guo [:shu]
Modified: 2012-08-02 06:22 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
minimal test case (136 bytes, application/javascript)
2012-07-07 22:28 PDT, Shu-yu Guo [:shu]
no flags Details
fix (2.23 KB, patch)
2012-07-30 22:26 PDT, Shu-yu Guo [:shu]
no flags Details | Diff | Review
fix + test case (2.60 KB, patch)
2012-07-30 22:48 PDT, Shu-yu Guo [:shu]
no flags Details | Diff | Review
fix + test case (3.59 KB, patch)
2012-08-01 16:47 PDT, Shu-yu Guo [:shu]
bhackett1024: review+
Details | Diff | Review

Description Shu-yu Guo [:shu] 2012-07-07 22:28:13 PDT
Created attachment 640021 [details]
minimal test case

Run the attached test case with -n -a -m to see the bug immediately, or -n -m to see the bug after it compiles the loop.
Comment 1 David Mandelin [:dmandelin] 2012-07-09 15:56:31 PDT
(In reply to Shu-yu Guo [:shu] from comment #0)
> Created attachment 640021 [details]
> minimal test case
> 
> Run the attached test case with -n -a -m to see the bug immediately, or -n
> -m to see the bug after it compiles the loop.

To wit, the interpreter prints true for all iterations, but with -a -m -n you get false after the first iteration.
Comment 2 Shu-yu Guo [:shu] 2012-07-30 22:24:28 PDT
This is caused by TI's use of XMM registers to store double return values when the known push type of a call is JSVAL_TYPE_DOUBLE in conjunction with attempts to lower |Function.prototype.call|.

What happens is that the |stub::SlowCall| rejoin for trying to lower call/apply results in setting the value into JSReturnReg_Type and JSReturnReg_Data before jumping _after_ the normal call IC. But if the known push type of the call is double, the non-lowering rejoin will use a XMM register, and will synch using that regardless of which branch it came from.

Thanks to Luke for suggesting that it might be double-related.
Comment 3 Shu-yu Guo [:shu] 2012-07-30 22:26:24 PDT
Created attachment 647442 [details] [diff] [review]
fix

Brian,

I'm allocating a new fp reg on slow call rejoin from attempting to lower. I reckon this needs some refactoring -- but the call IC in JM is pretty long and that whole function seems delicate.
Comment 4 Shu-yu Guo [:shu] 2012-07-30 22:48:44 PDT
Created attachment 647447 [details] [diff] [review]
fix + test case

See previous comment when reviewing.
Comment 5 Brian Hackett (:bhackett) 2012-07-31 05:22:47 PDT
Comment on attachment 647447 [details] [diff] [review]
fix + test case

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

Does the testcase work if you just do:

if (type == JSVAL_TYPE_DOUBLE)
    type = JSVAL_TYPE_UNKNOWN;

When lowering a call or apply?  I'd really like to avoid changing the call path's behavior if possible, because it's a giant mess of complexity (with call/apply lowering by far the worst offender) and I've mostly forgotten how it works.
Comment 6 Shu-yu Guo [:shu] 2012-07-31 08:03:33 PDT
(In reply to Brian Hackett (:bhackett) from comment #5)
> Comment on attachment 647447 [details] [diff] [review]
> fix + test case
> 
> Review of attachment 647447 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Does the testcase work if you just do:
> 
> if (type == JSVAL_TYPE_DOUBLE)
>     type = JSVAL_TYPE_UNKNOWN;
> 

I don't think that's any cleaner. You would have to do that earlier, before the return regs are pushed onto the frame, like:

if (lowerFunCallOrApply && type == JSVAL_TYPE_DOUBLE)
  type = JSVAL_TYPE_UNKNOWN;
frame.pushRegs(JSReturnReg_Type, JSReturnReg_Data, type);

And then floating point registers will never be used whenever we try to lower call/apply.
Comment 7 Shu-yu Guo [:shu] 2012-08-01 16:47:48 PDT
Created attachment 648140 [details] [diff] [review]
fix + test case

Use reloadEntry and remove redundant code in checkCallApplySpeculation. Thanks to Brian.
Comment 8 Brian Hackett (:bhackett) 2012-08-01 16:57:57 PDT
Comment on attachment 648140 [details] [diff] [review]
fix + test case

Thanks!
Comment 10 Andrew McCreight [:mccr8] 2012-08-01 17:31:46 PDT
Leave it open until it gets merged to mozilla-central.  Whoever merges it will update the bug.
Comment 11 Shu-yu Guo [:shu] 2012-08-01 17:34:05 PDT
(In reply to Andrew McCreight [:mccr8] from comment #10)
> Leave it open until it gets merged to mozilla-central.  Whoever merges it
> will update the bug.

Oops, wasn't aware of that policy. Thanks!
Comment 12 Andrew McCreight [:mccr8] 2012-08-01 17:34:55 PDT
The full details are here if you are interested. :)  https://wiki.mozilla.org/Inbound_Sheriff_Duty
Comment 13 Ed Morley [:emorley] 2012-08-02 06:22:40 PDT
https://hg.mozilla.org/mozilla-central/rev/a3221ce758e5

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