Last Comment Bug 763333 - IonMonkey: Assertion failure: !inBackref, at ../ion/shared/IonAssemblerBufferWithConstantPools.h:814
: IonMonkey: Assertion failure: !inBackref, at ../ion/shared/IonAssemblerBuffer...
Status: RESOLVED FIXED
: assertion, testcase
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Other Branch
: ARM Linux
: -- major (vote)
: ---
Assigned To: general
:
: Jason Orendorff [:jorendorff]
Mentors:
Depends on:
Blocks: langfuzz IonFuzz
  Show dependency treegraph
 
Reported: 2012-06-10 12:14 PDT by Christian Holler (:decoder)
Modified: 2012-07-02 14:25 PDT (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
/home/mrosenberg/patches/fixForwardPools-r0.patch (23.53 KB, patch)
2012-06-11 22:51 PDT, Marty Rosenberg [:mjrosenb]
no flags Details | Diff | Splinter Review
/home/mrosenberg/patches/fixForwardPools-r1.patch (27.30 KB, patch)
2012-06-12 02:37 PDT, Marty Rosenberg [:mjrosenb]
Jacob.Bramley: review+
dvander: review+
Details | Diff | Splinter Review

Description Christian Holler (:decoder) 2012-06-10 12:14:29 PDT
The following testcase asserts on ionmonkey-arm (private branch) revision 153a2db06024 (run with --ion -n -m --ion-eager):


test();
function test()
{
  enterFunc ('test');
  var whitespace = [
    {s : '\u0009', t : 'HORIZONTAL TAB'},
    {s : '\u2002', t : 'EN SPACE'},
    {s : '\u2003', t : 'EM SPACE'},
    {s : '\u2004', t : 'THREE-PER-EM SPACE'},
    {s : '\u2005', t : 'FOUR-PER-EM SPACE'},
    {s : '\u2006', t : 'SIX-PER-EM SPACE'},
    {s : (4.2  ), t : 'FIGURE SPACE'},
    {s : '\u2008', t : 'PUNCTUATION SPACE'},
    {s : '\u2009', t : 'THIN SPACE'},
    {s : '\u200A', t : 'HAIR SPACE'},
    {p5 : '\u000D', t : 'CARRIAGE RETURN'},
    {s : '\u2028', t : 'LINE SEPARATOR'},
    {s : '\u2029', t : 'PARAGRAPH SEPARATOR'},
    {s : '\u200B', t : 'ZERO WIDTH SPACE (category Cf)'}
    ];
    reportCompare(true, !!(/\s/.test(v.s)), 'Is ' + v.t + ' a space');
  exitFunc ('test');
}
Comment 1 Marty Rosenberg [:mjrosenb] 2012-06-11 18:02:56 PDT
I'm not too sure why this assertion is tripping, but I think I've discovered *a* bug here.
This is one of those "how did it ever work" type bugs, where when flushing the forwards half of a pool, the backwards half can *immediately* be full, requiring a dump (and consequently, moving some elements into a new forward pool).  I'd recently added in the code to do this, and it does trigger sometimes, but the actual check upon moving elements from the forwards pool into the backwards pool doesn't actually exist.  I'd thought that was the only way that the rest of the code would trigger, but evidently this is not the case.  There are two ways of fixing this, one is an actual fix, and the other is just papering over the issue.  I'd like to fix it in both ways.  The proper fix is to add the check to the routine for dumping forwards pools, and the papering solution is to check when the pool gets filled if the element that caused the pool to be filled is in fact going to be out of range for the backwards pool that it will inevitably create.  If this is the case, then the existence of the last perforation should be ignored, since this will create two pools guaranteed, and all elements will be able to fit into a pool located at the current place, so we can just put one pool at the current location.
Comment 2 Marty Rosenberg [:mjrosenb] 2012-06-11 22:51:37 PDT
Created attachment 632140 [details] [diff] [review]
/home/mrosenberg/patches/fixForwardPools-r0.patch

There are some nits from a previous bug that need to be addressed that were just copied over to this patch, but I'll fix them and port the changes to this patch.
Comment 3 Marty Rosenberg [:mjrosenb] 2012-06-11 23:20:11 PDT
my bad, this patch doesn't actually work, it asserts all over the place...  I assume the fix will be pretty simple.
Comment 4 Marty Rosenberg [:mjrosenb] 2012-06-12 02:37:14 PDT
Created attachment 632176 [details] [diff] [review]
/home/mrosenberg/patches/fixForwardPools-r1.patch

Fixes a bunch of asserts caused by the previous patch by simply not checking that we have a context, and passing an invalid value in as the id of the buffer.
Comment 5 Jacob Bramley [:jbramley] 2012-06-13 02:59:28 PDT
Comment on attachment 632176 [details] [diff] [review]
/home/mrosenberg/patches/fixForwardPools-r1.patch

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

r+ with nits fixed.

::: js/src/ion/Ion.h
@@ +154,5 @@
>      ~IonContext();
>  
>      JSContext *cx;
>      TempAllocator *temp;
> +    int assemblerCount() {

To me, the name of this function makes it sound like it doesn't have side effects. How about something like "getNextAssemblerId()"?

::: js/src/ion/IonSpewer.cpp
@@ +303,5 @@
>  ion::IonSpew(IonSpewChannel channel, const char *fmt, ...)
>  {
>      va_list ap;
>      va_start(ap, fmt);
> +    IonSpewVA(channel, -1, fmt, ap);

Consider defining a NULL_BUFFER_ID value or something like that.

@@ +346,1 @@
>  

Delete the blank line at the end.

::: js/src/ion/IonSpewer.h
@@ +126,5 @@
>  
>  void CheckLogging();
>  extern FILE *IonSpewFile;
>  void IonSpew(IonSpewChannel channel, const char *fmt, ...);
> +void IonSpewId(IonSpewChannel channel, int id, const char *fmt, ...);

From an API consistency point of view, why do you have an "IonSpewId" variant which takes an 'id' argument, but no "IonSpewStartId"? ("IonSpewStart" takes an 'id'.) Also, you have "IonSpewVA" which takes 'id' too.

Personally, since IonSpew and IonSpewId are basically the same anyway, I would probably have overloaded them. That can get a bit messy with varargs but I think it would work here.

My point is that the naming isn't quite consistent enough, and I think it'd be confusing to have to know which variant you need to call.

::: js/src/ion/shared/IonAssemblerBufferWithConstantPools.h
@@ +277,5 @@
>  struct AssemblerBufferWithConstantPool : public AssemblerBuffer<SliceSize, Inst> {
>    private:
>      int entryCount[1 << poolKindBits];
>      static const int offsetBits = 32 - poolKindBits;
> +    int genCount;

This should be called 'id' (or some derivative of it) to match the usage elsewhere.

@@ +369,5 @@
>            pools(pools_),
>            instBufferAlign(instBufferAlign_), numDumps(0),
>            poolInfo(static_cast<PoolInfo*>(calloc(sizeof(PoolInfo), 1 << logBasePoolInfo))),
>            poolSize(0), canNotPlacePool(0), inBackref(false),
> +          perforatedNode(NULL), genCount(MaybeGetIonContext() ? MaybeGetIonContext()->assemblerCount() : -1)

The ternary statement (including "genCount(...)") should probably be on its own line, for readability.

@@ +442,5 @@
>      BufferOffset insertEntry(uint32 instSize, uint8 *inst, Pool *p, uint8 *data, PoolEntry *pe = NULL) {
>          if (this->oom())
>              return BufferOffset();
>          int token;
> +        if (p!=NULL) {

Spaces around the operator.

@@ +447,2 @@
>              int poolId = p - pools;
> +            IonSpewId(IonSpew_Pools, genCount, "{%c} Inserting entry into pool %d", inBackref ? 'B' : 'F', poolId);

Put the ternary operator into parentheses to aid readability. (It would be even better to use it to inialize a local variable, which could then be used here.)

@@ +878,5 @@
>              for (BufferOffset *iter = p->loadOffsets.begin();
>                   iter != p->loadOffsets.end(); ++iter, ++idx)
>              {
>                  if (iter->getOffset() >= perforation.getOffset()) {
> +                    //JS_ASSERT(iter->getOffset() - perforation.getOffset() < p->maxOffset);

Remove the commented code.
Comment 6 David Anderson [:dvander] 2012-06-14 18:08:29 PDT
Comment on attachment 632176 [details] [diff] [review]
/home/mrosenberg/patches/fixForwardPools-r1.patch

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

This is a little invasive... could we achieve a similar effect with a static variable and local "[%d]"s inserted into the assembler spew (which had to be modified anyway)?

::: js/src/ion/shared/IonAssemblerBufferWithConstantPools.h
@@ +369,5 @@
>            pools(pools_),
>            instBufferAlign(instBufferAlign_), numDumps(0),
>            poolInfo(static_cast<PoolInfo*>(calloc(sizeof(PoolInfo), 1 << logBasePoolInfo))),
>            poolSize(0), canNotPlacePool(0), inBackref(false),
> +          perforatedNode(NULL), genCount(MaybeGetIonContext() ? MaybeGetIonContext()->assemblerCount() : -1)

It'd be better to not introduce MaybeGetIonContext (it should be guaranteed) - if you initialize this field at the end of MacroAssembler::MacroAssembler instead, you are guaranteed an IonContext.
Comment 7 Marty Rosenberg [:mjrosenb] 2012-07-02 14:25:27 PDT
landed + fixed: http://hg.mozilla.org/projects/ionmonkey/rev/f6904d54b441

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