Last Comment Bug 732863 - IonMonkey: Crash [@ JSString::isAtom]
: IonMonkey: Crash [@ JSString::isAtom]
Status: RESOLVED FIXED
: crash, testcase
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Other Branch
: x86_64 Linux
: -- major (vote)
: ---
Assigned To: Nicolas B. Pierron [:nbp]
:
:
Mentors:
Depends on:
Blocks: langfuzz IonFuzz
  Show dependency treegraph
 
Reported: 2012-03-04 18:13 PST by Christian Holler (:decoder)
Modified: 2013-01-14 08:15 PST (History)
7 users (show)
choller: in‑testsuite+
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Type status during the second compilation of addThis. (4.52 KB, text/plain)
2012-03-10 16:38 PST, Nicolas B. Pierron [:nbp]
no flags Details
Do not ignore type barrier. (2.44 KB, patch)
2012-03-10 21:16 PST, Nicolas B. Pierron [:nbp]
dvander: review+
Details | Diff | Splinter Review

Description Christian Holler (:decoder) 2012-03-04 18:13:48 PST
The following testcase crashes on ionmonkey revision 1fd6c40d3852 (run with --ion -n -m --ion-eager):


function TestCase(n, d, e, a) {}
function reportCompare (expected, actual, description) {
  var testcase = new TestCase("unknown-test-name", description, expected, actual);
  try   {  }  catch(ex)  {  }
}
var UBound = 0;
var statusitems = [ ];
var actual = '';
var actualvalues = [ ];
var expectedvalues = [ ];
addThis();
actual %=  match(1, 2, Math.exp, Math.log);
addThis();
function match(x, y, F, G) {}
function addThis() {
  actualvalues[UBound] += actual;
  UBound++;
  for (var i = 0; i < UBound; i++) {
    reportCompare(expectedvalues[i], actualvalues[i], statusitems[i]);
  }
}
Comment 1 Christian Holler (:decoder) 2012-03-04 18:14:43 PST
Crash trace:

Program received signal SIGSEGV, Segmentation fault.
0x0000000000444536 in JSString::isAtom (this=0x0) at /home/ownhero/homes/mozilla/repos/ionmonkey/js/src/vm/String.h:385
385             bool atomized = (d.lengthAndFlags & ATOM_MASK) == ATOM_FLAGS;
(gdb) bt
#0  0x0000000000444536 in JSString::isAtom (this=0x0) at /home/ownhero/homes/mozilla/repos/ionmonkey/js/src/vm/String.h:385
#1  0x000000000044674e in js::CompartmentChecker::check (this=0x7fffffffb140, str=0x0) at ../jscntxtinlines.h:184
#2  0x00000000004467e4 in js::CompartmentChecker::check (this=0x7fffffffb140, v=...) at ../jscntxtinlines.h:192
#3  0x0000000000449be3 in js::assertSameCompartment<JS::Value> (cx=0xcd5d00, t1=...) at ../jscntxtinlines.h:254
#4  0x000000000050f374 in js::Interpret (cx=0xcd5d00, entryFrame=0x7ffff0beb1d0, interpMode=js::JSINTERP_NORMAL)
    at /home/ownhero/homes/mozilla/repos/ionmonkey/js/src/jsinterp.cpp:3090
#5  0x0000000000500170 in js::RunScript (cx=0xcd5d00, script=0x7ffff09072e0, fp=0x7ffff0beb1d0) at /home/ownhero/homes/mozilla/repos/ionmonkey/js/src/jsinterp.cpp:463
#6  0x00000000005004b7 in js::InvokeKernel (cx=0xcd5d00, args=..., construct=js::NO_CONSTRUCT) at /home/ownhero/homes/mozilla/repos/ionmonkey/js/src/jsinterp.cpp:526
#7  0x0000000000460c89 in js::Invoke (cx=0xcd5d00, args=..., construct=js::NO_CONSTRUCT) at /home/ownhero/homes/mozilla/repos/ionmonkey/js/src/jsinterp.h:157
#8  0x00000000005006b0 in js::Invoke (cx=0xcd5d00, thisv=..., fval=..., argc=3, argv=0x7fffffffc260, rval=0x7fffffffc228)
    at /home/ownhero/homes/mozilla/repos/ionmonkey/js/src/jsinterp.cpp:558
#9  0x0000000000868224 in js::ion::InvokeFunction (cx=0xcd5d00, fun=0x7ffff090f780, argc=3, argv=0x7fffffffc258, rval=0x7fffffffc228)
    at /home/ownhero/homes/mozilla/repos/ionmonkey/js/src/ion/VMFunctions.cpp:65
Comment 2 Nicolas B. Pierron [:nbp] 2012-03-09 14:42:52 PST
I am able to reproduce it with the following (--ion -n --ion-eager):

// Compiled
function TestCase(a) { }

// Not compiled (try)
function reportCompare (actual) {
  TestCase(actual);
  try   {  }  catch(ex)  {  }
}

// Compiled
function addThis(bound) {
    actualvalues[bound] = undefined + actual;
    reportCompare(actualvalues[bound]);
}

var actual = '';
var actualvalues = [];
addThis(0);
actual = NaN;
addThis(1);


Reducing the test case more cause the SEGV to disappear.  The first call of addThis cause the specialization of the assembly, which trigger another compilation at the second call of addThis which then cause the SEGV when checking the arguments of TestCase function.
Comment 3 Nicolas B. Pierron [:nbp] 2012-03-09 20:28:25 PST
function addThis(bound) {
    actualvalues[bound] = undefined + actual;
    reportCompare(actualvalues[bound]);
}

loc   line  op
----- ----  --
main:
00000:  12  getgname "actualvalues"
00005:  12  getarg 0
00008:  12  getgname "undefined"
00013:  12  getgname "actual"
00018:  12  add
00019:  12  setelem (1)
00020:  12  pop
00021:  13  callgname "reportCompare"
00026:  13  undefined
00027:  13  notearg
00028:  13  getgname "actualvalues"
00033:  13  getarg 0
00036:  13  getelem (2)
00037:  13  notearg
00038:  13  call 1
00041:  13  pop
00042:  13  stop


js::analyze::ScriptAnalysis::typeBarriers (this=0x7ffff7f5c850, cx=0xce5d00, offset=0)
Value returned = (js::types::TypeBarrier *) 0x0

js::analyze::ScriptAnalysis::typeBarriers (this=0x7ffff7f5c850, cx=0xce5d00, offset=13)
Value returned = (js::types::TypeBarrier *) 0x7ffff7f5d3e0

js::analyze::ScriptAnalysis::typeBarriers (this=0x7ffff7f5c850, cx=0xce5d00, offset=21)
Value returned = (js::types::TypeBarrier *) 0x0

js::analyze::ScriptAnalysis::typeBarriers (this=0x7ffff7f5c850, cx=0xce5d00, offset=28)
Value returned = (js::types::TypeBarrier *) 0x0

js::analyze::ScriptAnalysis::typeBarriers (this=0x7ffff7f5c850, cx=0xce5d00, offset=36)
Value returned = (js::types::TypeBarrier *) 0x0   # wrong !


The code store a value at location (1) to «actualvalues» but the type inference does not infer that the  getelem (2) output of «actualvalues» can be a value, but only returns the previous observed type, which has been observed at the first call of addThis (i-e a String).
Comment 4 Brian Hackett (:bhackett) 2012-03-10 06:29:44 PST
Can you paste the compartment's type printout? (cx->compartment->types.print)
Comment 5 Nicolas B. Pierron [:nbp] 2012-03-10 16:38:27 PST
Created attachment 604694 [details]
Type status during the second compilation of addThis.

Thanks, this output is extremely helpful for debugging.  So from this output, I deduce that the type barrier which is produced on «actual» should cause a bailout.
Comment 6 Brian Hackett (:bhackett) 2012-03-10 17:12:51 PST
(In reply to Nicolas B. Pierron [:pierron] from comment #5)
> Created attachment 604694 [details]
> Type status during the second compilation of addThis.
> 
> Thanks, this output is extremely helpful for debugging.  So from this
> output, I deduce that the type barrier which is produced on «actual» should
> cause a bailout.

Yeah, that barrier on actual should trip the second time the function is executed and invalidate the jitcode.
Comment 7 Nicolas B. Pierron [:nbp] 2012-03-10 21:16:23 PST
Created attachment 604729 [details] [diff] [review]
Do not ignore type barrier.

Make sure type barrier are always inserted even for non-effectful instruction, because type barrier are the assumptions of the type-inference.
Comment 8 David Anderson [:dvander] 2012-03-12 18:08:03 PDT
Comment on attachment 604729 [details] [diff] [review]
Do not ignore type barrier.

Review of attachment 604729 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/ion/IonBuilder.cpp
@@ -3137,5 @@
>              return pushConstant(NullValue());
>          break;
>        default:
> -        MUnbox::Mode mode = ins->isEffectful() ? MUnbox::TypeBarrier : MUnbox::Fallible;
> -        barrier = MUnbox::New(ins, MIRTypeFromValueType(type), mode);

We can't change this though. A type barrier failure will look at regs.pc for reflowing type information, but regs.pc is undefined for idempotent instructions.

As-is, this code should work, because the fallible version will trigger a bailout and re-run the whole instruction.

::: js/src/ion/MIR.h
@@ +1323,5 @@
>      {
>          JS_ASSERT(ins->type() == MIRType_Value);
>          setResultType(type);
>          setMovable();
> +        if (mode_ == TypeBarrier)

Nice catch - yeah, we definitely always need the guard here.
Comment 9 Nicolas B. Pierron [:nbp] 2012-03-13 09:57:49 PDT
https://hg.mozilla.org/projects/ionmonkey/rev/e159956eb94c
Comment 10 Christian Holler (:decoder) 2013-01-14 08:15:22 PST
A testcase for this bug was automatically identified at js/src/jit-test/tests/ion/bug732863.js.

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