511575 - TM: Crash [@ js_HashString] with "continue LABEL;" (in ojay 0.2.1)

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Jesse Ruderman]

Attachment: reasonably simple shell testcase

Comment 1

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

(In reply to comment #0)
> Created an attachment (id=395495) [details]
> reasonably simple shell testcase

autoBisect shows this is probably related to bug 508051:

The first bad revision is:
changeset:   31476:d1171f8cc234
user:        Jason Orendorff
date:        Fri Aug 14 15:47:04 2009 -0500
summary:     Bug 508051 part 1 - Avoid imacros for JSOP_SETELEM. Re-landing per comment 8. r=gal.


Exception Type:  EXC_BAD_ACCESS (SIGSEGV)
Exception Codes: KERN_INVALID_ADDRESS at 0x000000002000000d
Crashed Thread:  0

Thread 0 Crashed:
0   js-opt-tm-darwin              	0x000c5eb0 js_HashString + 112
1   js-opt-tm-darwin              	0x00026e81 JS_DHashTableOperate + 33
2   js-opt-tm-darwin              	0x0001c0ba js_AtomizeString + 74
3   js-opt-tm-darwin              	0x000f10be SetPropertyByName(JSContext*, JSObject*, JSString**, long*) + 142
4   ???                           	0x001f0b14 0 + 2034452
5   ???                           	0xbfffcb98 0 + 3221212056
6   ???                           	0x001f0cf4 0 + 2034932
7   ???                           	0xbffff1f8 0 + 3221221880
8   js-opt-tm-darwin              	0x00107f4a js_MonitorLoopEdge(JSContext*, unsigned int&) + 2250
9   js-opt-tm-darwin              	0x00057fd7 js_Interpret + 44135
10  js-opt-tm-darwin              	0x0005ce22 js_Execute + 386
11  js-opt-tm-darwin              	0x0000eabc JS_ExecuteScript + 60
12  js-opt-tm-darwin              	0x00004d50 Process(JSContext*, JSObject*, char*, int) + 1616
13  js-opt-tm-darwin              	0x00007ecf main + 879
14  js-opt-tm-darwin              	0x0000244b _start + 209
15  js-opt-tm-darwin              	0x00002379 start + 41

Comment 2

[Jason Orendorff [:jorendorff]]

Reduced slightly:

  for (var i = 0; i < 9; i++) {
      (function() {
          outer:
          for (var j = 0; j < 1; j++)
              for (var p in {x:1})
                  if (p > "q")
                      continue outer;
          return 0;
       })();
  }

It's pretty sensitive: remove the return statement and it'll stop crashing.

The trace isn't unboxing p in this case, or something.

Comment 3

[Jason Orendorff [:jorendorff]]

Ah, after I fix a non-crashing bug in how we handle `continue LABEL;`, this reduces to:

  outer:
  for (var j = 0; j < 10; j++)
      for (var p in {x:1})
          if (p > "q")
              continue outer;

Still crashes. I'll file a separate bug for `continue LABEL;` since I have a patch that includes a separate test case.

Comment 4

[Jason Orendorff [:jorendorff]]

Attachment: v1

nanojit is throwing away stack writes before js_CallTree because it doesn't expect js_CallTree to access the stack!

The solution is to emit an xbarrier before js_CallTree.

(This bites us in this particular test case because when we hit `continue label`, the inner tree exits at a pc with sp two words *lower* than its entry point.  nanojit throws away stores in that two-word region of the stack.  I think it could be much more than two words, though, if the continue/break were to leave a let-block or multiple for-in loops.)

Comment 5

[Andreas Gal :gal]

Comment on attachment 395961 [details] [diff] [review]
v1

Take an OOM_EXIT snapshot at the top and then use that in all 3 locations in the patch (sp and rp stack overflows) by making new guard records for it 3 times.


>diff --git a/js/src/jstracer.cpp b/js/src/jstracer.cpp
>--- a/js/src/jstracer.cpp
>+++ b/js/src/jstracer.cpp
>@@ -4188,16 +4188,25 @@ TraceRecorder::prepareTreeCall(Fragment*
>         lir->insStorei(inner_sp_ins = lir->ins2i(LIR_piadd, lirbuf->sp,
>                 - treeInfo->nativeStackBase /* rebase sp to beginning of outer tree's stack */
>                 + sp_adj /* adjust for stack in outer frame inner tree can't see */
>                 + ti->nativeStackBase), /* plus the inner tree's stack base */
>                 lirbuf->state, offsetof(InterpState, sp));
>         lir->insStorei(lir->ins2i(LIR_piadd, lirbuf->rp, rp_adj),
>                 lirbuf->state, offsetof(InterpState, rp));
>     }
>+
>+    /*
>+     * The inner tree will probably access stack slots. So tell nanojit not to
>+     * discard or defer stack writes before calling js_CallTree.
>+     *
>+     * (The ExitType of this snapshot is nugatory. The exit can't be taken.)
>+     */
>+    LIns* guardRec = createGuardRecord(snapshot(OOM_EXIT));
>+    lir->insGuard(LIR_xbarrier, NULL, guardRec);
> }
> 
> static unsigned
> BuildGlobalTypeMapFromInnerTree(Queue<JSTraceType>& typeMap, VMSideExit* inner)
> {
> #if defined DEBUG
>     unsigned initialSlots = typeMap.length();
> #endif
>diff --git a/js/src/trace-test/tests/basic/testContinueWithLabel3.js b/js/src/trace-test/tests/basic/testContinueWithLabel3.js
>new file mode 100644
>--- /dev/null
>+++ b/js/src/trace-test/tests/basic/testContinueWithLabel3.js
>@@ -0,0 +1,6 @@
>+// bug 511575 comment 3
>+outer:
>+for (var j = 0; j < 10; j++)
>+    for (var p in {x:1})
>+        if (p > "q")
>+            continue outer;
>diff --git a/js/src/trace-test/tests/basic/testContinueWithLabel4.js b/js/src/trace-test/tests/basic/testContinueWithLabel4.js
>new file mode 100644
>--- /dev/null
>+++ b/js/src/trace-test/tests/basic/testContinueWithLabel4.js
>@@ -0,0 +1,25 @@
>+// bug 511575
>+
>+Function.prototype.X = 42;
>+
>+function ownProperties() {
>+    var props = {};
>+    var r = function () {};
>+
>+    outer:
>+    for (var a in r) {
>+        for (var b in r) {
>+            if (a == b) {
>+                continue outer;
>+            }
>+        }
>+        print("SHOULD NOT BE REACHED");
>+        props[a] = true;
>+    }
>+    return props;
>+}
>+
>+for (var i = 0; i < 12; ++i) {
>+    print(i);
>+    ownProperties();
>+}

Comment 6

[Jason Orendorff [:jorendorff]]

http://hg.mozilla.org/tracemonkey/rev/1c95347e920c

Comment 7

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/1c95347e920c

Comment 8

[Robert Sayre]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/dcd13e1dcc9e

Comment 9

[Bob Clary [:bc] (inactive)]

js/src/trace-test/tests/basic/testContinueWithLabel3.js
js/src/trace-test/tests/basic/testContinueWithLabel4.js

Comment 10

[Bob Clary [:bc] (inactive)]

v 1.9.3, 1.9.2