Last Comment Bug 746377 - IonMonkey: Assertion failure: mir->type() == MIRType_Value, at ion/x86/Lowering-x86.cpp:54 or Crash [@ js::ion::MDefinition::isInitProp]
: IonMonkey: Assertion failure: mir->type() == MIRType_Value, at ion/x86/Loweri...
Status: RESOLVED FIXED
[jsbugmon:update,reconfirm]
: assertion, testcase
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Other Branch
: All All
: -- major (vote)
: ---
Assigned To: Nicolas B. Pierron [:nbp]
:
Mentors:
: 746366 (view as bug list)
Depends on:
Blocks: langfuzz IonFuzz IonEager
  Show dependency treegraph
 
Reported: 2012-04-17 16:00 PDT by Christian Holler (:decoder)
Modified: 2013-02-07 05:15 PST (History)
7 users (show)
choller: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Fix JSOP_NEWINIT. (3.76 KB, patch)
2012-04-24 17:28 PDT, Nicolas B. Pierron [:nbp]
sstangl: review+
Details | Diff | Review

Description Christian Holler (:decoder) 2012-04-17 16:00:52 PDT
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) 2012-04-17 16:01:46 PDT
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) 2012-04-19 15:28:48 PDT
JSBugMon: The testcase found in this bug no longer reproduces (tried revision de015aff650d).
Comment 3 Nicolas B. Pierron [:nbp] 2012-04-24 16:09:47 PDT
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] 2012-04-24 17:28:38 PDT
Created attachment 618109 [details] [diff] [review]
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] 2012-04-25 11:56:21 PDT
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] 2012-04-25 12:19:06 PDT
(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] 2012-04-25 12:27:45 PDT
(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] 2012-04-25 12:29:33 PDT
(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] 2012-04-30 19:20:02 PDT
https://hg.mozilla.org/projects/ionmonkey/rev/086958bf99e7
Comment 10 Sean Stangl [:sstangl] 2012-05-02 13:25:49 PDT
*** Bug 746366 has been marked as a duplicate of this bug. ***
Comment 11 Christian Holler (:decoder) 2013-02-07 05:15:17 PST
Automatically extracted testcase for this bug was committed:

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

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