316862 - 'Disassemble to bytecode' is a bit broken

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P3)

Description

[Aiko]

In particular, the uint24 opcode can't be disassembled. Example shell session:

js> function f() { return 5 << 16 }
js> f
function f() {
    return 327680;
}
js> dis(f)
main:
00000:  uint24
Source notes:
js>

This is with the JS debug shell, cvs updated an hour ago. Maybe this is a side effect of bug 155081?

Comment 1

[Brendan Eich [:brendan]]

DEBUG-only patch coming up, good for JS1.6 RC1.

/be

Comment 2

[Brendan Eich [:brendan]]

Attachment: fix

I think shaver buddied the UINT24/LITOPX patch.  This should land on the trunk soon, as it is not part of the build.

/be

Comment 3

[Brendan Eich [:brendan]]

Attachment: fix, v2

Comments and a fix to disassemble JSOP_DEFLOCALFUN's 2nd immediate.

/be

Comment 4

[Brendan Eich [:brendan]]

Noting dependency because any testcase that uses the shell's dis function will want bug 321971 to be fixed.

Bob, I'll stick a testcase sketch.

/be

Comment 5

[Brendan Eich [:brendan]]

Attachment: dis test with > 64K identifier generated script

Comment 6

[Brendan Eich [:brendan]]

Attachment: fix, v3 + patch from bug 321971

This is an all-in-one patch, if you already have the fix for bug 321971, just cut out the jsemit.c part.

/be

Comment 7

[Brendan Eich [:brendan]]

This bug's patch is a DEBUG-only change, not counting the patch for bug 321971 to jsemit.c that's included in the latest attachment, so it's safe for branches.

/be

Comment 8

[Brendan Eich [:brendan]]

Attachment: fix, v3 (no other patches mixed in)

Ignore that last patch, I'm obsoleting it.  Better for review and branch landings keep this patch addressing this bug only.  Caveat testers: get the blocking bug's patch and apply it first.

/be

Comment 9

[Mike Shaver (:shaver emeritus)]

Comment on attachment 207245 [details] [diff] [review]
fix, v3 (no other patches mixed in)

>+      case JOF_UINT24:
>+        if (op == JSOP_FINDNAME) {
>+            atom = js_GetAtom(cx, &script->atomMap, GET_LITERAL_INDEX(pc));
>+            str = js_ValueToSource(cx, ATOM_KEY(atom));
>+            if (!str)
>+                return 0;
>+            fprintf(fp, " %s", JS_GetStringBytes(str));
>+        } else {
>+            fprintf(fp, " %u", GET_LITERAL_INDEX(pc));
>+        }
>+        break;

Test !JSOP_FINDNAME and break from the then-block, to 
de-indent the bulk of this clause, and avoid fat-then-hanging-over-thin-else?

Also, please assert that you're getting JSOP_LITERAL or 
JSOP_UINT24 in the not-FINDNAME case, to make it easier
on whoever adds the next JOF_UINT24 bytecode.

r=shaver with such changes.

Comment 10

[Brendan Eich [:brendan]]

(In reply to comment #9)

> Test !JSOP_FINDNAME and break from the then-block, to 
> de-indent the bulk of this clause, and avoid fat-then-hanging-over-thin-else?

I tested the other sense, for clarity of test and to cast out the special case (I commented how this is a special case to avoid burning a format for JOF_FINDNAME).

> Also, please assert that you're getting JSOP_LITERAL or 
> JSOP_UINT24 in the not-FINDNAME case, to make it easier
> on whoever adds the next JOF_UINT24 bytecode.

Done and done -- committed.  Thanks!

/be

Comment 11

[Brendan Eich [:brendan]]

Attachment: updated per shaver's comments as checked into trunk, for branch

DEBUG-only, not part of built products, wanted for SpiderMonkey 1.6 release.

/be

Comment 12

[Mike Shaver (:shaver emeritus)]

Comment on attachment 207472 [details] [diff] [review]
updated per shaver's comments as checked into trunk, for branch

Are we really going to release SM1.6 off the 1.8.0.1 branch?  I would think it'd not be worth the branch noise, and that we'd just go for 1.8.1 landings.

Comment 13

[Brendan Eich [:brendan]]

(In reply to comment #12)
> (From update of attachment 207472 [details] [diff] [review] [edit])
> Are we really going to release SM1.6 off the 1.8.0.1 branch?  I would think
> it'd not be worth the branch noise, and that we'd just go for 1.8.1 landings.

We want 1.8.0.1 to match JS1.6, I think.  You're right that this DEBUG-only patch could land only in a JS1.6 mini-branch, but then we have to think more than two seconds about whether everything in 1.8.0.1 is in JS1.6.  Why allow doubt here?

/be

Comment 14

[Brendan Eich [:brendan]]

Comment on attachment 207472 [details] [diff] [review]
updated per shaver's comments as checked into trunk, for branch

Ah, shaver's right.  If we end up needing this, it can go in with implicit a+ for NPOB.

/be

Comment 15

[Brendan Eich [:brendan]]

Checked into the branches.

/be

Comment 16

[Bob Clary [:bc] (inactive)]

v 20060113 1.8.0.1, 1.8.1, 1.9a1 windows/linux/mac with the addition on the trunk of a stop statement after the return.

Note that occasionally I saw f.toString return |function f() { return 5 << 16; }| instead of |function f() { return 327680; }|. Happened mostly in 1.8 opt builds?

note to self: make dis return a string for automatic comparison.