917585 - SpiderMonkey: Relax conditions on optimized arguments in the presence of aliased formals

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Kannan Vijayan [:djvj]]

Currently, we disable magic-arguments optimization when a formal argument is captured by an inner function.  This is done to preserve correct behaviour in cases like the following:

function foo(a) {
  blah(function (x) { a = x; });
  return arguments[0];
}

function blah(f) {
  f(9);
}

print(foo(1));

In the above case, using an optimized arguments value for "foo" will lead to the |arguments[0]| being retreived from its stack frame, not the call object, leading to incorrect output.

However, this is a bit strict.  If the contents of the arguments object are never accessed (e.g. via a GETELEM, or a FUNAPPLY), then we can guarantee that any changes made to the scope object will never be observed through the arguments object.

So, this restriction can be relaxed slightly to be the following:
If a function uses |arguments| in a GETELEM or FUNAPPLY context (i.e. it observes the arguments object contents), and the function contains aliased formals, then disable arguments optimization.

This case is hit in jQuery 1.6.4.  It doesn't seem to show up in the latest jQuery (1.10.2), but it's a pretty simple fix, and this situation may show up in other cases.

Comment 1

[Kannan Vijayan [:djvj]]

Attachment: relax-argsobj-conditions.patch

Adds an |argumentsObjectObserved_| flag to ScriptAnalysis.  Sets that flag when noticing |arguments| object uses within GETELEM or FUNAPPLY cases.  Uses it in |needsArgsObj| check.

Running through try now: https://tbpl.mozilla.org/?tree=Try&rev=149812735730

Comment 2

[Kannan Vijayan [:djvj]]

Requesting fuzzing for this.  Not sure if I'm legitimately relaxing assumptions here, and this sort of thing is a good candidate for having corner cases not covered in jit-tests.

Also kicked off a more comprehensive try:
https://tbpl.mozilla.org/?tree=Try&rev=2239beac9fcd

Comment 3

[Kannan Vijayan [:djvj]]

Comment on attachment 806324 [details] [diff] [review]
relax-argsobj-conditions.patch

Changes as described in comment 0 and comment 1.

Comment 4

[Kannan Vijayan [:djvj]]

Clearing needinfo, rebasing patch, and re-requesting fuzzing.

Comment 5

[Kannan Vijayan [:djvj]]

Attachment: relax-optargs-constraints.patch

Rebased patch.

Comment 6

[Kannan Vijayan [:djvj]]

Patch applies to m-c tip:

parent: 147701:c870b2803c2b tip
 merge b2g-inbound to mozilla-central
branch: default

Comment 7

[Jan de Mooij [:jandem]]

Comment on attachment 806689 [details] [diff] [review]
relax-optargs-constraints.patch

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

Sounds good. Can you add a jit-test similar to the script in comment 0 if we don't have one yet? r=me with that.

Comment 8

[Kannan Vijayan [:djvj]]

Test case added.  Pushed: https://hg.mozilla.org/integration/mozilla-inbound/rev/dae08c3d48bc

Comment 9

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/dae08c3d48bc