Closed
Bug 771871
Opened 12 years ago
Closed 12 years ago
[TI] Wrong result with bound Function.prototype.call saved as an object property
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
RESOLVED
FIXED
mozilla17
People
(Reporter: shu, Assigned: shu)
Details
(Whiteboard: [js:p2])
Attachments
(2 files, 2 obsolete files)
136 bytes,
application/javascript
|
Details | |
3.59 KB,
patch
|
bhackett1024
:
review+
|
Details | Diff | Splinter Review |
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•12 years ago
|
||
(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.
Summary: [TI] Bound Function.prototype.call saved as an object property behaves weirdly → [TI] Wrong result with bound Function.prototype.call saved as an object property
Whiteboard: [js:p2]
Assignee | ||
Comment 2•12 years ago
|
||
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.
Assignee | ||
Comment 3•12 years ago
|
||
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.
Assignee | ||
Comment 4•12 years ago
|
||
See previous comment when reviewing.
Attachment #647442 -
Attachment is obsolete: true
Assignee | ||
Updated•12 years ago
|
Attachment #647447 -
Flags: review?(bhackett1024)
Comment 5•12 years ago
|
||
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.
Assignee | ||
Comment 6•12 years ago
|
||
(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.
Assignee | ||
Comment 7•12 years ago
|
||
Use reloadEntry and remove redundant code in checkCallApplySpeculation. Thanks to Brian.
Attachment #647447 -
Attachment is obsolete: true
Attachment #647447 -
Flags: review?(bhackett1024)
Assignee | ||
Updated•12 years ago
|
Attachment #648140 -
Flags: review?(bhackett1024)
Comment 8•12 years ago
|
||
Comment on attachment 648140 [details] [diff] [review] fix + test case Thanks!
Attachment #648140 -
Flags: review?(bhackett1024) → review+
Assignee | ||
Comment 9•12 years ago
|
||
http://hg.mozilla.org/integration/mozilla-inbound/rev/a3221ce758e5
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Comment 10•12 years ago
|
||
Leave it open until it gets merged to mozilla-central. Whoever merges it will update the bug.
Assignee: general → shu
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee | ||
Comment 11•12 years ago
|
||
(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•12 years ago
|
||
The full details are here if you are interested. :) https://wiki.mozilla.org/Inbound_Sheriff_Duty
Comment 13•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/a3221ce758e5
Status: REOPENED → RESOLVED
Closed: 12 years ago → 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
You need to log in
before you can comment on or make changes to this bug.
Description
•