732863 - IonMonkey: Crash [@ JSString::isAtom]

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

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)]

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]]

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]]

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 [Laid off!]]

Can you paste the compartment's type printout? (cx->compartment->types.print)

Comment 5

[Nicolas B. Pierron [:nbp]]

Attachment: 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 [Laid off!]]

(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]]

Attachment: 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] - inactive, e-mail if emergency]

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]]

https://hg.mozilla.org/projects/ionmonkey/rev/e159956eb94c

Comment 10

[Christian Holler (:decoder)]

A testcase for this bug was automatically identified at js/src/jit-test/tests/ion/bug732863.js.