364017 - Assertion failure: map->vector && i < map->length, at jsatom.c:919

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jeff Walden [:Waldo]]

js> dis(function() {
  XML.prototype.function::toString = function() { return "foo"; };
})
flags: LAMBDA INTERPRETED
main:
00000:  name "XML"
00003:  getprop "prototype"
00006:  anonfunobj (function () {return "foo";})
00009:  setmethod "toString"
00012:  pop
00013:  stop

Source notes:
  0:     0 [   0] newline
  1:     3 [   3] pcbase   offset 3
Assertion failure: map->vector && i < map->length, at jsatom.c:919

Comment 1

[Igor Bukanov]

Most likely this is a regression from bug 331966.

Comment 2

[Igor Bukanov]

(In reply to comment #1)
> Most likely this is a regression from bug 331966.

It is not, but still I will look into it.

Comment 3

[Jeff Walden [:Waldo]]

(In reply to comment #2)
> It is not, but still I will look into it.

Bug 355258 comment 5 may be of interest here, then.

Comment 4

[Igor Bukanov]

Attachment: Fix for jsshell

The patch updates the source note printout code in jsshell to distinguish LABEL from METHODBASE based on the bytecode the code points to.

Comment 5

[Brendan Eich [:brendan]]

Comment on attachment 249185 [details] [diff] [review]
Fix for jsshell

>+/*
>+ * SrcNotes assumes that SRC_METHODBASE should be distinguished from SRC_LABEL
>+ * using the bytecode the source note points to.
>+ */
>+JS_STATIC_ASSERT(SRC_LABEL == SRC_METHODBASE);
>+

In the rush for Firefox 2 I failed to catch this.  I had a plan to extend js_SrcNoteSpec with lists of bytecodes (initialized static arrays, or perhaps C strings) for which the aliases were valid.  Comments?

> static void
> SrcNotes(JSContext *cx, JSScript *script)
> {
>+    const char NAME_PATTERN[] = "%-8s";

I'm superstitious about old C compilers, so if you could make this static, move it down to bottom of declarations, and use FORMAT or FMT instead of PATTERN, I'd be grateful.

>     uintN offset, delta, caseOff;
>     jssrcnote *notes, *sn;
>     JSSrcNoteType type;
>+    uintN op;

Why not JSOp?  Helps in gdb.

>+        if (type == SRC_LABEL) {
>+            op = script->code[offset];
>+            if (op == JSOP_GETMETHOD || op == JSOP_SETMETHOD) {
>+                /* This is SRC_METHODBASE. */
>+                fprintf(gOutFile, NAME_PATTERN, "methodbase");
>+                goto print_offset;
>+            }
>+            JS_ASSERT(op == JSOP_NOP ||
>+                      op == JSOP_TABLESWITCH ||
>+                      op == JSOP_TABLESWITCHX ||
>+                      op == JSOP_LOOKUPSWITCH ||
>+                      op == JSOP_LOOKUPSWITCHX);

The SRC_LABEL notes emitted for the switch opcodes do not apply to JSOP_*SWITCH*, rather to case expressions that have had constants propagated to replace names:

js> eval("const A=1, B=2, C=3; function f(x){switch(x){case A:return 1;case B:return 2;case C:return 3}}")
js> f
function f(x) {
    switch (x) {
      case A:
        return 1;
      case B:
        return 2;
      case C:
        return 3;
      default:;
    }
}
js> dis(f)
main:
00000:  getarg 0
00003:  tableswitch defaultOffset 23 low 1 high 3
        1: 13
        2: 15
        3: 19
00016:  one
00017:  return
00018:  uint16 2
00021:  return
00022:  uint16 3
00025:  return
00026:  stop

Source notes:
  0:     3 [   3] switch   length 23
  3:     9 [   6] label    atom 0 (A)
  5:    11 [   2] label    atom 1 (B)
  7:    13 [   2] label    atom 2 (C)

In this example, which requires eval in order for the optimization to be considered safe, the A SRC_LABEL applies to the constant 1 whose two-byte jump offset is at bytecode 10; to address this using the standard decoding, subtract -1 to get 9. Etc. for 2/12 and 3/14.  See the JSOP_TABLESWITCH* and JSOP_LOOKUPSWITCH* cases in Decompile, note the pc2 variable passed to js_GetSrcNote.

So the upshot is that you can't load script->code[offset] and test for a known bytecode -- you need to remember that you're in a switch of the right kind.  Yuck.

/be

Comment 6

[Igor Bukanov]

Attachment: Fix for jsshell v2

This version detects the switch and reads script->code[offset] only when the label note is outside the switch tables.

I also changed the code to print "case", not "label" for the switch case notes.

Comment 7

[Igor Bukanov]

Attachment: Fix for jsshell v3

In this version I removed +1/-1 game in GetSwitchTableBounds which was a leftover from the inirial prototype.

Comment 8

[Igor Bukanov]

(In reply to comment #5)
> I had a plan to extend js_SrcNoteSpec with lists of bytecodes (initialized static arrays, or perhaps C strings) for which the aliases were valid.  Comments?

But why aliases are neccessery? The total number of all source notes is 28 AFAICS which is less then 1<<5. 
 

Comment 9

[Brendan Eich [:brendan]]

(In reply to comment #8)
> But why aliases are neccessery? The total number of all source notes is 28
> AFAICS which is less then 1<<5. 

We would like more than 28 source note *names*, in order to make decompiler code intelligible given the new JS1.7 features.  But as you note we do not need more than 28 distinct codes.  So the trick is to decompile the appropriate name in context of the bytecode it annotates.

/be

Comment 10

[Brendan Eich [:brendan]]

Comment on attachment 249258 [details] [diff] [review]
Fix for jsshell v3

Looks great.

I should take a followup to make getting the name part of the jsemit.c machinery, instead of a shell helper.

/be

Comment 11

[Igor Bukanov]

I committed the patch from comment 7 to the trunk:

Checking in js.c;
/cvsroot/mozilla/js/src/js.c,v  <--  js.c
new revision: 3.128; previous revision: 3.127
done

Comment 12

[Igor Bukanov]

Comment on attachment 249258 [details] [diff] [review]
Fix for jsshell v3

Is the approval is necessary for 1.8.1 branch given that the file is not a part of browser's build?

Comment 13

[Brendan Eich [:brendan]]

(In reply to comment #12)
> (From update of attachment 249258 [details] [diff] [review] [edit])
> Is the approval is necessary for 1.8.1 branch given that the file is not a part
> of browser's build?

Not in my book.

/be

Comment 14

[Igor Bukanov]

Comment on attachment 249258 [details] [diff] [review]
Fix for jsshell v3

Given the previous comment the approval is not necessary for 1.8.1

Comment 15

[Igor Bukanov]

I committed the patch from comment 7 to MOZILLA_1_8_BRANCH: 

Checking in js.c;
/cvsroot/mozilla/js/src/js.c,v  <--  js.c
new revision: 3.93.2.14; previous revision: 3.93.2.13
done

Comment 16

[Bob Clary [:bc] (inactive)]

/cvsroot/mozilla/js/tests/e4x/Regress/regress-364017.js,v  <--  regress-364017.js
initial revision: 1.1

Comment 17

[Bob Clary [:bc] (inactive)]

verified fixed 1.8.1, 1.9.0 2007-01-23 win/mac*/linux