Closed Bug 960168 Opened 7 years ago Closed 7 years ago

Reified block scopes should prevent magic optimized arguments

Categories

(Core :: JavaScript Engine, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla29

People

(Reporter: wingo, Assigned: wingo)

References

Details

Attachments

(1 file)

If a function has a JSOP_ARGUMENTS, jsanalyze.cpp will compute an SSA graph for the function in order to see if the arguments object escapes.  However its analysis is not quite right in the presence of block scopes.  Bug 956173 has one error case.

The attached patch makes PUSHBLOCKSCOPE prevent optimization of the arguments object.  This seems to be in spirit with the surrounding GETLOCAL cases.  Note that this does not affect functions that don't reference "arguments"; those for which argumentsHasVarBinding() is false.  (The whole thing is too complicated; see bug 960040.)
Summary: Reified block scopes should cause the arguments optimization to fail → Reified block scopes should prevent magic optimized arguments
Assignee: nobody → wingo
Comment on attachment 8360531 [details] [diff] [review]
Reified block scopes should prevent magic optimized arguments

Like I said, not an optimal patch, but everything else seems to punt on "let", so...
Attachment #8360531 - Flags: review?(luke)
Comment on attachment 8360531 [details] [diff] [review]
Reified block scopes should prevent magic optimized arguments

You're right, everything else does.  Having unaliased let variables live in the (nfixed, nslots) tends to complicate things for both jsinfer/jsanalyze/ion, so it gets ignored.  The "obvious" fix, it seems, would be to stick unaliased block-scoped variables in the [0, nfixed) range (reusing slots for disjointly scoped variables).  By any chance is this already in your queue?  ISTR hearing you mention it.
Attachment #8360531 - Flags: review?(luke) → review+
Tx for the review.  Putting let vars in the nfixed range would make a lot of sense, but it's not currently in my queue.  I think I should get generators baseline jitting first :)
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/45a6f3480c59
Status: NEW → RESOLVED
Closed: 7 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
You need to log in before you can comment on or make changes to this bug.