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

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P3)

Description

[Brendan Eich [:brendan]]

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

/be

Comment 1

[Brendan Eich [:brendan]]

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

Comment 2

[Brendan Eich [:brendan]]

Attachment: proposed fix

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

/be

Comment 3

[Igor Bukanov]

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.

Comment 4

[Igor Bukanov]

(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

[Igor Bukanov]

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

[Igor Bukanov]

(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.

Comment 7

[Brendan Eich [:brendan]]

(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

Comment 8

[Brendan Eich [:brendan]]

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

/be

Comment 9

[Brendan Eich [:brendan]]

bla bla bla.

bla bla bla.

bla bla bla bla bla bla bla bla bla blaaaaaaaab labbbbbbbbbbb blbaaaaaaaa

Comment 10

[Brendan Eich [:brendan]]

(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

Comment 11

[Brendan Eich [:brendan]]

Attachment: proposed fix, v2 (with test)

Comment 12

[Brendan Eich [:brendan]]

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

/be

Comment 13

[Igor Bukanov]

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.

Comment 14

[Brendan Eich [:brendan]]

http://hg.mozilla.org/tracemonkey/rev/25c733afa957

/be

Comment 15

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/25c733afa957