743094 - IonMonkey: Assertion failure: safepoint.gcSpills().empty(), at js/src/ion/IonFrames.cpp:432

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

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


gczeal(2)
function test() {
  typeof (new test("1")) != 'function'
}
test();

Comment 1

[Jesse Ruderman]

I can't reproduce this on Mac.

Comment 2

[Christian Holler (:decoder)]

This seems to assert differently now. Before it was:

Assertion failure: allocated(), at ../../jsgc.h:498

Now it is: 

Assertion failure: safepoint.gcSpills().empty(), at js/src/ion/IonFrames.cpp:432


Stepping through crashes, with a very large stack:

(gdb) bt 8
#0  0x080eecc0 in js::gc::Arena::isAligned (thing=4151488464, thingSize=0) at /home/ownhero/homes/mozilla/repos/ionmonkey/js/src/jsgc.h:582
#1  0x0810a421 in js::gc::Cell::isAligned (this=0xf772afd0) at /home/ownhero/homes/mozilla/repos/ionmonkey/js/src/jsgc.h:895
#2  0x08112499 in js::gc::CheckMarkedThing<JSObject> (trc=0x86f8dc4, thing=0xf772afd0) at /home/ownhero/homes/mozilla/repos/ionmonkey/js/src/jsgcmark.cpp:90
#3  0x08110bc0 in js::gc::MarkInternal<JSObject> (trc=0x86f8dc4, thingp=0xfff8316c) at /home/ownhero/homes/mozilla/repos/ionmonkey/js/src/jsgcmark.cpp:104
#4  0x0810bb8e in js::gc::MarkKind (trc=0x86f8dc4, thingp=0xfff8316c, kind=JSTRACE_OBJECT) at /home/ownhero/homes/mozilla/repos/ionmonkey/js/src/jsgcmark.cpp:235
#5  0x0810c051 in js::gc::MarkValueInternal (trc=0x86f8dc4, v=0xf79cf0f8) at /home/ownhero/homes/mozilla/repos/ionmonkey/js/src/jsgcmark.cpp:334
#6  0x0810c271 in js::gc::MarkValueRootRange (trc=0x86f8dc4, len=2, vec=0xf79cf0f0, name=0x84f7f36 "vm_stack") at /home/ownhero/homes/mozilla/repos/ionmonkey/js/src/jsgcmark.cpp:372
#7  0x082637c4 in js::gc::MarkValueRootRange (trc=0x86f8dc4, begin=0xf79cf0f0, end=0xf79cf100, name=0x84f7f36 "vm_stack") at ../jsgcmark.h:124
(More stack frames follow...)
(gdb) x /i $pc
=> 0x80eecc0 <js::gc::Arena::isAligned(uintptr_t, size_t)+30>:  divl   0xc(%ebp)
(gdb) info reg ebp
ebp            0xfff83098       0xfff83098
(gdb) 


Stack is over 4000 frames big. Marking s-s because this involves GC and it's not clear if this is a recursion crash or something strange going on here.

Comment 3

[Nicolas B. Pierron [:nbp]]

Hydra buildfarm can reproduce this assertion with a debug shell on the following test cases:

    ./js --ion --ion-eager -n ./jit-test/tests/basic/bug716013.js
    ./js --ion --ion-eager -n ./jit-test/tests/basic/bug737384.js
    ./js --ion --ion-eager -n ./jit-test/tests/basic/bug744356.js
    ./js --ion --ion-eager -n ./jit-test/tests/ion/bug730152.js
    ./js --ion --ion-eager -n ./jit-test/tests/jaeger/recompile/bug674391.js

Comment 4

[Nicolas B. Pierron [:nbp]]

(In reply to Christian Holler (:decoder) from comment #2)
> This seems to assert differently now. Before it was:
> 
> Assertion failure: allocated(), at ../../jsgc.h:498
> 

pierron> can we have this assertion if we forget to mark something ?
terrence> that is possible if you did not mark something and the arena became empty, causing the gc to set the arena as not allocated at the end of gc (presuming it swept everything in it)

My current guess is that we are not marking spilled registers and the assertion has been added to highlight the fact that this was not yet implemented in IonMonkey.

Comment 5

[Christian Holler (:decoder)]

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

Comment 6

[Nicolas B. Pierron [:nbp]]

Attachment: Mark GC registers, Fix machine state of OOL calls.

Comment 7

[David Anderson [:dvander] - inactive, e-mail if emergency]

Comment on attachment 617694 [details] [diff] [review]
Mark GC registers, Fix machine state of OOL calls.

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

::: js/src/ion/IonFrames.cpp
@@ +60,5 @@
> +pushRegsBase(uintptr_t *base, GeneralRegisterSet set)
> +{
> +    // ARM is forcing us to have aligned pushes. (see MacroAssembler::PushRegsInMask).
> +    size_t stackSpace = set.size() * STACK_SLOT_SIZE;
> +    return base - (stackSpace + 7 % 8) / sizeof(uintptr_t *);

This code, as well as RegisterSet::size(), strikes me as more complicated than it might need to be. Could we just implement a reverse set iterator?

@@ +252,5 @@
>      return *this;
>  }
>  
> +uintptr_t *
> +IonFrameIterator::spillBase() const

This should assert the frame type is IonFrame_JS

::: js/src/ion/RegisterSets.h
@@ +390,5 @@
> +        uint32 sum4  = (sum2  & 0x33333333) + ((sum2  & 0xcccccccc) >> 2);
> +        uint32 sum8  = (sum4  & 0x0f0f0f0f) + ((sum4  & 0xf0f0f0f0) >> 4);
> +        uint32 sum16 = (sum8  & 0x00ff00ff) + ((sum8  & 0xff00ff00) >> 8);
> +        return sum16;
> +    }

Why do we need this complicated code?

::: js/src/ion/Safepoints.h
@@ +114,5 @@
> +    GeneralRegisterSet restSpills() const {
> +        GeneralRegisterSet wrapper = GeneralRegisterSet(Registers::WrapperMask);
> +        GeneralRegisterSet rest =
> +            GeneralRegisterSet::Intersect(GeneralRegisterSet::Intersect(wrapper, allSpills()),
> +                                          GeneralRegisterSet::Not(gcSpills()));

What is this code doing? Why doesn't allSpills() suffice?

Comment 8

[David Anderson [:dvander] - inactive, e-mail if emergency]

Comment on attachment 617694 [details] [diff] [review]
Mark GC registers, Fix machine state of OOL calls.

>-    gcSpills_ = GeneralRegisterSet(stream_.readUnsigned());
>-    if (gcSpills_.empty())
>-        allSpills_ = gcSpills_;
>+    allSpills_ = GeneralRegisterSet(stream_.readUnsigned());
>+    if (allSpills_.empty())
>+        gcSpills_ = allSpills_;
>     else
>-        allSpills_ = GeneralRegisterSet(stream_.readUnsigned());
>+        gcSpills_ = GeneralRegisterSet(stream_.readUnsigned());

Wow - I have no idea how any of this worked in the first place. Nice catch. Patch looks good but I'd prefer a simpler way to read the spill slots.

Comment 9

[Nicolas B. Pierron [:nbp]]

(In reply to David Anderson [:dvander] from comment #7)
> Comment on attachment 617694 [details] [diff] [review]
> Mark GC registers, Fix machine state of OOL calls.
> 
> Review of attachment 617694 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/ion/IonFrames.cpp
> @@ +60,5 @@
> > +pushRegsBase(uintptr_t *base, GeneralRegisterSet set)
> > +{
> > +    // ARM is forcing us to have aligned pushes. (see MacroAssembler::PushRegsInMask).
> > +    size_t stackSpace = set.size() * STACK_SLOT_SIZE;
> > +    return base - (stackSpace + 7 % 8) / sizeof(uintptr_t *);
> 
> This code, as well as RegisterSet::size(), strikes me as more complicated
> than it might need to be. Could we just implement a reverse set iterator?

I don't see how a reverse iterator could help here.  The problem solved by these lines is that ARM need an aligned stack and do

size_t new_diff = (diff + 7) & ~7;
reserveStack(new_diff);
write_offset = new_diff - diff; /* renamed for this example */

> ::: js/src/ion/RegisterSets.h
> @@ +390,5 @@
> > +        uint32 sum4  = (sum2  & 0x33333333) + ((sum2  & 0xcccccccc) >> 2);
> > +        uint32 sum8  = (sum4  & 0x0f0f0f0f) + ((sum4  & 0xf0f0f0f0) >> 4);
> > +        uint32 sum16 = (sum8  & 0x00ff00ff) + ((sum8  & 0xff00ff00) >> 8);
> > +        return sum16;
> > +    }
> 
> Why do we need this complicated code?

Because it is faster and easy to read once you understand that sumX means sum of X/2 bits numbers which results in number of X bits.

> ::: js/src/ion/Safepoints.h
> @@ +114,5 @@
> > +    GeneralRegisterSet restSpills() const {
> > +        GeneralRegisterSet wrapper = GeneralRegisterSet(Registers::WrapperMask);
> > +        GeneralRegisterSet rest =
> > +            GeneralRegisterSet::Intersect(GeneralRegisterSet::Intersect(wrapper, allSpills()),
> > +                                          GeneralRegisterSet::Not(gcSpills()));
> 
> What is this code doing? Why doesn't allSpills() suffice?

allSpills is a superset of gcSpills, which contains all live registers -- not spills.  the remaining spilled registers are registers which are erase by the wrapper mask and which are not yet spilled with the gcSpills register set.


Out-Of-Line paths are only saving the necessary set of registers, which correspond to gcSpills and restSpills.  allSpills should be renamed to allLiveRegs.

> Wow - I have no idea how any of this worked in the first place. Nice catch.
> Patch looks good but I'd prefer a simpler way to read the spill slots.

Based on how we are reading the spilled registers, I don't think saveLive need to separate the 2 sets of registers as it was designed with cdleary.  It might be possible to use only one pushRegs in saveLive and then having only one loop in which we filter registers parts of the gcSpills, almost as it was done previously.

Comment 10

[David Anderson [:dvander] - inactive, e-mail if emergency]

Comment on attachment 617694 [details] [diff] [review]
Mark GC registers, Fix machine state of OOL calls.

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

Simple is better here, especially since we're solving hard problems to begin with. Safepoint registers should always be immediately after the preset, reserved frame size, so ARM should not need any adjustment.

Looking at the existing code, I don't understand why PushRegsInMask performs any alignment. It's only used before calls which always perform their own stack alignment. I'd check with Marty to see why that's there (hg blame says bug 714949). If we absolutely need the alignment, we could just make Push/PopRegsInMask store before the alignment rather than after. Then we wouldn't need to compute the alignment later.

Comment 11

[Nicolas B. Pierron [:nbp]]

(In reply to David Anderson [:dvander] from comment #10)
> Comment on attachment 617694 [details] [diff] [review]
> Mark GC registers, Fix machine state of OOL calls.
> 
> Review of attachment 617694 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Simple is better here, especially since we're solving hard problems to begin
> with. Safepoint registers should always be immediately after the preset,
> reserved frame size, so ARM should not need any adjustment.
> 
> Looking at the existing code, I don't understand why PushRegsInMask performs
> any alignment. It's only used before calls which always perform their own
> stack alignment. I'd check with Marty to see why that's there (hg blame says
> bug 714949). If we absolutely need the alignment, we could just make
> Push/PopRegsInMask store before the alignment rather than after. Then we
> wouldn't need to compute the alignment later.

I discuss with marty about this issue, what is possible is to move the optional padding between the GPRs and FPUs registers.  FPUs registers need to be aligned to save cycles and the stack is expected to be aligned at any time.  As we do not care about FPUs during marking, the marking phase won't have to handle the padding at all.  
The padding between GPRs and FPUs will matter when we will try to recover the machine state for the debugger or f.arguments.  Currently we are unable to recover doubles stored in volatile registers after an out-of-line call.

Another option would be to move the FPUs before the GPRs and put the padding at the end, but in such case we need to inform the marking that we need to skip the FPUs.

Comment 12

[Nicolas B. Pierron [:nbp]]

Nothing to hide …

The reason that this bug is related to GC is not a concerns as this bug is only dealing with the lack of marking in IonMonkey code, which by the way is a known issue.

Comment 13

[Nicolas B. Pierron [:nbp]]

Attachment: Remove stack alignments of OOL VM calls.

This remove the alignment *issue* reported as a blocked of the validation of the previous patch.

dvander: Please r+ the other patch if you r+ this one and have no more comments on the other patch from which the pushRegsBase function has been removed.

Comment 14

[David Anderson [:dvander] - inactive, e-mail if emergency]

Comment on attachment 622903 [details] [diff] [review]
Remove stack alignments of OOL VM calls.

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

Thanks for fixing this.

Comment 15

[David Anderson [:dvander] - inactive, e-mail if emergency]

Comment on attachment 617694 [details] [diff] [review]
Mark GC registers, Fix machine state of OOL calls.

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

r=me with a better name for "restSpills"

Comment 16

[Marty Rosenberg [:mjrosenb]]

Comment on attachment 622903 [details] [diff] [review]
Remove stack alignments of OOL VM calls.

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

::: js/src/ion/arm/Trampoline-arm.cpp
@@ +573,4 @@
>      if (f.explicitArgs) {
>          argsBase = r5;
>          regs.take(argsBase);
> +        masm.ma_add(sp, Imm32(IonExitFrameLayout::SizeWithFooter()), argsBase);

This has certainly been an active line for the past couple of days

Comment 17

[Nicolas B. Pierron [:nbp]]

https://hg.mozilla.org/projects/ionmonkey/rev/33d9ffa2b48c
https://hg.mozilla.org/projects/ionmonkey/rev/aa9c962d4a35
https://hg.mozilla.org/projects/ionmonkey/rev/8f467ac2e650 (fix merge issue)

Comment 18

[Christian Holler (:decoder)]

Automatically extracted testcase for this bug was committed:

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