Last Comment Bug 364017 - Assertion failure: map->vector && i < map->length, at jsatom.c:919
: Assertion failure: map->vector && i < map->length, at jsatom.c:919
Status: VERIFIED FIXED
: verified1.8.1.2
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: x86 Linux
: -- critical (vote)
: ---
Assigned To: Igor Bukanov
:
: Jason Orendorff [:jorendorff]
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2006-12-15 18:18 PST by Jeff Walden [:Waldo] (remove +bmo to email)
Modified: 2007-01-29 10:01 PST (History)
0 users
bob: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Fix for jsshell (2.78 KB, patch)
2006-12-19 17:19 PST, Igor Bukanov
no flags Details | Diff | Splinter Review
Fix for jsshell v2 (4.25 KB, patch)
2006-12-20 03:08 PST, Igor Bukanov
no flags Details | Diff | Splinter Review
Fix for jsshell v3 (4.18 KB, patch)
2006-12-20 07:27 PST, Igor Bukanov
brendan: review+
Details | Diff | Splinter Review

Description Jeff Walden [:Waldo] (remove +bmo to email) 2006-12-15 18:18:30 PST
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 2006-12-15 18:28:25 PST
Most likely this is a regression from bug 331966.
Comment 2 Igor Bukanov 2006-12-15 18:35:36 PST
(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] (remove +bmo to email) 2006-12-15 18:56:37 PST
(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 2006-12-19 17:19:44 PST
Created attachment 249185 [details] [diff] [review]
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] 2006-12-19 18:04:57 PST
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 2006-12-20 03:08:52 PST
Created attachment 249240 [details] [diff] [review]
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 2006-12-20 07:27:10 PST
Created attachment 249258 [details] [diff] [review]
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 2006-12-20 09:26:39 PST
(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] 2006-12-20 12:02:25 PST
(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] 2006-12-20 14:00:09 PST
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 2006-12-20 17:49:53 PST
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 2006-12-20 17:52:45 PST
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] 2006-12-20 17:59:35 PST
(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 2006-12-20 18:02:49 PST
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 2006-12-20 18:25:34 PST
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:] 2007-01-18 02:42:37 PST
/cvsroot/mozilla/js/tests/e4x/Regress/regress-364017.js,v  <--  regress-364017.js
initial revision: 1.1
Comment 17 Bob Clary [:bc:] 2007-01-29 10:01:30 PST
verified fixed 1.8.1, 1.9.0 2007-01-23 win/mac*/linux

Note You need to log in before you can comment on or make changes to this bug.