Clean up JSOP_DEFFUN and duplicated methodjit StubCall logic, fixing latent arguments override bug

RESOLVED FIXED in mozilla2.0b7

Status

()

P3
normal
RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: brendan, Assigned: brendan)

Tracking

Trunk
mozilla2.0b7
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: fixed-in-tracemonkey)

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

8 years ago
Followup for bug 599009. More details and even a patch in a few.

/be
(Assignee)

Comment 1

8 years ago
Ok, patch is all about clearer control flow, logically minimized assertions (and more assertions), and better comments. I don't think there's a substantive change beyond these.

/be
Status: NEW → ASSIGNED
(Assignee)

Comment 2

8 years ago
Created attachment 481657 [details] [diff] [review]
proposed fix

bz, jorendorff: feel free to have a look too -- would welcome your feedback.

/be
Attachment #481657 - Flags: review?(igor)

Comment 3

8 years ago
Comment on attachment 481657 [details] [diff] [review]
proposed fix

>diff --git a/js/src/jsinterp.cpp b/js/src/jsinterp.cpp
>@@ -5361,46 +5361,51 @@ BEGIN_CASE(JSOP_DEFFUN)
>+        } else if (parent->isCall()) {
>+            JS_ASSERT(parent == pobj);
>+
>+            uintN oldAttrs = ((Shape *) prop)->attributes();
>+            JS_ASSERT(!(oldAttrs & (JSPROP_READONLY | JSPROP_GETTER | JSPROP_SETTER)));
>+
>             /*
>+             * We may be processing a function sub-statement in global or
>+             * function code: assign rather than redefine if the essential
>+             * attributes are not changing.
>              */
>+            if ((oldAttrs & (JSPROP_ENUMERATE | JSPROP_PERMANENT)) == attrs)
>+                doSet = true;

Nit: here attrs is always JSPROP_ENUMERATE | JSPROP_PERMANENT. The comments and code should reflect that.
Attachment #481657 - Flags: review?(igor) → review+

Comment 4

8 years ago
(In reply to comment #3)
> >             /*
> >+             * We may be processing a function sub-statement in global or
> >+             * function code: assign rather than redefine if the essential
> >+             * attributes are not changing.
> >              */
> >+            if ((oldAttrs & (JSPROP_ENUMERATE | JSPROP_PERMANENT)) == attrs)
> >+                doSet = true;
> 
> Nit: here attrs is always JSPROP_ENUMERATE | JSPROP_PERMANENT. The comments and
> code should reflect that.

And given that and the fact that the Call object does not leak that if almost equivalent to the check that prop was not defined in the eval. The only exception is the arguments name. Its properties are JSPROP_PERMANENT | JSPROP_SHARED so given the above the doSet will be false and the arguments name will be redeclared. But that erases its getter and setter leading to a bug as the following prints "[object Arguments]", not the function source.

function test(arg) {
    eval(arg);
    {
	function arguments() { return 1; }
    }
    print(arguments);
}

test();

This can be fixed if the check would simply become:

if (!(oldAttrs & JSPROP_PERMANENT))
    doSet = true;

Comment 5

8 years ago
The above bug even exists even for DEFLOCALFUN as the following also prints the arguments object.

function test(arg) {
    function arguments() { return 1; }
    return arguments;
}

dis(test);
print(test());

Comment 6

8 years ago
(In reply to comment #4)
> This can be fixed if the check would simply become:
> 
> if (!(oldAttrs & JSPROP_PERMANENT))
>     doSet = true;


I meant if (oldAttrs & JSPROP_PERMANENT) here.
(Assignee)

Comment 7

8 years ago
(In reply to comment #6)
> (In reply to comment #4)
> > This can be fixed if the check would simply become:
> > 
> > if (!(oldAttrs & JSPROP_PERMANENT))
> >     doSet = true;
> 
> I meant if (oldAttrs & JSPROP_PERMANENT) here.

This is much better.

BTW, I was not sure which generated better code, given the knowledge that attrs was (JSPROP_ENUMERATE | JSPROP_PERMANENT). So I wrote this test program:

#include <stdio.h>
#include <stdlib.h>

int f1(int a, int b) {
    return (a & (1 | 2)) == b;
}

int f2(int a) {
    return (a & (1 | 2)) == (1 | 2);
}

int f3(int a) {
    return (~a & (1 | 2)) == 0;
}

int main(int argc, char **argv) {
    int a = (argc > 1) ? atoi(argv[1]) : 0;
    int b = (1 | 2);
    printf("%d\n", f1(a, b));
    printf("%d\n", f2(a));
    printf("%d\n", f3(a));
    return 0;
}

The disassembly from gdb:

0x100000db0 <_Z2f1ii>:	push   %rbp
0x100000db1 <_Z2f1ii+1>:	mov    %rsp,%rbp
0x100000db4 <_Z2f1ii+4>:	and    $0x3,%edi
0x100000db7 <_Z2f1ii+7>:	xor    %eax,%eax
0x100000db9 <_Z2f1ii+9>:	cmp    %esi,%edi
0x100000dbb <_Z2f1ii+11>:	sete   %al
0x100000dbe <_Z2f1ii+14>:	leaveq 
0x100000dbf <_Z2f1ii+15>:	retq   
0x100000dc0 <_Z2f2i>:	push   %rbp
0x100000dc1 <_Z2f2i+1>:	mov    %rsp,%rbp
0x100000dc4 <_Z2f2i+4>:	and    $0x3,%edi
0x100000dc7 <_Z2f2i+7>:	xor    %eax,%eax
0x100000dc9 <_Z2f2i+9>:	cmp    $0x3,%edi
0x100000dcc <_Z2f2i+12>:	sete   %al
0x100000dcf <_Z2f2i+15>:	leaveq 
0x100000dd0 <_Z2f2i+16>:	retq   
0x100000dd1 <_Z2f2i+17>:	nopl   0x0(%rax)
0x100000dd8 <_Z2f2i+24>:	nopl   0x0(%rax,%rax,1)
0x100000de0 <_Z2f3i>:	push   %rbp
0x100000de1 <_Z2f3i+1>:	mov    %rsp,%rbp
0x100000de4 <_Z2f3i+4>:	not    %edi
0x100000de6 <_Z2f3i+6>:	xor    %eax,%eax
0x100000de8 <_Z2f3i+8>:	test   $0x3,%dil
0x100000dec <_Z2f3i+12>:	sete   %al
0x100000def <_Z2f3i+15>:	leaveq 
0x100000df0 <_Z2f3i+16>:	retq   

f1 is 16 bytes, f2 is 25, f3 is 17. This is why I stuck with the expression in the last patch. This may not be a fair test, though.But it's moot, thanks to your finding the pre-existing arguments bug. Just testing for permanent fixes that and directly expresses the intent. It does however count on enumerate being consistent among all the possible properties of a Call object, but that can't matter. I'll assert. New patch in a bit.

/be
(Assignee)

Comment 8

8 years ago
Hrm, textarea lost a line break (two actually) after the period in "though.But" in my last comment. Minefield bug?

/be
(Assignee)

Comment 9

8 years ago
bla bla bla.

bla bla bla.

bla bla bla bla bla bla bla bla bla blaaaaaaaab labbbbbbbbbbb blbaaaaaaaa
(Assignee)

Comment 10

8 years ago
(In reply to comment #5)
> The above bug even exists even for DEFLOCALFUN as the following also prints the
> arguments object.
> 
> function test(arg) {
>     function arguments() { return 1; }
>     return arguments;
> }
> 
> dis(test);
> print(test());

This JSOP_DEFLOCALFUN variant is bug 530650. Luke has it, I hope it will be fixed soon. I can take it if that would help.

/be
Summary: Clean up JSOP_DEFFUN and duplicated methodjit StubCall logic → Clean up JSOP_DEFFUN and duplicated methodjit StubCall logic, fixing latent arguments override bug
(Assignee)

Comment 11

8 years ago
Created attachment 481888 [details] [diff] [review]
proposed fix, v2 (with test)
Attachment #481657 - Attachment is obsolete: true
Attachment #481888 - Flags: review?(igor)
(Assignee)

Comment 12

8 years ago
Bugzilla interdiff whines but works. Wish it had more self-confidence and just worked...

/be

Comment 13

8 years ago
Comment on attachment 481888 [details] [diff] [review]
proposed fix, v2 (with test)

>diff --git a/js/src/jsfun.cpp b/js/src/jsfun.cpp
>+        } else if (parent->isCall()) {
...
>             /*
>+             * We may be processing a function sub-statement in global or
>+             * function code: assign rather than redefine if the essential

Nit: Given the if the code cannot be gloabl.
Attachment #481888 - Flags: review?(igor) → review+
(Assignee)

Comment 14

8 years ago
http://hg.mozilla.org/tracemonkey/rev/25c733afa957

/be
Whiteboard: fixed-in-tracemonkey

Comment 15

8 years ago
http://hg.mozilla.org/mozilla-central/rev/25c733afa957
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
Target Milestone: mozilla2.0b8 → mozilla2.0b7
You need to log in before you can comment on or make changes to this bug.