Last Comment Bug 730108 - IonMonkey: Pool bug
: IonMonkey: Pool bug
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: ARM Linux
: -- normal (vote)
: ---
Assigned To: Marty Rosenberg [:mjrosenb]
:
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-02-23 14:15 PST by Marty Rosenberg [:mjrosenb]
Modified: 2012-03-16 16:25 PDT (History)
1 user (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
/home/mrosenberg/patches/more_pool_fixes-r0.patch (19.74 KB, patch)
2012-02-29 05:02 PST, Marty Rosenberg [:mjrosenb]
Jacob.Bramley: review+
Details | Diff | Splinter Review
/home/mrosenberg/patches/more_pool_fixes-r1.patch (21.83 KB, patch)
2012-03-02 00:04 PST, Marty Rosenberg [:mjrosenb]
Jacob.Bramley: review+
Details | Diff | Splinter Review

Description Marty Rosenberg [:mjrosenb] 2012-02-23 14:15:06 PST
The bug is kind of silly, and easily fixable, but there is a similar bug that will require a bit more effort to fix.

What goes wrong:
1) choose a perforation point (unconditional branch) before making any pool references
2) generate a whole bunch of code without any unconditional branches
3) generate a pool reference
At this point, there are several thousand bytes between the perforation point and the reference to the pool.  Eventually, the pool is flushed, and we attempt to use the perforation point, so all of the constants are (correctly) placed into the back-reference half of the pool, at which point they are out of range of the actual pool.  When we go to generate the references, an assertion is thrown.

The easiest fix for now is to not select an unconditional branch as a perforation point if there are no pool entries.

A more general fix will be to monitor when an element moves out of range of the backwards pool while it is being filled from the forwards pool, and to immediately start a new forwards pool
Comment 1 Marty Rosenberg [:mjrosenb] 2012-02-29 05:02:49 PST
Created attachment 601579 [details] [diff] [review]
/home/mrosenberg/patches/more_pool_fixes-r0.patch

Unfortunately, I am a bad person, and while writing this patch, also managed to include the fix for bug 728850.  Since there isn't a good file-level separation between the patches, and I was planning on having Jacob review both patches in any case, I decided to not split them.
Comment 2 Jacob Bramley [:jbramley] 2012-02-29 08:48:09 PST
Comment on attachment 601579 [details] [diff] [review]
/home/mrosenberg/patches/more_pool_fixes-r0.patch

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

The only major issue is a pointer aliasing violation, but this could be fixed with a minor modification.

::: js/src/ion/arm/Assembler-arm.cpp
@@ +1969,5 @@
>          new (i) InstBLImm(BOffImm(offset), cond);
>      JSC::ExecutableAllocator::cacheFlush(i, 4);
>  }
> +struct PoolHeader {
> +    // size should take into account the pool header.

You should also mention that the units are sizeof(Instruction), not bytes.

@@ +2053,5 @@
> +Assembler::nextInstruction(uint8 *inst_, uint32 *count)
> +{
> +    Instruction *inst = reinterpret_cast<Instruction*>(inst_);
> +    if (count != NULL)
> +        *count += 4;

a: 'sizeof(Instruction)' might be better than '4'.

b: The way this is called, I expected *count to be an offset between inst_ and the next instruction. That is, 'inst->next() - inst_'. The caller could just as well use a 'for' loop to calculate the count value as it's implemented here.

@@ +2058,5 @@
> +    return reinterpret_cast<uint8*>(inst->next());
> +}
> +bool extractPoolHeader(Instruction *inst, PoolHeader **ph)
> +{
> +    PoolHeader *tmp = reinterpret_cast<PoolHeader *>(inst);

Pointer aliasing is hard to avoid at times, but it's dangerous and can cause bugs that are obscure and hard to debug. This line of code violates C/C++ pointer aliasing rules, since PoolHeader and Instruction are not compatible types.

@@ +2070,5 @@
> +// Cases to be handled:
> +// 1) no pools in sight => return this+1
> +// 2) this+1 is an artificial guard for a pool => return first instruction after the pool
> +// 3) this+1 is a natural guard => return the branch
> +// 4) this is a branch, right before a pool => return first instruction after the pool

This is a helpful comment.

@@ +2077,5 @@
> +{
> +    Assembler::Condition c;
> +    Instruction *ret = this+1;
> +    bool thisIsGuard = false;
> +    bool nextIsGuard = false;

These booleans are unused.

@@ +2085,5 @@
> +    // If it isn't a guard, then start looking ahead.
> +    if (c == Assembler::Always &&
> +        (this->is<InstBXReg>() || this->is<InstBImm>()) &&
> +        extractPoolHeader(ret, &ph)) {
> +        return ret+ph->size;

Spaces around the '+'.

@@ +2090,5 @@
> +    } else {
> +        ret->extractCond(&c);
> +        if (c == Assembler::Always && (ret->is<InstBXReg>() || ret->is<InstBImm>())) {
> +            if (extractPoolHeader(ret + 1, &ph) && !ph->isNatural)
> +                return ret + 1 + ph->size;

This could be much clearer if you made helper functions to check for guards. To illustrate:

if (this->isPoolGuard(&ph)) {
    return ret + ph->size;
} else if (ret->isArtificialPoolGuard(&ph)) {
    return ret + 1 + ph->size;
}
return ret;

::: js/src/ion/arm/Assembler-arm.h
@@ +1683,5 @@
>              *c = (Assembler::Condition)(data & 0xf0000000);
>      }
>      // Get the next instruction in the instruction stream.
>      // This will likely eventually do neat things like ignore
>      // constant pools and their guards.

You did that! The comment needs updating.

::: js/src/ion/shared/IonAssemblerBufferWithConstantPools.h
@@ +204,5 @@
>      }
>      int getAlignment() {
>          return alignment;
>      }
> +    uint32 addPool(uint32 start) {

The name of this method is a little misleading. I'm not sure that I can think of a better one, though. What do you think of "addPoolSize"?
Comment 3 Marty Rosenberg [:mjrosenb] 2012-03-02 00:03:29 PST
(In reply to Jacob Bramley [:jbramley] from comment #2)


> a: 'sizeof(Instruction)' might be better than '4'.
> 

> b: The way this is called, I expected *count to be an offset between inst_
> and the next instruction. That is, 'inst->next() - inst_'. The caller could
> just as well use a 'for' loop to calculate the count value as it's
> implemented here.
> 
Possibly, but |inst->next() - inst| is going to return the number of bytes between them, rather than the number of bytes worth of instructions between them, the important difference being pools. If there were a pool between inst and inst->next(), the value assumed would be huge, and wrong.


> Pointer aliasing is hard to avoid at times, but it's dangerous and can cause
> bugs that are obscure and hard to debug. This line of code violates C/C++
> pointer aliasing rules, since PoolHeader and Instruction are not compatible
> types.
> 
I've decided to go a slightly more complex route, and make PoolHeader inherit from Instruction.
I'll probably do the same for pool hints, eventually.



> @@ +2090,5 @@
> > +    } else {
> > +        ret->extractCond(&c);
> > +        if (c == Assembler::Always && (ret->is<InstBXReg>() || ret->is<InstBImm>())) {
> > +            if (extractPoolHeader(ret + 1, &ph) && !ph->isNatural)
> > +                return ret + 1 + ph->size;
> 
> This could be much clearer if you made helper functions to check for guards.
> To illustrate:
> 
> if (this->isPoolGuard(&ph)) {
>     return ret + ph->size;
> } else if (ret->isArtificialPoolGuard(&ph)) {
>     return ret + 1 + ph->size;
> }
> return ret;
> 
Indeed it is!
Comment 4 Marty Rosenberg [:mjrosenb] 2012-03-02 00:04:56 PST
Created attachment 602274 [details] [diff] [review]
/home/mrosenberg/patches/more_pool_fixes-r1.patch

normally fixing nits isn't cause for re-review, but I did make some pretty serious changes to PoolHeader.
Comment 5 Jacob Bramley [:jbramley] 2012-03-02 01:51:47 PST
Comment on attachment 602274 [details] [diff] [review]
/home/mrosenberg/patches/more_pool_fixes-r1.patch

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

Good stuff!

::: js/src/ion/arm/Assembler-arm.cpp
@@ +1985,5 @@
> +        Header(const Instruction *i) {
> +            JS_STATIC_ASSERT(sizeof(Header) == sizeof(uint32));
> +            memcpy(this, i, sizeof(Header));
> +            JS_ASSERT(ONES == 0xffff);
> +       }

The indentation is wrong here.

@@ +2003,5 @@
> +        Header tmp(this);
> +        return tmp.isNatural;
> +    }
> +    static bool isTHIS(const Instruction &i) {
> +        return (*i.raw() & 0xffff0000) == 0xffff0000;

This same mechanism could be used for size() and isNatural() too, and then you could do away with the whole Header struct and its bitfields. Maybe I'm too used to manually masking bits off in C! The Header allows you to use this 4-byte header even if sizeof(Instruction) is not 4, but isTHIS makes that assumption anyway.

@@ +2128,5 @@
> +    // If it isn't a guard, then start looking ahead.
> +    if (instIsGuard(this, &ph)) {
> +        return ret + ph->size();
> +    } else {
> +        if (instIsArtificialGuard(ret, &ph))

I'd squash that onto a single 'else if' line.

::: js/src/ion/arm/Assembler-arm.h
@@ +1689,3 @@
>      // Sometimes, an api wants a uint32 (or a pointer to it) rather than
>      // an instruction.  raw() just coerces this into a pointer to a uint32
> +    uint32 *raw() const { return (uint32*)this; }

Actually, now I look, the raw() method violates pointer aliasing rules too. I think you could fix it by returning &data (and then no cast is required anyway).
Comment 6 Marty Rosenberg [:mjrosenb] 2012-03-16 16:25:14 PDT
landed: http://hg.mozilla.org/projects/ionmonkey/rev/af7981fd0ca4

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