Closed
Bug 421806
Opened 17 years ago
Closed 17 years ago
"Assertion failure: *pcstack[pcdepth - 1] == JSOP_ENTERBLOCK"
Categories
(Core :: JavaScript Engine, defect, P1)
Core
JavaScript Engine
Tracking
()
VERIFIED
FIXED
mozilla1.9beta5
People
(Reporter: jruderman, Assigned: igor)
References
Details
(Keywords: assertion, regression, testcase)
Attachments
(1 file, 2 obsolete files)
23.80 KB,
patch
|
igor
:
review+
|
Details | Diff | Splinter Review |
try { let([] = c) 1; } catch(e) { this.a.b; }
Expected:
TypeError: this.a is undefined
Result in opt:
TypeError: (void 0) is undefined
Result in debug:
Assertion failure: *pcstack[pcdepth - 1] == JSOP_ENTERBLOCK, at jsopcode.c:5150
This LOCAL_ASSERT is part of ReconstructPCStack, which was added in bug 420399.
Assignee | ||
Updated•17 years ago
|
Assignee: general → igor
Assignee | ||
Comment 1•17 years ago
|
||
> Assertion failure: *pcstack[pcdepth - 1] == JSOP_ENTERBLOCK, at
This assertion is bogus. let([] = ...) generates let blocks with zero locals. Thus in this case the top-most pc on pcstack points to the pc that generated the let expression (JSOP_ONE).
Assignee | ||
Comment 2•17 years ago
|
||
After looking at this I have found another bad bug in the code: DecompileCode still access the stack of the last interpreted frame to find the pc of the missing operands.
Assignee | ||
Comment 3•17 years ago
|
||
The patch fixes the assert and changes the decompiler logic to use the modeled pcstack, not something below fp->spbase. Another fix is for the generator to allocate just depth slots for the stack, not depth * 2.
While looking into this I was wondering why the decompiler has to recurs into DecompileValueGenerator in GetOff. AFAICS if the decompiler starts from the expression statement start, it should have all the necessary slots computed by the time it reaches pc, shouldn't it?
Attachment #308334 -
Flags: review?(brendan)
Comment 4•17 years ago
|
||
This should block.
/be
Flags: blocking1.9?
OS: Mac OS X → All
Hardware: PC → All
Target Milestone: --- → mozilla1.9beta5
Comment 5•17 years ago
|
||
(In reply to comment #3)
> While looking into this I was wondering why the decompiler has to recurs into
> DecompileValueGenerator in GetOff. AFAICS if the decompiler starts from the
> expression statement start, it should have all the necessary slots computed by
> the time it reaches pc, shouldn't it?
See bug 346642. One example:
with({x: (new (b = 1))}) (2).x
/be
Comment 6•17 years ago
|
||
Comment on attachment 308334 [details] [diff] [review]
v1
>+#define FAILED_EXPRESSION_DECOMPILER ((char *) 1)
Safe enough if some caller fails to test for this, a nearly null crash. A static char FAILURE_COOKIE[] = "..." would be even safer and maybe prettier.
>+/*
>+ * Decompile a part of expression up to the given pc. The function returns
>+ * NULL on out-of-memory, FAILED_EXPRESSION_DECOMPILER sentinel when the
"or" after the comma above.
>+ * decompiler fails due to a bug and/or unimplemented feature or the
Doubled space before "or the" should be ", ".
>+ * decompiled string.
Suggest "on success" to finish this sentence in context.
>+ name = DecompileExpression(cx, script, fp->fun, pc);
>+ if (name == FAILED_EXPRESSION_DECOMPILER)
>+ goto do_fallback;
>+ return name;
Could invert the FAIL... test and return name in the consequent, avoid a goto.
Patch seems good, I tried some torture-tests from Jesse's comments in bug 346642. r=me if it tests well in the suite, which IIRC has regression tests for all fuzz-found decompiler bugs. Bob, please deny if I'm wrong ;-). In that event we need Jesse to re-fuzz.
/be
Attachment #308334 -
Flags: review?(brendan) → review+
Comment on attachment 308334 [details] [diff] [review]
v1
"modelled" => "modeled"
> The decompiler wants to see on pcstack [leaveblockexpr], not
[leaveblockexpr] on the pcstack
> [enterblock] or the pc that ended the a simulated let
the and a are both articles, pick precisely one
> expression when [enterblock] defines zero locals like in
"like in" => "as in:"
Assignee | ||
Comment 8•17 years ago
|
||
(In reply to comment #5)
> See bug 346642. One example:
>
> with({x: (new (b = 1))}) (2).x
I have not find a particular explanation for this in this bug. Why decompiling {x: (new (b = 1))} inside with is problematic while decompiling {x: (new (b = 1))} is not? Or is the real problem is decompiling of new (b = 1) ?
Assignee | ||
Comment 9•17 years ago
|
||
(In reply to comment #6)
> Safe enough if some caller fails to test for this, a nearly null crash. A
> static char FAILURE_COOKIE[] = "..." would be even safer and maybe prettier.
DecompileExpression returns a freshly-allocated string. As such returning FAILURE_COOKIE does not add robustness if a caller forget to check for the cookie. It will crash when trying to free(FAILURE_COOKIE). Or is the suggestion to simply return JS_strdup(cx, "/* DECOMPILER BUG */") on errrors?
Assignee | ||
Comment 10•17 years ago
|
||
(In reply to comment #9)
> Or is the suggestion to simply return JS_strdup(cx, "/* DECOMPILER BUG */") on errrors?
That will remove the need to check in callers for FAILED_EXPRESSION_DECOMPILER or FAILURE_COOKIE. On the other hand it will prevent a fallback based on uneval of a vlaue form the stack slot that caused the error for JSOP_BINDNAME:
/*
* JSOP_BINDNAME is special: it generates a value, the base object of a
* reference. But if it is the generating op for a diagnostic produced by
* js_DecompileValueGenerator, the name being bound is irrelevant. Just
* fall back to the base object.
*/
if (op == JSOP_BINDNAME)
return FAILED_EXPRESSION_DECOMPILER;
Assignee | ||
Comment 11•17 years ago
|
||
The new version just addresses the nits without touching FAILED_EXPRESSION_DECOMPILER:
@@ -786,5 +786,5 @@ ReconstructPCStack(JSContext *cx, JSScri
* Decompile a part of expression up to the given pc. The function returns
- * NULL on out-of-memory, FAILED_EXPRESSION_DECOMPILER sentinel when the
- * decompiler fails due to a bug and/or unimplemented feature or the
- * decompiled string.
+ * NULL on out-of-memory, or FAILED_EXPRESSION_DECOMPILER sentinel when the
+ * decompiler fails due to a bug and/or unimplemented feature, or the
+ * decompiled string on success.
*/
@@ -4864,5 +4864,4 @@ js_DecompileValueGenerator(JSContext *cx
name = DecompileExpression(cx, script, fp->fun, pc);
- if (name == FAILED_EXPRESSION_DECOMPILER)
- goto do_fallback;
- return name;
+ if (name != FAILED_EXPRESSION_DECOMPILER)
+ return name;
@@ -5164,5 +5163,5 @@ ReconstructPCStack(JSContext *cx, JSScri
/*
- * The decompiler wants to see on pcstack [leaveblockexpr], not
- * [enterblock] or the pc that ended the a simulated let
- * expression when [enterblock] defines zero locals like in
+ * The decompiler wants to see [leaveblockexpr] on pcstack, not
+ * [enterblock] or the pc that ended a simulated let expression
+ * when [enterblock] defines zero locals as in:
*
Attachment #308334 -
Attachment is obsolete: true
Attachment #308821 -
Flags: review?(brendan)
Comment 12•17 years ago
|
||
Comment on attachment 308821 [details] [diff] [review]
v1b
>+ * Decompile a part of expression up to the given pc. The function returns
>+ * NULL on out-of-memory, or FAILED_EXPRESSION_DECOMPILER sentinel when the
One last nit: "the" before FAILED_EXPRESSION_DECOMPILER and rewrap (should look less ragged right to boot!).
/be
Attachment #308821 -
Flags: review?(brendan) → review+
Updated•17 years ago
|
Flags: blocking1.9? → blocking1.9+
Priority: -- → P1
Reporter | ||
Comment 13•17 years ago
|
||
Patch v1 holds up fine to 10 hours of jsfunfuzz. I even added something to jsfunfuzz in an attempt to find more "incorrect error message" DVG bugs like the one in comment 0, but only ended up finding bug 422501 and bug 422504.
Assignee | ||
Comment 14•17 years ago
|
||
The new version addresses the nit from the comment 12:
#define FAILED_EXPRESSION_DECOMPILER ((char *) 1)
/*
* Decompile a part of expression up to the given pc. The function returns
- * NULL on out-of-memory, or FAILED_EXPRESSION_DECOMPILER sentinel when the
- * decompiler fails due to a bug and/or unimplemented feature, or the
+ * NULL on out-of-memory, or the FAILED_EXPRESSION_DECOMPILER sentinel when
+ * the decompiler fails due to a bug and/or unimplemented feature, or the
* decompiled string on success.
*/
Attachment #308821 -
Attachment is obsolete: true
Attachment #308978 -
Flags: review+
Assignee | ||
Comment 15•17 years ago
|
||
I checked in the patch from the comment 14 to the trunk:
http://bonsai.mozilla.org/cvsquery.cgi?module=PhoenixTinderbox&branch=HEAD&cvsroot=%2Fcvsroot&date=explicit&mindate=1205362741&maxdate=1205363029&who=igor%25mir2.org
Status: NEW → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
Comment 16•17 years ago
|
||
you ignored this:
"modelled" => "modeled"
:(
Comment 17•17 years ago
|
||
verified linux|mac|windows
/cvsroot/mozilla/js/tests/js1_7/expressions/regress-421806.js,v <-- regress-421806.js
initial revision: 1.1
Status: RESOLVED → VERIFIED
Flags: in-testsuite+
Flags: in-litmus-
You need to log in
before you can comment on or make changes to this bug.
Description
•