Closed
Bug 1171777
Opened 9 years ago
Closed 9 years ago
Incorrect optimization of negative integer property access
Categories
(Core :: JavaScript Engine: JIT, defect)
Tracking
()
RESOLVED
FIXED
mozilla41
Tracking | Status | |
---|---|---|
firefox38 | --- | unaffected |
firefox38.0.5 | --- | unaffected |
firefox39 | + | fixed |
firefox40 | + | fixed |
firefox41 | --- | fixed |
People
(Reporter: jason, Assigned: arai)
References
Details
(Keywords: regression, Whiteboard: regression)
Attachments
(1 file, 1 obsolete file)
2.41 KB,
patch
|
jandem
:
review+
jandem
:
feedback+
lizzard
:
approval-mozilla-aurora+
lizzard
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.10; rv:40.0) Gecko/20100101 Firefox/40.0 Build ID: 20150604004008 Steps to reproduce: The following jsbin throws an error to the console, but should not: http://jsbin.com/nohabu/1 http://jsbin.com/nohabu/1/edit?js [code] The error occurs after approximately 1000 iterations through a loop, but each iteration should be independent. This leads me to suspect that the code becomes incorrect after it is optimized. The problem occurs when using a global variable with a negative integer value to access a property of an object. It seems that the object must have more than one integer property in order to trigger the bug. I have verified that the problem occurs in Firefox 39 (currently beta) and Firefox 40 (developer edition), but does not occur in Firefox 38. FWIW, this is a reduced test case of a bug that is triggered in the open source math typesetting library MathQuill: https://github.com/mathquill/mathquill/tree/dev This typesetting library is used in the Desmos graphing calculator, and you can see the bug in the wild when you load graphs with many typeset equations: https://www.desmos.com/calculator/7oytaenp0b. Here the error is called "Error: prayer failed: leftward is properly set up." Actual results: Error thrown after approximately 1000 iterations. Expected results: Each iteration should behave the same way.
Updated•9 years ago
|
Component: Untriaged → JavaScript Engine: JIT
Product: Firefox → Core
Assignee | ||
Comment 1•9 years ago
|
||
Here's regression range https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=4a061226ac74&tochange=3f7d0f1390e3 Seems to be bug 1131531.
Assignee | ||
Comment 2•9 years ago
|
||
So, here https://dxr.mozilla.org/mozilla-central/source/js/src/jit/IonCaches.cpp#3533 > /* static */ bool > GetElementIC::canAttachDenseElementHole(JSObject* obj, const Value& idval, TypedOrValueRegister output) > { > if (!idval.isInt32()) > return false; idval can be negative. In the testcase, it's -1. I think it should return false for that case, am I correct?
Comment 3•9 years ago
|
||
Mercurial says evilpie wrote this code, needinfo'ing :)
Flags: needinfo?(evilpies)
Assignee | ||
Comment 4•9 years ago
|
||
fwiw, I'm running try run with draft patch https://treeherder.mozilla.org/#/jobs?repo=try&revision=48fd9302c17c
Assignee | ||
Comment 5•9 years ago
|
||
Added the sign check both to canAttachDenseElementHole and GenerateDenseElementHole. Is this correct way to solve the issue? at least it passes the jstests and jit-test: https://treeherder.mozilla.org/#/jobs?repo=try&revision=48fd9302c17c
Attachment #8615954 -
Flags: feedback?(jdemooij)
Comment 6•9 years ago
|
||
[Tracking Requested - why for this release]: Breaks code in some cases. We need to make sure we don't ship this in 39!
Comment 7•9 years ago
|
||
// Make sure index is positive. Strictly speaking you're ensuring the index is nonnegative, I think.
Updated•9 years ago
|
Flags: needinfo?(evilpies)
Assignee | ||
Comment 8•9 years ago
|
||
(In reply to Han Seoul-Oh from comment #7) > // Make sure index is positive. > > Strictly speaking you're ensuring the index is nonnegative, I think. Thanks :)
Attachment #8615954 -
Attachment is obsolete: true
Attachment #8615954 -
Flags: feedback?(jdemooij)
Attachment #8616414 -
Flags: feedback?(jdemooij)
Comment 9•9 years ago
|
||
Comment on attachment 8616414 [details] [diff] [review] Make sure the index is nonnegative in GetElement IC with hole. Review of attachment 8616414 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for taking this, arai! ::: js/src/jit/IonCaches.cpp @@ +3627,5 @@ > // Unbox the index. > masm.unboxInt32(val, indexReg); > > + // Make sure index is nonnegative. > + masm.branch32(Assembler::LessThan, indexReg, Imm32(0), &failures); We should also do this check in the else-branch, in that case it's an unboxed int32 and can still be negative. Would it be possible to write a test for that? You can also move this line after the if-else, but that's a bit complicated because we need to pop |object| in the then-case but not in the else-case...
Attachment #8616414 -
Flags: feedback?(jdemooij) → feedback+
Assignee | ||
Comment 10•9 years ago
|
||
Comment on attachment 8616414 [details] [diff] [review] Make sure the index is nonnegative in GetElement IC with hole. as discussed on IRC, else-branch shouldn't happen. and in that case I think we don't need to move it out of then-branch. I'll file a bug for asserting else-branch never hit, shortly.
Attachment #8616414 -
Flags: review?(jdemooij)
Updated•9 years ago
|
Attachment #8616414 -
Flags: review?(jdemooij) → review+
Comment 12•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/d9040b0c3a99
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox41:
--- → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
Updated•9 years ago
|
Keywords: regression
Comment 13•9 years ago
|
||
Tracked for 39, 40, already fixed in 41.
status-firefox38:
--- → unaffected
status-firefox38.0.5:
--- → unaffected
Comment 14•9 years ago
|
||
I'd like to uplift this to beta (along with whatever else needs fixing from comment 10) Let's not ship this regression in 39! The beta 5 build is this thursday, and next Wednesday June 17 is the last day to get changes in for uplift to beta 7. I'd like not to leave it that late so we have more time to fix things. Thanks!
Assignee | ||
Comment 15•9 years ago
|
||
Comment on attachment 8616414 [details] [diff] [review] Make sure the index is nonnegative in GetElement IC with hole. Same patch is applicable to aurora/beta. About bug 1172470, for now, I'm going to fix that only in m-c. It should be just an unused code, not a critical one. Approval Request Comment > [Feature/regressing bug #] bug 1131531, which was landed to mozilla39 > [User impact if declined] Some JavaScript (like the testcase in comment #0) works wrongly because of the optimization. > [Describe test coverage new/current, TreeHerder] Convered in automated test added by this patch (js/src/jit-test/tests/basic/dense-elements-hole-negative.js), and it's passed on m-i/m-c. > [Risks and why] Low. this patch only disables some optimization paths. The behavior becomes back to previous one. > [String/UUID change made/needed] None
Attachment #8616414 -
Flags: approval-mozilla-beta?
Attachment #8616414 -
Flags: approval-mozilla-aurora?
Comment 16•9 years ago
|
||
Comment on attachment 8616414 [details] [diff] [review] Make sure the index is nonnegative in GetElement IC with hole. Approved for uplift to beta; fixes a regression. Thanks for adding a test!
Attachment #8616414 -
Flags: approval-mozilla-beta?
Attachment #8616414 -
Flags: approval-mozilla-beta+
Attachment #8616414 -
Flags: approval-mozilla-aurora?
Attachment #8616414 -
Flags: approval-mozilla-aurora+
Updated•9 years ago
|
status-firefox39:
--- → affected
status-firefox40:
--- → affected
Updated•9 years ago
|
Assignee: nobody → arai.unmht
You need to log in
before you can comment on or make changes to this bug.
Description
•