Incorrect decompilation with array comprehension in catchguard

VERIFIED FIXED in mozilla1.9alpha5

Status

()

P1
critical
VERIFIED FIXED
12 years ago
12 years ago

People

(Reporter: jruderman, Assigned: brendan)

Tracking

(Blocks: 1 bug, {regression, testcase})

Trunk
mozilla1.9alpha5
regression, testcase
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

12 years ago
"c" becomes "[]":

js> function() { try { } catch(a if [b for each (c in [])]) { } } 
function () {
    try {
    } catch (a if [b for each ([] in [])]) {
    }
}

I think this also causes
Assertion failure: (uintN)i < ss->top, at jsopcode.c:2483
(Reporter)

Updated

12 years ago
Keywords: regression
(Reporter)

Updated

12 years ago
Severity: normal → critical
(Assignee)

Comment 1

12 years ago
Did the fix for bug 351102 cause this bug? In any event, the decompiler stack is not being managed to include the necessary slot for the exception.

/be
Assignee: general → brendan
(Assignee)

Updated

12 years ago
Status: NEW → ASSIGNED
OS: Mac OS X → All
Priority: -- → P1
Hardware: PC → All
Target Milestone: --- → mozilla1.9alpha5
(Assignee)

Comment 2

12 years ago
Created attachment 263378 [details] [diff] [review]
fix

Should be self-explanatory. Variation on the case shows the need for conditioning in JSOP_LEAVEBLOCK:

function() { try { } catch(a if [b for each (c in [])]) { print(a) } catch (e) { print(e) }}

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

Comment 3

12 years ago
Comment on attachment 263378 [details] [diff] [review]
fix

>Index: jsopcode.c
>===================================================================
>RCS file: /cvsroot/mozilla/js/src/jsopcode.c,v
>retrieving revision 3.228
>diff -p -u -8 -r3.228 jsopcode.c
>--- jsopcode.c	1 May 2007 18:07:44 -0000	3.228
>+++ jsopcode.c	1 May 2007 19:08:09 -0000
>@@ -2426,44 +2426,57 @@ Decompile(SprintStack *ss, jsbytecode *p
>                   case SRC_CATCH:
...
>                     if (*pc == JSOP_DUP) {
>                         sn2 = js_GetSrcNote(jp->script, pc);
>                         if (sn2 && SN_TYPE(sn2) == SRC_HIDDEN) {
>                             /*
>                              * This is a hidden dup to save the exception for
>-                             * later. It must exist only when the catch has
>-                             * an exception guard.
>+                             * later. It is emitted only when the catch has an
>+                             * exception guard.
>                              */

As I understand, the decompiler has to model the stack precisely as the interpreter does for asserts and stack calculations based on jsopcode.tbl to work. So this dup is not hidden, but rather very visible! Thus I suggest to remove the hidden node from the guarded catch dup and rather annotate the following one with SRC_DESTRUCT.

Otherwise it seems OK.
(Assignee)

Comment 4

12 years ago
Created attachment 263414 [details] [diff] [review]
fix, v2

Good point, thanks.

/be
Attachment #263378 - Attachment is obsolete: true
Attachment #263414 - Flags: review?(igor)
Attachment #263378 - Flags: review?(igor)

Updated

12 years ago
Attachment #263414 - Flags: review?(igor) → review+
(Assignee)

Comment 5

12 years ago
Fixed on trunk:

js/src/jsemit.c 3.246
js/src/jsopcode.c 3.229

/be
Blocks: 355044
Status: ASSIGNED → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED
(Assignee)

Comment 6

12 years ago
Created attachment 263441 [details] [diff] [review]
patch I committed

Fixed a recent unpicked whitespace nit in jsopcode.c.

/be
Attachment #263414 - Attachment is obsolete: true
Attachment #263441 - Flags: review+

Comment 7

12 years ago
/cvsroot/mozilla/js/tests/js1_7/decompilation/regress-375794.js,v  <--  regress-375794.js
initial revision: 1.1
Flags: in-testsuite+
(Reporter)

Updated

12 years ago
Depends on: 379483

Comment 8

12 years ago
verified fixed 1.9.0 linux/mac* 2007-05-05 shell.
Status: RESOLVED → VERIFIED

Updated

12 years ago
Depends on: 379860
(Reporter)

Updated

12 years ago
No longer blocks: 349611
(Reporter)

Updated

12 years ago
Blocks: 349611
You need to log in before you can comment on or make changes to this bug.