Incorrect decompilation with array comprehension in catchguard

VERIFIED FIXED in mozilla1.9alpha5

Status

()

Core
JavaScript Engine
P1
critical
VERIFIED FIXED
11 years ago
11 years ago

People

(Reporter: Jesse Ruderman, 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

11 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

11 years ago
Keywords: regression
(Reporter)

Updated

11 years ago
Severity: normal → critical
(Assignee)

Comment 1

11 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

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

Comment 2

11 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

11 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

11 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

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

Comment 5

11 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: 11 years ago
Resolution: --- → FIXED
(Assignee)

Comment 6

11 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+
/cvsroot/mozilla/js/tests/js1_7/decompilation/regress-375794.js,v  <--  regress-375794.js
initial revision: 1.1
Flags: in-testsuite+
(Reporter)

Updated

11 years ago
Depends on: 379483
verified fixed 1.9.0 linux/mac* 2007-05-05 shell.
Status: RESOLVED → VERIFIED
(Reporter)

Updated

11 years ago
No longer blocks: 349611
(Reporter)

Updated

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