746377 - IonMonkey: Assertion failure: mir->type() == MIRType_Value, at ion/x86/Lowering-x86.cpp:54 or Crash [@ js::ion::MDefinition::isInitProp]

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

The following testcase asserts on ionmonkey revision 67bf9a4a1f77 (run with --ion -n):


var actual = '';
test();
function test()
{
  a = {x: 1};
  b = {__proto__: a};
    print(actual += test(1,2,3,4));
}

Comment 1

[Christian Holler (:decoder)]

Crash when stepping through assertions looks similar to that in bug 746366:


Program received signal SIGABRT, Aborted.
Assertion failure: live->empty(), at /home/ownhero/homes/mozilla/repos/ionmonkey/js/src/ion/LinearScan.cpp:719

Program received signal SIGABRT, Aborted.

Program received signal SIGSEGV, Segmentation fault.
0x0847e359 in js::ion::MDefinition::isInitProp (this=0x0) at ../ion/MIR.h:441
441         MIR_OPCODE_LIST(OPCODE_CASTS)
(gdb) bt 8
#0  0x0847e359 in js::ion::MDefinition::isInitProp (this=0x0) at ../ion/MIR.h:441
#1  0x0847e903 in js::ion::MDefinition::toInitProp (this=0x0) at ../ion/MIR.h:4394
#2  0x0847fd06 in js::ion::LInitProp::mir (this=0x872cf38) at ../ion/LIR-Common.h:280
#3  0x08485ec5 in js::ion::CodeGenerator::visitInitProp (this=0xffffb4a0, lir=0x872cf38) at /home/ownhero/homes/mozilla/repos/ionmonkey/js/src/ion/CodeGenerator.cpp:1008
#4  0x083eacf7 in js::ion::LInitProp::accept (this=0x872cf38, visitor=0xffffb4a0) at /home/ownhero/homes/mozilla/repos/ionmonkey/js/src/ion/LIR-Common.h:260
#5  0x08485463 in js::ion::CodeGenerator::generateBody (this=0xffffb4a0) at /home/ownhero/homes/mozilla/repos/ionmonkey/js/src/ion/CodeGenerator.cpp:824
#6  0x0848be21 in js::ion::CodeGenerator::generate (this=0xffffb4a0) at /home/ownhero/homes/mozilla/repos/ionmonkey/js/src/ion/CodeGenerator.cpp:2048
#7  0x083972ab in TestCompiler (builder=..., graph=...) at /home/ownhero/homes/mozilla/repos/ionmonkey/js/src/ion/Ion.cpp:732

Comment 2

[Christian Holler (:decoder)]

JSBugMon: The testcase found in this bug no longer reproduces (tried revision de015aff650d).

Comment 3

[Nicolas B. Pierron [:nbp]]

I can reproduce it with:

./js --ion --ion-eager -n ./jit-test/tests/basic/bug732693.js
Assertion failure: mir->type() == MIRType_Value, at ./ion/x64/Lowering-x64.cpp:54

The assertion is caused by the lack of typePolicy function in the MIR.

Comment 4

[Nicolas B. Pierron [:nbp]]

Attachment: Fix JSOP_NEWINIT.

* Fix argument order for InitProp VM function.
* NewObject is not initialized the same way as NewInit.
* Set mir used unconditionally by the code generator.
* Add Type policy to convert the input when needed.

Comment 5

[Sean Stangl [:sstangl]]

Comment on attachment 618109 [details] [diff] [review]
Fix JSOP_NEWINIT.

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

::: js/src/ion/VMFunctions.cpp
@@ +223,5 @@
>  
>  JSObject*
>  NewInitObject(JSContext *cx, HandleObject baseObj, types::TypeObject *type)
>  {
> +    RootedVarObject obj(cx);

MethodJIT just uses "JSObject *". Should be safe to use that.

@@ +224,5 @@
>  JSObject*
>  NewInitObject(JSContext *cx, HandleObject baseObj, types::TypeObject *type)
>  {
> +    RootedVarObject obj(cx);
> +    if (*baseObj.address()) {

This should be "if (baseObj)". For the JSOP_NEWINIT case, baseObj is NULL, and attempting to take .address() will segfault.

Comment 6

[Nicolas B. Pierron [:nbp]]

(In reply to Sean Stangl from comment #5)
> Comment on attachment 618109 [details] [diff] [review]
> Fix JSOP_NEWINIT.
> 
> Review of attachment 618109 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/ion/VMFunctions.cpp
> @@ +223,5 @@
> >  
> >  JSObject*
> >  NewInitObject(JSContext *cx, HandleObject baseObj, types::TypeObject *type)
> >  {
> > +    RootedVarObject obj(cx);
> 
> MethodJIT just uses "JSObject *". Should be safe to use that.

If methodJIT is not using a RootedVarObject, this is likely to be a bug which will cause issue with moving GC.

terrence: can you confirm this ?

> @@ +224,5 @@
> >  JSObject*
> >  NewInitObject(JSContext *cx, HandleObject baseObj, types::TypeObject *type)
> >  {
> > +    RootedVarObject obj(cx);
> > +    if (*baseObj.address()) {
> 
> This should be "if (baseObj)". For the JSOP_NEWINIT case, baseObj is NULL,
> and attempting to take .address() will segfault.

HandleObject is never null and target the JSObject* which is pushed on the stack.  HandleObject::address return the ptr_ field (JSObject**) which is never null (as ensured by callVM).  Taking the address of it allow use to read the value.  Another way would be to check reinterpret_cast<JSObject*>(baseObj) instead of *baseObj.address()

Comment 7

[Sean Stangl [:sstangl]]

(In reply to Nicolas B. Pierron [:pierron] from comment #6)
> HandleObject is never null and target the JSObject* which is pushed on the
> stack.  HandleObject::address return the ptr_ field (JSObject**) which is
> never null (as ensured by callVM).  Taking the address of it allow use to
> read the value.  Another way would be to check
> reinterpret_cast<JSObject*>(baseObj) instead of *baseObj.address()

Are you sure it's actually a HandleObject? The argument passing is via "pushArg(ImmGCPtr(lir->mir()->baseObj()))", where baseObj() returns a JSObject *.

Comment 8

[Sean Stangl [:sstangl]]

(In reply to Sean Stangl from comment #7)
> Are you sure it's actually a HandleObject? The argument passing is via
> "pushArg(ImmGCPtr(lir->mir()->baseObj()))", where baseObj() returns a
> JSObject *.

Nicolas explained to me in-person that the conversion from JSObject* to HandleObject occurs magically in callVM function templates, so the above is fine.

Comment 9

[Nicolas B. Pierron [:nbp]]

https://hg.mozilla.org/projects/ionmonkey/rev/086958bf99e7

Comment 11

[Christian Holler (:decoder)]

Automatically extracted testcase for this bug was committed:

https://hg.mozilla.org/mozilla-central/rev/2e891e0db397