Last Comment Bug 759323 - IonMonkey: (ARM) Assertion failure: branch.is<InstBranchImm>(), at arm/Assembler-arm.cpp:1944 or Crash [@ js::ion::Instruction::encode]
: IonMonkey: (ARM) Assertion failure: branch.is<InstBranchImm>(), at arm/Assemb...
Status: RESOLVED FIXED
: assertion, testcase
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Other Branch
: ARM Linux
: -- major (vote)
: ---
Assigned To: general
:
Mentors:
Depends on:
Blocks: langfuzz IonFuzz
  Show dependency treegraph
 
Reported: 2012-05-29 06:54 PDT by Christian Holler (:decoder)
Modified: 2012-06-12 15:12 PDT (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
/home/mrosenberg/patches/PoolSpew-r0.patch (65.28 KB, patch)
2012-06-01 00:04 PDT, Marty Rosenberg [:mjrosenb]
Jacob.Bramley: review+
Details | Diff | Splinter Review

Description Christian Holler (:decoder) 2012-05-29 06:54:37 PDT
The following testcase asserts on ionmonkey-arm (private branch) revision  (run with --ion -n -m --ion-eager):


function add_p0to127(o) {
  o.p00 = ++i;o.p01 = ++i;o.p02 = ++i;o.p03 = ++i;o.p04 = ++i;o.p05 = ++i;o.p06 = ++i;o.p07 = ++i;
  o.p10 = ++i;o.p11 = ++i;o.p12 = ++i;o.p13 = ++i;o.p14 = ++i;o.p15 = -i;o.p16 = ++i;o.p17 = ++i;
  o.p60 = ++i;o.p61 = ++i;o.p62 = ++i;o.p63 = ++i;o.p64 = ++i;o.p65 = ++i;o.p66 = ++i;o.p67 = ++i;
  o.p70 = ++i;o.p71 = ++i;o.p72 = ++i;o.p73 = ++i;o.p74 = ++i;o.p75 = ++i;o.p76 = ++i;o.p77 = ++i;
  o.p80 = ++i;o.p81 = ++i;o.p82 = ++i;o.p83 = ++i;o.p84 = ++i;o.p85 = ++i;o.p86 = ++i;o.p87 = ++i;
  o.p90 = ++i;o.p91 = ++i;o.p92 = ++i;o.p93 = ++i;o.p94 = ++i;o.p95 = ++i;o.p96 = ++i;o.p97 = ++i;
  o.p100= ++i;o.p101= ++i;o.p102= ++i;o.p103= ++i;o.p104= ++i;o.p105= ++i;o.p106= ++i;o.p107= ++i;
  o.p110= ++i;o.p111= ++i;o.p112= ++i;o.p113= ++i;o.p114= ++i;o.p115= ++i;o.p116= ++i;o.p117= ++i;
  o.p120= ++i;o.p121= ++i;o.p122= ++i;o.p123= ++i;o.p124= ++i;o.p125= ++i;o.p126= ++i;o.p127= ++i;
}
var oarr = [];
for (var i = 0; i < 2; i++)
  oarr[i] = {};
var o = add_p0to127(oarr[0]);
var o2 = add_p0to127(oarr[1]);
Comment 1 Marty Rosenberg [:mjrosenb] 2012-05-29 16:44:41 PDT
whee, pool bugs!
not yet confirmed, but my theory is currently that we attempt to emit a conditional branch,
and record its offset.  We need to record its offset in order to patch the correct location when its label gets bound.
As we attempt to emit the branch, we notice that a pool needs to be dumped, so rather than emitting the branch as expected, we emit a branch over the pool, which has the offset that was recorded earlier.
Later, we attempt to patch up the actual branch and instead get the branch-over-pool, and since it looks like a branch, everything is fine, but this branch doesn't look like the last branch in the list, so we attempt to extract the next branch, which is generally not actually a branch.

I'm going to blame the system of of asking for the offset of the next instruction, then adding the next instruction.
My proposal for a fix is to have the process of adding the instruction also return its offset, so the values are always in sync.
Comment 2 Marty Rosenberg [:mjrosenb] 2012-06-01 00:04:26 PDT
Created attachment 629089 [details] [diff] [review]
/home/mrosenberg/patches/PoolSpew-r0.patch

Evidently, I was not really paying attention when I created this patch, since it is two patches rolled into one.  The first actually fixes this bug.  The second adds in a spew channel for the pool code, since the pool code is quite complex, and having it spew debug data is useful.
Comment 3 Jacob Bramley [:jbramley] 2012-06-01 07:19:01 PDT
Comment on attachment 629089 [details] [diff] [review]
/home/mrosenberg/patches/PoolSpew-r0.patch

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

r+ with minor fixes.

::: js/src/ion/IonSpewer.cpp
@@ +216,5 @@
>              "  bailouts   Bailouts\n"
>              "  caches     Inline caches\n"
>              "  osi        Invalidation\n"
>              "  safepoints Safepoints\n"
> +            "  pools      Pools (ARM only for now)\n"

It's probably best to call them "Literal pools" in the description, just for clarification for those not familiar with ARM.

::: js/src/ion/arm/Assembler-arm.cpp
@@ +46,5 @@
>  #include "assembler/jit/ExecutableAllocator.h"
>  
>  using namespace js;
>  using namespace js::ion;
> +void dbg_break() {}

Shouldn't dbg_break do something, or is it just a hook to attach a breakpoint to? If the latter, it's debug code and should be removed.

@@ +1119,5 @@
>  Assembler::align(int alignment)
>  {
> +    BufferOffset ret;
> +    while (!m_buffer.isAligned(alignment)) {
> +        BufferOffset tmp = as_mov(r0, O2Reg(r0));

Shouldn't you use as_nop()?

@@ +1121,5 @@
> +    BufferOffset ret;
> +    while (!m_buffer.isAligned(alignment)) {
> +        BufferOffset tmp = as_mov(r0, O2Reg(r0));
> +        if (!ret.assigned())
> +            ret = tmp;

If the buffer was already aligned, this will return an unassigned BufferOffset. Is that intentional?

@@ +1131,3 @@
>  Assembler::as_nop()
>  {
> +    return writeInst(0xe320f000);

"mov r0, r0" is actually a more suitable no-op instruction than the architectural "nop".

@@ +1590,2 @@
>  }
> +static int labelSTOP = 0;

This is only used in one place in the whole patch. Given that the value is just zero, can't you just hard-code it? The code that uses it would become this:

if (!l->id) {
  dbg_break();
}

@@ +1601,5 @@
>      if (l->bound()) {
> +        BufferOffset ret = as_nop();
> +        Instruction *i = editSrc(ret);
> +        as_b(BufferOffset(l).diffB<BOffImm>(ret), c, ret);
> +        return ret;

This change emits a NOP, then overwrites it with a branch. Why not just emit a branch?

@@ +1606,3 @@
>      } else {
>          // Ugh.  int32 :(
> +        int32 old = l->used() ? l->offset() : LabelBase::INVALID_OFFSET;

Since you already test l->used(), you should move the ternary stuff into the existing if/else statement.

@@ +1617,3 @@
>          }
> +        int32 check = l->use(ret.getOffset());
> +        JS_ASSERT(check == old);

'check' is only used in DEBUG mode, so it will generate a warning for release builds.

@@ +1628,4 @@
>  }
>  
>  // blx can go to either an immediate or a register.
>  // When blx'ing to a register, we change processor mode

To comply with architectural terms: s/mode/state/

(I know it's not in your change, but it's nearby.)

@@ +1637,4 @@
>  {
> +    return writeInst(((int) c) | op_blx | r.code());
> +}
> +BufferOffset

Add a similar comment as for as_blx. That is, bl can only go to an immediate offset, and it never changes processor state.

@@ +1646,2 @@
>  // bl #imm can have a condition code, blx #imm cannot.
>  // blx reg can be conditional.

This comment doesn't really fit here, since as_bl only generates bl <imm> anyway.

@@ +1651,4 @@
>      if (l->bound()) {
> +        BufferOffset ret = as_nop();
> +        as_bl(BufferOffset(l).diffB<BOffImm>(ret), c, ret);
> +        return ret;

Again, why generate a NOP then overwrite it?

@@ +1655,2 @@
>      } else {
> +        int32 old = l->used() ? l->offset() : LabelBase::INVALID_OFFSET ;

If that operation is going to be common, it should be a new method of Label: l->offset_if_used()

@@ +1741,4 @@
>  Assembler::as_vnmls(VFPRegister vd, VFPRegister vn, VFPRegister vm,
>                    Condition c)
>  {
>      JS_NOT_REACHED("Feature NYI");

Return a value anyway, to avoid compiler warnings.

::: js/src/ion/shared/IonAssemblerBufferWithConstantPools.h
@@ +628,5 @@
>          // is being filled in. The backwards half of the pool is always in a state
>          // where it is sane. Everything that needs to be done here is for "sanity's sake".
>          // The per-buffer pools need to be reset, and we need to record the size of the pool.
> +        IonSpew(IonSpew_Pools, "Finishing pool %d", numDumps);
> +       JS_ASSERT(inBackref);

Indentation.
Comment 4 Marty Rosenberg [:mjrosenb] 2012-06-01 14:50:31 PDT
(In reply to Jacob Bramley [:jbramley] from comment #3)
> Comment on attachment 629089 [details] [diff] [review]
> /home/mrosenberg/patches/PoolSpew-r0.patch
> 
> Review of attachment 629089 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r+ with minor fixes.
> 
> ::: js/src/ion/IonSpewer.cpp
> @@ +216,5 @@
> >              "  bailouts   Bailouts\n"
> >              "  caches     Inline caches\n"
> >              "  osi        Invalidation\n"
> >              "  safepoints Safepoints\n"
> > +            "  pools      Pools (ARM only for now)\n"
> 
> It's probably best to call them "Literal pools" in the description, just for
> clarification for those not familiar with ARM.
> 
> ::: js/src/ion/arm/Assembler-arm.cpp
> @@ +46,5 @@
> >  #include "assembler/jit/ExecutableAllocator.h"
> >  
> >  using namespace js;
> >  using namespace js::ion;
> > +void dbg_break() {}
> 
> Shouldn't dbg_break do something, or is it just a hook to attach a
> breakpoint to? If the latter, it's debug code and should be removed.
> 
It is just a hook, but it was already there, I just moved it up.  I can pull it out if you want.

> @@ +1121,5 @@
> > +    BufferOffset ret;
> > +    while (!m_buffer.isAligned(alignment)) {
> > +        BufferOffset tmp = as_mov(r0, O2Reg(r0));
> > +        if (!ret.assigned())
> > +            ret = tmp;
> 
> If the buffer was already aligned, this will return an unassigned
> BufferOffset. Is that intentional?
> 
yes, no instructions were emitted, so there is no reason to know their offset.

 
> @@ +1590,2 @@
> >  }
> > +static int labelSTOP = 0;
> 
> This is only used in one place in the whole patch. Given that the value is
> just zero, can't you just hard-code it? The code that uses it would become
> this:
> 
> if (!l->id) {
>   dbg_break();
> }
> 
the point is I can set it at runtime as the need arises
in fact, in my .gdbinit, I set a breakpoint on main that sets a whole bunch of these variables.

> @@ +1601,5 @@
> >      if (l->bound()) {
> > +        BufferOffset ret = as_nop();
> > +        Instruction *i = editSrc(ret);
> > +        as_b(BufferOffset(l).diffB<BOffImm>(ret), c, ret);
> > +        return ret;
> 
> This change emits a NOP, then overwrites it with a branch. Why not just emit
> a branch?
Long story short, because this will be more obvious if I got it wrong.
I can't emit a branch with the correct offset outright because I never know what the offset of the instruction will be until we've already written it.  This may be part of the reason that I initially didn't
use this method of dealing with offsets.  I think I may change the entire interface to just return an Instruction*, so the assembler can write the data, and the AssemblerBuffer just deals with "amount of space" 
> @@ +1606,3 @@
> >      } else {
> >          // Ugh.  int32 :(
> > +        int32 old = l->used() ? l->offset() : LabelBase::INVALID_OFFSET;
> 
> Since you already test l->used(), you should move the ternary stuff into the
> existing if/else statement.

Done.  I'd initially done that so |old| would always be initialized.

> ::: js/src/ion/shared/IonAssemblerBufferWithConstantPools.h
> @@ +628,5 @@
> >          // is being filled in. The backwards half of the pool is always in a state
> >          // where it is sane. Everything that needs to be done here is for "sanity's sake".
> >          // The per-buffer pools need to be reset, and we need to record the size of the pool.
> > +        IonSpew(IonSpew_Pools, "Finishing pool %d", numDumps);
> > +       JS_ASSERT(inBackref);
> 
> Indentation.
Comment 5 Marty Rosenberg [:mjrosenb] 2012-06-12 15:12:44 PDT
Landed: http://hg.mozilla.org/projects/ionmonkey/rev/6aaf148d4ce8

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