Last Comment Bug 772328 - function statements shouldn't shadow formals
: function statements shouldn't shadow formals
Status: RESOLVED FIXED
[js:p2]
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla17
Assigned To: Luke Wagner [:luke]
:
Mentors:
Depends on:
Blocks: 775323
  Show dependency treegraph
 
Reported: 2012-07-09 20:08 PDT by Luke Wagner [:luke]
Modified: 2012-08-09 08:46 PDT (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
simplify MakeDefIntoUse (11.38 KB, patch)
2012-07-25 16:28 PDT, Luke Wagner [:luke]
no flags Details | Diff | Review
add IsArgOp and IsLocalOp (patch 2) (5.71 KB, patch)
2012-07-25 16:30 PDT, Luke Wagner [:luke]
ejpbruel: review+
Details | Diff | Review
fix and test (patch 3) (30.99 KB, patch)
2012-07-25 16:37 PDT, Luke Wagner [:luke]
no flags Details | Diff | Review
combined patch for fuzzing (applies to cset 7ad3878167c1) (56.95 KB, patch)
2012-07-25 16:42 PDT, Luke Wagner [:luke]
gary: feedback+
choller: feedback-
Details | Diff | Review
simplify MakeDefIntoUse (patch 1) (11.87 KB, patch)
2012-07-25 18:29 PDT, Luke Wagner [:luke]
no flags Details | Diff | Review
combined patch for fuzzing (applies to cset 462036803a34) (45.31 KB, patch)
2012-07-25 18:32 PDT, Luke Wagner [:luke]
choller: feedback-
Details | Diff | Review
fix and test (patch 3) (32.10 KB, patch)
2012-07-26 11:49 PDT, Luke Wagner [:luke]
ejpbruel: review+
Details | Diff | Review
combined for fuzzing (applies to cset 462036803a34) (46.46 KB, patch)
2012-07-26 12:17 PDT, Luke Wagner [:luke]
gary: feedback-
Details | Diff | Review
simplify MakeDefIntoUse (patch 1) (12.53 KB, patch)
2012-07-26 15:15 PDT, Luke Wagner [:luke]
ejpbruel: review+
Details | Diff | Review
combined patch for fuzzing (applies to cset 462036803a34) (47.12 KB, patch)
2012-07-26 15:35 PDT, Luke Wagner [:luke]
gary: feedback+
Details | Diff | Review
simplify ParseNode::resolve (patch 4) (1.34 KB, patch)
2012-08-07 16:06 PDT, Luke Wagner [:luke]
ejpbruel: review+
Details | Diff | Review

Description Luke Wagner [:luke] 2012-07-09 20:08:58 PDT
IIUC, this should return 42:

  function f(x) {
    function x() {}
    arguments[0] = 42;
    return x
  }

The reason is that Parser::functionDef calls addVariable if there is an existing formal binding.

Note: this offers a new opportunity for (undetected) shapes with duplicate ids.
Comment 1 Luke Wagner [:luke] 2012-07-09 20:09:30 PDT
(dmandelin needs [:dmandelin])
Comment 2 David Mandelin [:dmandelin] 2012-07-10 10:37:46 PDT
(In reply to Luke Wagner [:luke] from comment #1)
> (dmandelin needs [:dmandelin])

Heh, I never put one in because I figured my name chars were unique enough, so I was always waiting for someone to ask. :-)
Comment 3 Nicholas Nethercote [:njn] 2012-07-11 02:53:57 PDT
> (dmandelin needs [:dmandelin])

Hear hear!  This has caught me lots of times;  I reflexively put a ':' in front of everybody's names.
Comment 4 Luke Wagner [:luke] 2012-07-25 16:25:15 PDT
In addition to being wrong, the function-shadowing-formals gets in the way of bug 775323.
Comment 5 Luke Wagner [:luke] 2012-07-25 16:28:59 PDT
Created attachment 645931 [details] [diff] [review]
simplify MakeDefIntoUse

As explained in the comment added by this patch, MakeDefIntoUse is called when a function statement redeclares another function.  Currently we handle this by turning the former into a name that uses the latter.  We then hack the ParseNode's op to be JSOP_NOP which is a signal to the emitter not to do anything.  This causes trouble for a later patch.
Comment 6 Luke Wagner [:luke] 2012-07-25 16:30:01 PDT
Created attachment 645932 [details] [diff] [review]
add IsArgOp and IsLocalOp (patch 2)

I'm tired of forgetting whether it's JOF_OPMODE or JOF_OPTYPE...
Comment 7 Luke Wagner [:luke] 2012-07-25 16:37:45 PDT
Created attachment 645937 [details] [diff] [review]
fix and test (patch 3)

This patch fixes the bug by making the op of the PNK_FUNC node tell whether the function is stored in an arg or local.  This patch also cleans up the logic in Parser::functionDef to be more understandable and conducive to the changes coming in bug 775323 (viz., avoiding the querying of bindings).
Comment 8 Luke Wagner [:luke] 2012-07-25 16:42:29 PDT
Created attachment 645942 [details] [diff] [review]
combined patch for fuzzing (applies to cset 7ad3878167c1)

These patches assert and depend on several new frontend invariants and change some core frontend bits.  If you guys have time, do you suppose you could point the fuzzers at this patch and see if they find anything?
Comment 9 Christian Holler (:decoder) 2012-07-25 17:12:21 PDT
Comment on attachment 645942 [details] [diff] [review]
combined patch for fuzzing (applies to cset 7ad3878167c1)

function F(x, y) {}
function test(N) {
  var src = F.toSource(-1)+"\n";
  src = Array(N + 1).join(src);
  eval(src);
}
for (var i = 0; i != 10; ++i) {
  var time = test(1000 + i * 2000);
}


asserts with

Assertion failure: pnu->isUsed(), at js/src/frontend/Parser.cpp:878
Comment 10 Gary Kwong [:gkw] [:nth10sd] 2012-07-25 18:00:37 PDT
Comment on attachment 645942 [details] [diff] [review]
combined patch for fuzzing (applies to cset 7ad3878167c1)

I could spare a few mins for this - nothing severe immediately found on my side.
Comment 11 Luke Wagner [:luke] 2012-07-25 18:29:47 PDT
Created attachment 645987 [details] [diff] [review]
simplify MakeDefIntoUse (patch 1)

Phew, thanks!  Here is the fixed patch.
Comment 12 Luke Wagner [:luke] 2012-07-25 18:32:40 PDT
Created attachment 645989 [details] [diff] [review]
combined patch for fuzzing (applies to cset 462036803a34)

If you have any more time (no rush), it'd be great to see if this patch holds up
Comment 13 Christian Holler (:decoder) 2012-07-26 04:35:40 PDT
Comment on attachment 645989 [details] [diff] [review]
combined patch for fuzzing (applies to cset 462036803a34)

(function (call) { 
  function call() { } 
}).abstract.f++;


asserts with:

Assertion failure: *nextpc == JSOP_SETLOCAL || *nextpc == JSOP_SETALIASEDVAR, at js/src/jsopcode.cpp:4796
Comment 14 Luke Wagner [:luke] 2012-07-26 11:38:09 PDT
*shakes fist at decompiler; hasn't Benjamin removed that yet?
Comment 15 Luke Wagner [:luke] 2012-07-26 11:43:36 PDT
Ah, this is just an assertion fix.  decoder, was this the only failure?  If this is fixed, do you think the patch passes fuzzing or did fuzzing not run long before finding this?
Comment 16 Luke Wagner [:luke] 2012-07-26 11:49:43 PDT
Created attachment 646235 [details] [diff] [review]
fix and test (patch 3)
Comment 17 Luke Wagner [:luke] 2012-07-26 12:17:57 PDT
Created attachment 646255 [details] [diff] [review]
combined for fuzzing (applies to cset 462036803a34)
Comment 18 Gary Kwong [:gkw] [:nth10sd] 2012-07-26 12:33:29 PDT
Comment on attachment 646255 [details] [diff] [review]
combined for fuzzing (applies to cset 462036803a34)

(function() {
  var x = (function() {})
  function x() {}
  function x() {}
})()

Assertion failure: pnu->isUsed(),
Comment 19 Luke Wagner [:luke] 2012-07-26 15:15:02 PDT
Created attachment 646364 [details] [diff] [review]
simplify MakeDefIntoUse (patch 1)

Phew, painful.  Mutating parse node trees in place is not a good idea and fraught with peril.  Thanks Gary and Christian!
Comment 20 Luke Wagner [:luke] 2012-07-26 15:35:17 PDT
Created attachment 646369 [details] [diff] [review]
combined patch for fuzzing (applies to cset 462036803a34)
Comment 21 Gary Kwong [:gkw] [:nth10sd] 2012-07-27 11:18:49 PDT
Comment on attachment 646369 [details] [diff] [review]
combined patch for fuzzing (applies to cset 462036803a34)

A night of fuzzing on my MBP did not throw up anything weird. \o/
Comment 22 Luke Wagner [:luke] 2012-07-27 11:28:38 PDT
Phew, thanks again!
Comment 23 Luke Wagner [:luke] 2012-08-07 16:06:54 PDT
Created attachment 649856 [details] [diff] [review]
simplify ParseNode::resolve (patch 4)

A nice side-effect of the MakeDefIntoUse change is that we don't have this unbounded chain of definitions-converted-into-assignments which allows a nice simplification of ParseNode::resolve.
Comment 24 Eddy Bruel [:ejpbruel] 2012-08-08 09:39:05 PDT
Comment on attachment 646235 [details] [diff] [review]
fix and test (patch 3)

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

This patch looks alright to me, but not 100% sure I didn't miss anything. Double checked some stuff with jorendorff over irc to make sure. Should be ok to r+, since you ran this stuff through the fuzzer.
Comment 25 Eddy Bruel [:ejpbruel] 2012-08-08 11:22:42 PDT
Comment on attachment 646364 [details] [diff] [review]
simplify MakeDefIntoUse (patch 1)

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

I got nothing
Comment 26 Eddy Bruel [:ejpbruel] 2012-08-08 11:36:38 PDT
Comment on attachment 649856 [details] [diff] [review]
simplify ParseNode::resolve (patch 4)

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

Looks good. Glad to see this being simplified. Thanks Luke!
Comment 29 :Ms2ger 2012-08-09 05:14:56 PDT
Comment on attachment 646235 [details] [diff] [review]
fix and test (patch 3)

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

::: js/src/frontend/BytecodeEmitter.cpp
@@ +1202,5 @@
>  static bool
>  BindNameToSlot(JSContext *cx, BytecodeEmitter *bce, ParseNode *pn)
>  {
>      JS_ASSERT(pn->isKind(PNK_NAME));
> +    JS_ASSERT_IF(pn->isKind(PNK_FUNCTION), pn->isBound());

Can pn->isKind(PNK_FUNCTION) be true if the assert above this passes?
Comment 30 Luke Wagner [:luke] 2012-08-09 08:46:25 PDT
Good catch!  That is a left-over from earlier iterations of the patch.  I'll remove that in bug 775323.

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