453492 - DecompileDestructuringLHS: "Assertion failure: op == JSOP_ENUMELEM || op == JSOP_ENUMCONSTELEM"

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P3)

Description

[Jesse Ruderman]

./js
js> print(function() { [(let(a)1)[2]] = 3; });

Assertion failure: op == JSOP_ENUMELEM || op == JSOP_ENUMCONSTELEM, at jsopcode.cpp:1389

(mozilla-central, no tracing needed)

Comment 1

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

After bug 472450 was fixed, this bug became re-occurring more often, similar to  bug 416737. Both are non-TM related cases.

Nominating blocking1.9.1 because of this.

Comment 2

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

(In reply to comment #1)
> Nominating blocking1.9.1 because of this.

This doesn't occur on Gecko 1.9.0.x branch, I think this is a regression. Please consider wanted1.9.1 since it's marked blocking1.9.1- for whatever reasons.


for 1.9.0.x branch:
=====
$ ./js-dbg-moz190-intelmac 
js> print(function() { [(let(a)1)[2]] = 3; });
function () {
    [(let (a) 1)[2]] = 3;
}
js> 

gdb backtrace for shell from TM branch without -j:
=====
js> print(function() { [(let(a)1)[2]] = 3; });
Assertion failure: op == JSOP_ENUMELEM || op == JSOP_ENUMCONSTELEM, at ../jsopcode.cpp:1418
[New Thread 0xb7d416c0 (LWP 11614)]

Program received signal SIGTRAP, Trace/breakpoint trap.
[Switching to Thread 0xb7d416c0 (LWP 11614)]
JS_Assert (s=0x81e8b08 "op == JSOP_ENUMELEM || op == JSOP_ENUMCONSTELEM", file=0x81e6de5 "../jsopcode.cpp", ln=1418) at ../jsutil.cpp:68
68	    abort();
(gdb) bt
#0  JS_Assert (s=0x81e8b08 "op == JSOP_ENUMELEM || op == JSOP_ENUMCONSTELEM", file=0x81e6de5 "../jsopcode.cpp", ln=1418) at ../jsutil.cpp:68
#1  0x080dd3c2 in DecompileDestructuringLHS (ss=0xbfd0e404, pc=0x9628dfd "�", endpc=0x9628e0d "2\003�0\001b\be\r", hole=0xbfd0dee8) at ../jsopcode.cpp:1418
#2  0x080de1e3 in DecompileDestructuring (ss=0xbfd0e404, pc=0x9628dfd "�", endpc=0x9628e0d "2\003�0\001b\be\r") at ../jsopcode.cpp:1578
#3  0x080d4e4c in Decompile (ss=0xbfd0e404, pc=0x9628dfa "\f>7�", nb=21, nextop=JSOP_NOP) at ../jsopcode.cpp:3401
#4  0x080db793 in DecompileCode (jp=0x9628d58, script=0x9628db8, pc=0x9628df8 "�\003\f>7�", len=21, pcdepth=0) at ../jsopcode.cpp:4758
#5  0x080cf365 in js_DecompileFunction (jp=0x9628d58) at ../jsopcode.cpp:4927
#6  0x08058ac6 in JS_DecompileFunction (cx=0x9620d88, fun=0x96297a8, indent=0) at ../jsapi.cpp:4981
#7  0x080a225f in fun_toStringHelper (cx=0x9620d88, indent=0, argc=0, vp=0x962a02c) at ../jsfun.cpp:1580
#8  0x080a22e0 in fun_toString (cx=0x9620d88, argc=0, vp=0x962a02c) at ../jsfun.cpp:1590
#9  0x080af058 in js_Invoke (cx=0x9620d88, argc=0, vp=0x962a02c, flags=0) at ../jsinterp.cpp:1191
#10 0x080afccf in js_InternalInvoke (cx=0x9620d88, obj=0x96297a8, fval=157438120, flags=0, argc=0, argv=0x0, rval=0xbfd0e760) at ../jsinterp.cpp:1388
#11 0x080bb9a7 in js_TryMethod (cx=0x9620d88, obj=0x96297a8, atom=0x9621264, argc=0, argv=0x0, rval=0xbfd0e760) at ../jsobj.cpp:5443
#12 0x080bc9e4 in js_DefaultValue (cx=0x9620d88, obj=0x96297a8, hint=JSTYPE_STRING, vp=0xbfd0e7a4) at ../jsobj.cpp:4656
#13 0x0811299f in js_ValueToString (cx=0x9620d88, v=157456296) at ../jsstr.cpp:3098
#14 0x080699a8 in JS_ValueToString (cx=0x9620d88, v=157456296) at ../jsapi.cpp:557
#15 0x080509a4 in Print (cx=0x9620d88, argc=1, vp=0x962a018) at ../../shell/js.cpp:991
#16 0x081c675f in js_Interpret (cx=0x9620d88) at ../jsinterp.cpp:5011
#17 0x080ae929 in js_Execute (cx=0x9620d88, chain=0x9624000, script=0x9628e60, down=0x0, flags=0, result=0xbfd0f0d4) at ../jsinterp.cpp:1561
#18 0x08058863 in JS_ExecuteScript (cx=0x9620d88, obj=0x9624000, script=0x9628e60, rval=0xbfd0f0d4) at ../jsapi.cpp:5015
#19 0x08051b26 in Process (cx=0x9620d88, obj=0x9624000, filename=0x0, forceTTY=0) at ../../shell/js.cpp:481
#20 0x08052330 in ProcessArgs (cx=0x9620d88, obj=0x9624000, argv=0xbfd0f248, argc=0) at ../../shell/js.cpp:782
#21 0x08052632 in main (argc=0, argv=0xbfd0f248, envp=0xbfd0f24c) at ../../shell/js.cpp:4634
(gdb)

Comment 3

[Brendan Eich [:brendan]]

There must have been a regressing patch for another bug, possibly TM: but not necessarily. Anyone able to bisect?

/be

Comment 4

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

(In reply to comment #3)
> There must have been a regressing patch for another bug, possibly TM: but not
> necessarily. Anyone able to bisect?
> 
> /be

I've got a window from TM changeset 15157 to TM changeset 18797 (turns out these were from the days of the |make| compile method instead of autoconf213). Will keep bisecting till I get the final regression window.

Comment 5

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

The first bad revision is:
changeset:   18698:774fe6f69796
user:        Igor Bukanov
date:        Tue Sep 02 08:25:15 2008 +0200
summary:     bug 449494 - uniform handling of bytecodes with variable stack uses. r=mrbkap,brendan

hg bisect shows that this is a regression of bug 449494.

Comment 6

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

This bug is a regression of (i.e. blocks) bug 449494, fixing wrong direction.

Comment 7

[Brendan Eich [:brendan]]

Igor, could you field this one? I'll take any regression from a patch of mine on your list in trade, or not -- your call. Thanks,

/be

Comment 8

[Igor Bukanov]

I will check this.

Comment 9

[Igor Bukanov]

The bugs comes from the following line in Decompile, http://hg.mozilla.org/tracemonkey/file/0b36bddcefe4/js/src/jsopcode.cpp#l1914 :

1914    if (nb < 0 && -(nb + 1) == (intN)ss->top - cs->nuses + cs->ndefs)
1915        return pc;

Prior the bug 449494 cs->ndefs for JSOP_ENTERBLOCK was 0. My patch there changed that to -1 to indicate that JSOP_ENTERBLOCK pushes variable number of stack slots. This exposed a hidden bug in the above code that missed the fact that cs can refer to opcode with variable number of push and pop stack slots.

Now, the above code have a hidden bug in any case - it does not take into account that nuses and ndefs can denote variable

Comment 10

[Igor Bukanov]

(In reply to comment #9)
> 
> 1914    if (nb < 0 && -(nb + 1) == (intN)ss->top - cs->nuses + cs->ndefs)
> 1915        return pc;
...
> Now, the above code have a hidden bug in any case - it does not take into
> account that nuses and ndefs can denote variable

Yet another potential issue is that the current op can have a hidden note that is not supposed to contribute to the stack depth but which can match the condition by an accident.

Comment 11

[Igor Bukanov]

Attachment: v1

The patch hides most of the access to cs->(nuses|ndefs) behind 2 new inlines, GetStackDefs and GetStackUses that calculate the proper values for opcodes with variable stack usage.

The patch does not address that another potential issue from the comment 10. I guess I will file it as a separated bug.

Comment 12

[Brendan Eich [:brendan]]

Comment on attachment 369494 [details] [diff] [review]
v1

jorendorff pointed out that static JS_INLINE functions such as GetStackDefs and GetStackUses should have the js_ prefix, in another bug.

r=me with that if it makes sense to you.

/be

Comment 13

[Igor Bukanov]

Attachment: v2

The new version of the patch adds js_ prefix to inline functions.

Comment 14

[Igor Bukanov]

Attachment: v2 for real

The previous attachment has a wrong patch

Comment 15

[Igor Bukanov]

landed to TM - http://hg.mozilla.org/tracemonkey/rev/4ccfc2b54606

Comment 16

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/4ccfc2b54606

Comment 17

[Robert Sayre]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/9e1b151d7e61

Comment 18

[Bob Clary [:bc] (inactive)]

js1_8/regress/regress-453492.js
http://hg.mozilla.org/tracemonkey/rev/4c14dc59b518

Comment 19

[Bob Clary [:bc] (inactive)]

v 1.9.1, 1.9.2

Comment 20

[Bob Clary [:bc] (inactive)]

/cvsroot/mozilla/js/tests/js1_8/regress/regress-453492.js,v  <--  regress-453492.js
initial revision: 1.1