function statements shouldn't shadow formals

RESOLVED FIXED in mozilla17

Status

()

Core
JavaScript Engine
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: luke, Assigned: luke)

Tracking

unspecified
mozilla17
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [js:p2])

Attachments

(5 attachments, 6 obsolete attachments)

5.71 KB, patch
ejpbruel
: review+
Details | Diff | Splinter Review
32.10 KB, patch
ejpbruel
: review+
Details | Diff | Splinter Review
12.53 KB, patch
ejpbruel
: review+
Details | Diff | Splinter Review
47.12 KB, patch
Details | Diff | Splinter Review
1.34 KB, patch
ejpbruel
: review+
Details | Diff | Splinter Review
(Assignee)

Description

5 years ago
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.
(Assignee)

Comment 1

5 years ago
(dmandelin needs [:dmandelin])
(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. :-)
> (dmandelin needs [:dmandelin])

Hear hear!  This has caught me lots of times;  I reflexively put a ':' in front of everybody's names.
Whiteboard: [js:p2]
(Assignee)

Comment 4

5 years ago
In addition to being wrong, the function-shadowing-formals gets in the way of bug 775323.
Blocks: 775323
(Assignee)

Comment 5

5 years ago
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.
Assignee: general → luke
Status: NEW → ASSIGNED
Attachment #645931 - Flags: review?(ejpbruel)
(Assignee)

Comment 6

5 years ago
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...
Attachment #645932 - Flags: review?(ejpbruel)
(Assignee)

Comment 7

5 years ago
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).
Attachment #645937 - Flags: review?(ejpbruel)
(Assignee)

Comment 8

5 years ago
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?
Attachment #645942 - Flags: feedback?(gary)
Attachment #645942 - Flags: feedback?(choller)
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
Attachment #645942 - Flags: feedback?(choller) → feedback-
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.
Attachment #645942 - Flags: feedback?(gary) → feedback+
(Assignee)

Comment 11

5 years ago
Created attachment 645987 [details] [diff] [review]
simplify MakeDefIntoUse (patch 1)

Phew, thanks!  Here is the fixed patch.
Attachment #645931 - Attachment is obsolete: true
Attachment #645931 - Flags: review?(ejpbruel)
Attachment #645987 - Flags: review?(ejpbruel)
(Assignee)

Updated

5 years ago
Attachment #645987 - Attachment description: simplify MakeDefIntoUse → simplify MakeDefIntoUse (patch 1)
(Assignee)

Updated

5 years ago
Attachment #645932 - Attachment description: add IsArgOp and IsLocalOp → add IsArgOp and IsLocalOp (patch 2)
(Assignee)

Updated

5 years ago
Attachment #645937 - Attachment description: fix and test → fix and test (patch 3)
(Assignee)

Comment 12

5 years ago
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
Attachment #645942 - Attachment is obsolete: true
Attachment #645989 - Flags: feedback?(gary)
Attachment #645989 - Flags: feedback?(choller)
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
Attachment #645989 - Flags: feedback?(choller) → feedback-
(Assignee)

Comment 14

5 years ago
*shakes fist at decompiler; hasn't Benjamin removed that yet?
(Assignee)

Comment 15

5 years ago
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?
(Assignee)

Comment 16

5 years ago
Created attachment 646235 [details] [diff] [review]
fix and test (patch 3)
Attachment #645937 - Attachment is obsolete: true
Attachment #645937 - Flags: review?(ejpbruel)
(Assignee)

Comment 17

5 years ago
Created attachment 646255 [details] [diff] [review]
combined for fuzzing (applies to cset 462036803a34)
Attachment #645989 - Attachment is obsolete: true
Attachment #645989 - Flags: feedback?(gary)
Attachment #646255 - Flags: feedback?(gary)
(Assignee)

Updated

5 years ago
Attachment #646235 - Flags: review?(ejpbruel)
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(),
Attachment #646255 - Flags: feedback?(gary) → feedback-
(Assignee)

Comment 19

5 years ago
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!
Attachment #645987 - Attachment is obsolete: true
Attachment #646255 - Attachment is obsolete: true
Attachment #645987 - Flags: review?(ejpbruel)
Attachment #646364 - Flags: review?(ejpbruel)
(Assignee)

Comment 20

5 years ago
Created attachment 646369 [details] [diff] [review]
combined patch for fuzzing (applies to cset 462036803a34)
Attachment #646369 - Flags: feedback?(gary)
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/
Attachment #646369 - Flags: feedback?(gary) → feedback+
(Assignee)

Comment 22

5 years ago
Phew, thanks again!

Updated

5 years ago
Attachment #645932 - Flags: review?(ejpbruel) → review+
(Assignee)

Comment 23

5 years ago
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.
Attachment #649856 - Flags: review?(ejpbruel)
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.
Attachment #646235 - Flags: review?(ejpbruel) → review+
Comment on attachment 646364 [details] [diff] [review]
simplify MakeDefIntoUse (patch 1)

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

I got nothing
Attachment #646364 - Flags: review?(ejpbruel) → review+
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!
Attachment #649856 - Flags: review?(ejpbruel) → review+
(Assignee)

Comment 27

5 years ago
Thank you

https://hg.mozilla.org/integration/mozilla-inbound/rev/bb94aecac98f
https://hg.mozilla.org/integration/mozilla-inbound/rev/e8153d805296
https://hg.mozilla.org/integration/mozilla-inbound/rev/441cf1a50af8
https://hg.mozilla.org/integration/mozilla-inbound/rev/5ea43af33ec9
Target Milestone: --- → mozilla17
https://hg.mozilla.org/mozilla-central/rev/bb94aecac98f
https://hg.mozilla.org/mozilla-central/rev/e8153d805296
https://hg.mozilla.org/mozilla-central/rev/441cf1a50af8
https://hg.mozilla.org/mozilla-central/rev/5ea43af33ec9
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
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?
(Assignee)

Comment 30

5 years ago
Good catch!  That is a left-over from earlier iterations of the patch.  I'll remove that in bug 775323.
You need to log in before you can comment on or make changes to this bug.