Closed Bug 593556 Opened 10 years ago Closed 10 years ago

JM: variable from outer scope is not seen in "with" block

Categories

(Core :: JavaScript Engine, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
Tracking Status
blocking2.0 --- betaN+

People

(Reporter: jruderman, Assigned: dvander)

References

(Blocks 2 open bugs)

Details

(Keywords: regression, testcase, Whiteboard: fixed-in-tracemonkey[suspect-regress-ts_cold_generated_med][suspect-regress-dromaeo_css][suspect-regress-dromaeo_jslib])

Attachments

(1 file)

(function () {
    var c = 3;
    (function () {
        with({}) {
            print(c);
        }
    })();
})();

Interpreter: 3
Method JIT:  undefined

The first bad revision is:
  changeset:   52276:9852618873d9
  user:        David Anderson <danderson@mozilla.com>
  date:        Mon Jul 12 15:00:58 2010 -0700
  summary:     [JAEGER] Remove stores from OOL transitions when
               not needed (bug 573083).
blocking2.0: --- → ?
This may be folded into bug 585524, which is closely related.
Assignee: general → dvander
blocking2.0: ? → betaN+
Attached patch fix attemptSplinter Review
Most of the diff is unindenting. This patch makes primaryExpr() build use-def information for names inside |with| statements, however, it makes sure to mark the names as deoptimized.

NewBindingNode no longer steals an existing pn if it's inside a |with|.

BindNameToSlot now makes sure not to emit GETGNAME for deoptimized definitions. (GETGNAME says that the name is either on the global object or is a ReferenceError.)
Attachment #473807 - Flags: review?(brendan)
Blocks: 585524
No longer depends on: 585524
Comment on attachment 473807 [details] [diff] [review]
fix attempt

>+            bool inWith = stmt && stmt->type == STMT_WITH;

No need for this bool, just test (stmt && stmt->type == STMT_WITH) later where you make single use of it:

>+            if (inWith)
>+                pn->pn_dflags = PND_DEOPTIMIZED;

But clobbering dflags is not a good idea if something precomputed other flags. Why not wait a bit more for where dflags is |='ed:

>+
>+            JS_ASSERT(dn->pn_defn);
>+            LinkUseToDef(pn, dn, tc);
>+
>+            /* Here we handle the backward function reference case. */
>+            if (tokenStream.peekToken() != TOK_LP)
>+                dn->pn_dflags |= PND_FUNARG;
>+
>+            pn->pn_dflags |= (dn->pn_dflags & PND_FUNARG);

and do the PND_DEOPTIMIZED |= here to keep all such together?

Looks good otherwise -- does it fix the testcases with 'with' and 'function' sub-statements?

/be
Attachment #473807 - Flags: review?(brendan) → review+
That clobbering was a typo. Thanks for catching.

No, it does not fix the other bugs on file. Those need more changes to BindVarOrConst, I think.
http://hg.mozilla.org/mozilla-central/rev/eba54c4edd6f
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Whiteboard: fixed-in-tracemonkey → fixed-in-tracemonkey[suspect-regress-ts_cold_generated_med]
A changeset from this bug was associated with a XP Ts, Cold MED Dirty Profile regression on Firefox. boo-urns :(

  Previous: avg 420.356 stddev 3.679 of 30 runs up to a9d1ad0bc386
  New     : avg 430.832 stddev 1.905 of 5 runs since a60414d076b5
  Change  : +10.475 (2.49% / z=2.847)
  Graph   : http://mzl.la/bZFaB3

The regression occurred from changesets in the following range:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=a9d1ad0bc386&tochange=a60414d076b5

The tag [suspect-regress-ts_cold_generated_med] has been added to the status whiteboard;
please remove it only once you have confirmed this bug is not the cause
of the regression.
Whiteboard: fixed-in-tracemonkey[suspect-regress-ts_cold_generated_med] → fixed-in-tracemonkey[suspect-regress-ts_cold_generated_med][suspect-regress-dromaeo_css]
A changeset from this bug was associated with a Win7 Dromaeo (CSS) regression on Firefox. boo-urns :(

  Previous: avg 2014.419 stddev 40.480 of 30 runs up to a9d1ad0bc386
  New     : avg 1901.610 stddev 12.432 of 5 runs since a60414d076b5
  Change  : -112.809 (-5.6% / z=2.787)
  Graph   : http://mzl.la/9gFu4n

The regression occurred from changesets in the following range:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=a9d1ad0bc386&tochange=a60414d076b5

The tag [suspect-regress-dromaeo_css] has been added to the status whiteboard;
please remove it only once you have confirmed this bug is not the cause
of the regression.
Whiteboard: fixed-in-tracemonkey[suspect-regress-ts_cold_generated_med][suspect-regress-dromaeo_css] → fixed-in-tracemonkey[suspect-regress-ts_cold_generated_med][suspect-regress-dromaeo_css][suspect-regress-dromaeo_jslib]
A changeset from this bug was associated with a Win7 Dromaeo (jslib) regression on Firefox. boo-urns :(

  Previous: avg 127.610 stddev 4.222 of 30 runs up to a9d1ad0bc386
  New     : avg 116.384 stddev 0.751 of 5 runs since a60414d076b5
  Change  : -11.226 (-8.8% / z=2.659)
  Graph   : http://mzl.la/bZu5UP

The regression occurred from changesets in the following range:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=a9d1ad0bc386&tochange=a60414d076b5

The tag [suspect-regress-dromaeo_jslib] has been added to the status whiteboard;
please remove it only once you have confirmed this bug is not the cause
of the regression.
A changeset from this bug was associated with a XP Dromaeo (CSS) regression on Firefox. boo-urns :(

  Previous: avg 2045.275 stddev 49.676 of 30 runs up to a9d1ad0bc386
  New     : avg 1936.120 stddev 13.937 of 5 runs since a60414d076b5
  Change  : -109.155 (-5.34% / z=2.197)
  Graph   : http://mzl.la/b0dlou

The regression occurred from changesets in the following range:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=a9d1ad0bc386&tochange=a60414d076b5

The tag [suspect-regress-dromaeo_css] has been added to the status whiteboard;
please remove it only once you have confirmed this bug is not the cause
of the regression.
A changeset from this bug was associated with a XP Dromaeo (jslib) regression on Firefox. boo-urns :(

  Previous: avg 129.703 stddev 4.099 of 30 runs up to a9d1ad0bc386
  New     : avg 117.954 stddev 0.660 of 5 runs since a60414d076b5
  Change  : -11.749 (-9.06% / z=2.866)
  Graph   : http://mzl.la/avZij4

The regression occurred from changesets in the following range:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=a9d1ad0bc386&tochange=a60414d076b5

The tag [suspect-regress-dromaeo_jslib] has been added to the status whiteboard;
please remove it only once you have confirmed this bug is not the cause
of the regression.
You need to log in before you can comment on or make changes to this bug.