Last Comment Bug 740259 - add ALIASEDVAR ops
: add ALIASEDVAR ops
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla14
Assigned To: Luke Wagner [:luke]
:
: Jason Orendorff [:jorendorff]
Mentors:
Depends on: 740442 740443 740445 740446 743407 743408 743409 747554 754503
Blocks: 355480
  Show dependency treegraph
 
Reported: 2012-03-28 22:43 PDT by Luke Wagner [:luke]
Modified: 2012-05-19 07:58 PDT (History)
10 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
if scope then aliased (51.57 KB, patch)
2012-03-28 23:01 PDT, Luke Wagner [:luke]
bhackett1024: review+
Details | Diff | Splinter Review
if aliased then scope (67.90 KB, patch)
2012-03-28 23:06 PDT, Luke Wagner [:luke]
no flags Details | Diff | Splinter Review
fix let-var noting (7.38 KB, patch)
2012-03-28 23:11 PDT, Luke Wagner [:luke]
bhackett1024: review+
Details | Diff | Splinter Review
combined patch for fuzzing (applies to cset 7ed31daf07bd) (123.03 KB, patch)
2012-03-28 23:14 PDT, Luke Wagner [:luke]
no flags Details | Diff | Splinter Review
if aliased then scope (v.2) (69.54 KB, patch)
2012-03-29 00:38 PDT, Luke Wagner [:luke]
bhackett1024: review+
Details | Diff | Splinter Review
combined patch for fuzzing (applies to cset 7ed31daf07bd) (v.2) (1.91 KB, patch)
2012-03-29 00:39 PDT, Luke Wagner [:luke]
no flags Details | Diff | Splinter Review
combined patch for fuzzing (applies to cset 7ed31daf07bd) (v.2) (124.66 KB, patch)
2012-03-29 02:08 PDT, Luke Wagner [:luke]
gary: feedback+
Details | Diff | Splinter Review
combined patch for fuzzing (applies to cset d0430a04d92a) (v.3) (266.91 KB, patch)
2012-04-05 16:19 PDT, Luke Wagner [:luke]
no flags Details | Diff | Splinter Review
combined patch for fuzzing (applies to cset d0430a04d92a) (v.3) (266.96 KB, patch)
2012-04-05 18:26 PDT, Luke Wagner [:luke]
no flags Details | Diff | Splinter Review
combined patch for fuzzing (applies to cset 4e2d0a1a9edd) (v.4) (276.09 KB, patch)
2012-04-09 15:25 PDT, Luke Wagner [:luke]
choller: feedback+
gary: feedback+
Details | Diff | Splinter Review

Description Luke Wagner [:luke] 2012-03-28 22:43:02 PDT
Bug 659577 needs to statically distinguish access to "aliased" variables (args, vars, and let bindings accessed by nested functions or dynamic name operations).  This distinction is made by adding a set of JSOP_*ALIASED* opcodes which replace JSOP_*LOCAL*/JSOP_*ARG* when accessing a arg/var/let that is aliased.  Additionally, these ALIASED opcodes contain immediates that encode the exact location of the aliased var on the scope chain (by number of hops up fp->scopeChain and index of the variable in the scope object).

The two patches in this bug add the frontend analysis and hard assertions that the analysis is correct.  However, at runtime, activation objects still alias the stack frame so the actual runtime behavior isn't really changed (that is for bug 659577).

The first patch adds the "if" invariant:
  - "if a variable is accessed via scope object (Call or Block), it has been statically known to be aliased"
the second patch adds the "only if" invariant:
 - "if a variable is accessed directly (getlocal or getarg), it has been statically known to be unaliased"
Comment 1 Luke Wagner [:luke] 2012-03-28 23:01:07 PDT
Created attachment 610444 [details] [diff] [review]
if scope then aliased

This patch replaces the "calls eval" flag on TreeContext/JSScript with a new, more general "bindings accessed dynamically" flag (see comment on TCF_BINDINGS_ACCESSED_DYNAMICALLY for meaning).  The patch also fixes an existing bug with group assignment.

The only thing kinda gross about this is function statements.  I tried the simple thing first: if any function statement is used, set the deoptimizing "bindings accessed dynamically" flag.  However, this caused several browser leaks in mochitests where a scope now entrained a pointer.  These are a pain to track down (had to do it 3 times to remove flat closures) and I think people actually use function statements (by accident) so it seemed righteous to not totally deoptimize them.  Instead, I only set any local binding with the same name.  This was actually pretty simple and requires only a few lines of code.  Comment explains in more detail.
Comment 2 Luke Wagner [:luke] 2012-03-28 23:06:15 PDT
Created attachment 610448 [details] [diff] [review]
if aliased then scope

This patch adds the opcodes, frontend analysis, and interpreter asserts.  Unfortunately, for silly decompiler reasons, I had some ridealongs: inc, dec, and call variations.  I added a hard assert in Emit3 to catch all VAR/ARG ops and force them to funnel through the new EmitVarOp functions.  EmitVarOp then makes the decision for whether to emit an ALIASED op.

Brian, this touches some frontend code that you modified recently.  Also, could you think carefully about the jsanalyze/jsinfer/methodjit changes?  They were simple, but I'm worried I might be missing some angle.
Comment 3 Luke Wagner [:luke] 2012-03-28 23:11:47 PDT
Created attachment 610449 [details] [diff] [review]
fix let-var noting

This fixes a simple bug where MaybeEmitVarDecl would mistakenly mark let vars as aliased (which would mark some random var whose index matched the let var's stackDepth).
Comment 4 Luke Wagner [:luke] 2012-03-28 23:14:55 PDT
Created attachment 610452 [details] [diff] [review]
combined patch for fuzzing (applies to cset 7ed31daf07bd)

Patch passes tests, but, when it comes to the frontend/decompiler, that means nothing.  Gary and Christian, could you apply a dose of fuzz?
Comment 5 Gary Kwong [:gkw] [:nth10sd] 2012-03-29 00:09:33 PDT
> Patch passes tests, but, when it comes to the frontend/decompiler, that
> means nothing.  Gary and Christian, could you apply a dose of fuzz?

Early result:

function u([]) {
    with( * ) {}
}
uneval(this)

asserts 32-bit debug js shell on Mac 10.7 at Assertion failure: *pc == JSOP_GETARG,
Comment 6 Luke Wagner [:luke] 2012-03-29 00:38:10 PDT
Created attachment 610473 [details] [diff] [review]
if aliased then scope (v.2)

How can anyone forget empty destructuring parameters.  Simple fix.
Comment 7 Luke Wagner [:luke] 2012-03-29 00:39:22 PDT
Created attachment 610475 [details] [diff] [review]
combined patch for fuzzing (applies to cset 7ed31daf07bd) (v.2)
Comment 8 Gary Kwong [:gkw] [:nth10sd] 2012-03-29 01:56:30 PDT
(In reply to Luke Wagner [:luke] from comment #7)
> Created attachment 610475 [details] [diff] [review]
> combined patch for fuzzing (applies to cset 7ed31daf07bd) (v.2)

Unfortunately, this seems to be the patch for bug 733793. :(
Comment 9 Luke Wagner [:luke] 2012-03-29 02:08:42 PDT
Created attachment 610489 [details] [diff] [review]
combined patch for fuzzing (applies to cset 7ed31daf07bd) (v.2)

Oops, sorry
Comment 10 Gary Kwong [:gkw] [:nth10sd] 2012-03-29 10:27:39 PDT
Comment on attachment 610489 [details] [diff] [review]
combined patch for fuzzing (applies to cset 7ed31daf07bd) (v.2)

I also found bug 740446, but otherwise looks good after a round of overnight fuzzing.
Comment 11 Luke Wagner [:luke] 2012-03-29 15:53:18 PDT
Comment on attachment 610473 [details] [diff] [review]
if aliased then scope (v.2)

Oops, totally missing some type barriers.  I also think I need to decompose the INC/DEC ops like INCPROP/ELEM.
Comment 12 Luke Wagner [:luke] 2012-03-29 16:13:25 PDT
*** Bug 740443 has been marked as a duplicate of this bug. ***
Comment 13 Luke Wagner [:luke] 2012-03-29 18:52:40 PDT
*** Bug 740445 has been marked as a duplicate of this bug. ***
Comment 14 Luke Wagner [:luke] 2012-04-05 16:19:42 PDT
Created attachment 612731 [details] [diff] [review]
combined patch for fuzzing (applies to cset d0430a04d92a) (v.3)

Phew.  The last round pointed out just how broken 'arguments' was in the frontend, so I split off bug 740446.  This patch rolls it all up so I humbly submit it for another round o' fuzzin'.
Comment 15 Luke Wagner [:luke] 2012-04-05 18:26:00 PDT
Created attachment 612771 [details] [diff] [review]
combined patch for fuzzing (applies to cset d0430a04d92a) (v.3)

This one even builds on Windows.
Comment 16 Christian Holler (:decoder) 2012-04-06 06:13:08 PDT
I do have 12 bugs to triage from this patch, but as long as I haven't triaged it's not clear if they are caused by the patch or not. Will likely get to them tomorrow :)
Comment 17 Gary Kwong [:gkw] [:nth10sd] 2012-04-07 11:15:03 PDT
> Created attachment 612771 [details] [diff] [review]
> combined patch for fuzzing (applies to cset d0430a04d92a) (v.3)

function h(f, code) {
  var uf = uneval(f);
  var df = disassemble(f);
  var g = eval(uf);
  var dg = disassemble(g);
  if (df == dg) {
    return;
  }
  print("Original code:");
  print(code);
  print("");
  print("Original function:");
  print(uf);
  print(df);
  print("");
  print("Function from recompiling:");
  print(uf);
  print(dg);
  print("");
  print("Disassembly was not stable through decompilation");
}
h(eval("(function(){arguments});"), "arguments");


With your patch, it prints:

Original code:
arguments

Original function:
(function () {})
flags: LAMBDA
loc     op
-----   --
00000:  arguments
00001:  setlocal 0
00004:  pop
main:
00005:  stop

Source notes:
 ofs  line    pc  delta desc     args
---- ---- ----- ------ -------- ------


Function from recompiling:
(function () {})
flags: LAMBDA
loc     op
-----   --
main:
00000:  stop

Source notes:
 ofs  line    pc  delta desc     args
---- ---- ----- ------ -------- ------


Disassembly was not stable through decompilation


on m-c changeset 4721cf101ae6, it does not print anything. Is this related to the patch?
Comment 18 Luke Wagner [:luke] 2012-04-09 09:34:47 PDT
(In reply to Gary Kwong [:gkw, :nth10sd] from comment #17)
As a side-effect of 'arguments' being treated as a local binding (which it is), the expression 'arguments' is now considered effect-free (which it is), so it gets thrown away (just like (function f() { var x; x; }).  Thus, what you are seeing is expected.
Comment 19 Jesse Ruderman 2012-04-09 10:22:09 PDT
That doesn't seem right to me.  If it's effect-free, it should be thrown away in bytecode generation, not in decompilation.  On mozilla-central, it's thrown away in bytecode generation.
Comment 20 Luke Wagner [:luke] 2012-04-09 10:26:51 PDT
It is thrown away during bytecode compilation: note that the "main:" label contains only "stop".  The arguments;setlocal;pop is just the prologue that initializes the 'arguments' binding.  When the uneval-ed function is eval-ed, there is no 'arguments' mentioned at all so there is no prologue.
Comment 21 Luke Wagner [:luke] 2012-04-09 12:52:16 PDT
*** Bug 743409 has been marked as a duplicate of this bug. ***
Comment 22 Luke Wagner [:luke] 2012-04-09 15:25:45 PDT
Created attachment 613419 [details] [diff] [review]
combined patch for fuzzing (applies to cset 4e2d0a1a9edd) (v.4)

Last round found corner cases in bug 740446.  Let's try it one more time.

Green on try (not that that matters when it comes to the front-end ;)
Comment 23 Gary Kwong [:gkw] [:nth10sd] 2012-04-09 20:57:19 PDT
Comment on attachment 613419 [details] [diff] [review]
combined patch for fuzzing (applies to cset 4e2d0a1a9edd) (v.4)

feedback+ after a few hours' fuzzing.
Comment 24 Christian Holler (:decoder) 2012-04-10 04:42:18 PDT
Comment on attachment 613419 [details] [diff] [review]
combined patch for fuzzing (applies to cset 4e2d0a1a9edd) (v.4)

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

No further patch-specific assertions or crashes found over night :)
Comment 27 Luke Wagner [:luke] 2012-04-11 12:03:45 PDT
*** Bug 744501 has been marked as a duplicate of this bug. ***
Comment 28 Luke Wagner [:luke] 2012-04-11 12:12:22 PDT
*** Bug 355480 has been marked as a duplicate of this bug. ***
Comment 29 Luke Wagner [:luke] 2012-04-11 12:21:44 PDT
*** Bug 530650 has been marked as a duplicate of this bug. ***

Note You need to log in before you can comment on or make changes to this bug.