Closed Bug 410571 Opened 14 years ago Closed 14 years ago

Yield-expression disappears in decompilation of object literal

Categories

(Core :: JavaScript Engine, defect)

x86
macOS
defect
Not set
normal

Tracking

()

VERIFIED FIXED

People

(Reporter: jruderman, Assigned: Waldo)

References

Details

(Keywords: regression, testcase)

Attachments

(3 files, 2 obsolete files)

$ ./js -v 170
js> (function() { var t = { x: 3, y: yield 4 }; })
function () {
    var t = {x: 3, y:  };
}

This doesn't compile because the "yield 4" after "y:" is missing.
Blocks: js1.8
Regression from patch for bug 376957 -- I should have noticed the dependency on rval that cannot survive the split Sprint.

/be
Blocks: 376957
Flags: blocking1.9+
Waldo, can you take this one? I'll do a better job reviewing.

/be
No longer blocks: 376957
Blocks: 376957
Keywords: regression
Similar issue with |let|:

js> (function() { return { x: 1, y: let (z) z } })
function () {
    return {x: 1, y:  };
}

This bug is in the code that decompiles property initialisers, so it does not really depend on yield, let, etc. It depends on how strings popped from the decompiler's SprintStack get overwritten by result strings pushed in more than one Sprint call (no memory unsafety, just bad output).

/be
Assignee: nobody → jwalden+bmo
Attached patch JS_strdup and free (obsolete) — Splinter Review
This would be simpler (not necessary) if we just pushed a >=1-sized string in the place where the numerical index for array literals usually goes ("#" or "##" or similar protects the ", " from overwriting the start of |rval|), but I don't see a simple way to do that in a smaller-sized change without tweaking bytecode.
Attachment #295572 - Flags: review?(brendan)
Comment on attachment 295572 [details] [diff] [review]
JS_strdup and free

>                         const char *end = rval + strlen(rval);
> 
>                         if (*rval == '(')
>                             ++rval, --end;
>                         LOCAL_ASSERT(strncmp(rval, js_function_str, 8) == 0);
>                         LOCAL_ASSERT(rval[8] == ' ');
>                         rval += 8 + 1;
>                         LOCAL_ASSERT(*end ? *end == ')' : end[-1] == '}');
>                         if (Sprint(&ss->sprinter, "%s %s%s%.*s",
>                                    (lastop == JSOP_GETTER)
>                                    ? js_get_str : js_set_str,
>                                    xval,
>                                    (rval[0] != '(') ? " " : "",
>                                    end - rval, rval) < 0) {
>-                            return NULL;
>+                            goto initprop_error;

[snip]

>+              initprop_error:
>+                JS_free(cx, (char *) rval);

rval is not a pointer to a separate malloc block if the goto was taken.

Would prefer to go back to old code, avoid strdup/free, somehow just Sprint all at once. Too hard inline? Make a helper function to do it.

/be
Waldo: new patch coming?

/be
Yes, "soon-ish".
Attached patch Combine codepaths a bit (obsolete) — Splinter Review
1) Are you a fan of my code alignment in this patch?  I think I'm forging new ground, style-wise.  :-D
2) Are you a fan of non-toplevel-in-switch case statements?  We avoid a needless comparison this way, and we more or less have to do this to share the |isFirst| binding with the shared sprinting code.
Attachment #295572 - Attachment is obsolete: true
Attachment #301619 - Flags: review?(brendan)
Attachment #295572 - Flags: review?(brendan)
1) Are you a fan of my code alignment in this patch?  I think I'm forging new ground, style-wise.  :-D
2) Are you a fan of non-toplevel-in-switch case statements?  We avoid a needless comparison this way, and we more or less have to do this to share the |isFirst| binding with the shared sprinting code.
Attachment #301619 - Attachment is obsolete: true
Attachment #301620 - Flags: review?(brendan)
Attachment #301619 - Flags: review?(brendan)
Comment on attachment 301620 [details] [diff] [review]
Combine codepaths a bit

As a fan of Duff's Device I have no problem with putting case labels inside outer blocks, and in fact have done so in Decompile. But please don't overindent the labels or their code:

>               {
>                 JSBool isFirst;
> 
>+                case JSOP_INITELEM:

This should be indented to the same level as other cases.

>+                    /* Turn off most parens (all if there's only one initialiser). */

and this and subsequent lines should be four spaces to the left.

>+                    LOCAL_ASSERT(pc + len < endpc);
>+                    isFirst = (ss->opcodes[ss->top - 3] == JSOP_NEWINIT);
>+                    op = (isFirst &&
>+                          GetStr(ss, ss->top - 2)[0] == '0' &&
>+                          (JSOp) pc[len] == JSOP_ENDINIT)
>+                         ? JSOP_NOP
>+                         : JSOP_SETNAME;

The remaining isFirst uses all select "" or ", ", so you could common that into a const char *maybeComma block-local.

I mentally executed some of the control flow graph, then got tired. Do we have tests covering all paths? Did you flip the #ifdefs to test the old code? If it's hard to maintain we should consider getting rid of it instead of sort of kind of maintaining it.

r/a=me with nits and assuming good test coverage and results.

/be
Attachment #301620 - Flags: review?(brendan)
Attachment #301620 - Flags: review+
Attachment #301620 - Flags: approval1.9+
I couldn't find any problems in my testing, although it's a bit weird that that the rhs for an old-school getter (die!) can't be a let-expression.

Last question before committing and fixing: did I interpret the comments on indentation correctly?
Attachment #302292 - Flags: review?(brendan)
By the way, if old-school getters are to die (yes!  can we kill sharps too?  shaver will r+!), they shouldn't die in response to the non-existent difficulty of making this patch accommodate them.
Comment on attachment 302292 [details] [diff] [review]
Fix whitespace (?), maybeComma, add set of tests

Given the indentation (which is what I wanted, thanks for asking), why not put JSOP_INITELEM's case down where it was? More minimal diff that way, if I'm squinting right.

Let's get rid of old-style getters for js1.9 (after mozilla1.9).

/be
Attachment #302292 - Flags: review?(brendan) → review+
(In reply to comment #14)
> Given the indentation (which is what I wanted, thanks for asking), why not put
> JSOP_INITELEM's case down where it was? More minimal diff that way, if I'm
> squinting right.

Possibly; I don't think it helped for readability that way, tho.  It made it hard to compare INITELEM code vs. INITPROP, and it meant you had to scroll down, then back up to parse the shared part of INITELEM.

> Let's get rid of old-style getters for js1.9 (after mozilla1.9).

Whee!

In on trunk, fixed.
Status: NEW → RESOLVED
Closed: 14 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Jeff, I get the following error when running the test for this bug in the browser.

Error: syntax error
Source File: http://test.mozilla.com/tests/mozilla.org/js/js1_7/decompilation/regress-410571.js
Line: 153, Column: 9
Source Code:
function () { 

do you need to wrap the source in '()' before calling eval?
I don't think so; |function () { ... }| is a Program which is a Statement which is an ExpressionStatement.  For example:

javascript:eval("function%20()%20{\n}")

What's the contents of |dec| that are being eval'd there?
dec ~ anonymous function... |function () { ... }| See bug 376052. I changed it locally to wrap in () before the eval and it passes in the browser.
Waldo: ES3 doesn't produce an anonymous function expression as a Program.

/be
change to fail by compare rather than fail by uncaught exception and wrap source to be eval'd in '()'

Checking in regress-410571.js;
/cvsroot/mozilla/js/tests/js1_7/decompilation/regress-410571.js,v  <--  regress-410571.js
new revision: 1.2; previous revision: 1.1
v
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.