1092833 - Assertion failure: startOfUndefined <= nfixed, at jit/IonMacroAssembler.cpp

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

(function() {
    var a01
    var b02
    var c03
    var d04
    var e05
    var f06
    var g07
    var h08
    let i09
    var j10
    var k11
    var l12
    var m13
    function n14() {
        eval()
    }
})()

asserts js debug shell on m-c changeset 0e631971b841 with --ion-eager --no-threads at Assertion failure: startOfUndefined <= nfixed, at jit/IonMacroAssembler.cpp.

Debug configure options:

CC="clang -Qunused-arguments" CXX="clang++ -Qunused-arguments" AR=ar AUTOCONF=/usr/local/Cellar/autoconf213/2.13/bin/autoconf213 sh /Users/skywalker/trees/mozilla-central/js/src/configure --target=x86_64-apple-darwin12.5.0 --enable-debug --enable-optimize --enable-nspr-build --enable-more-deterministic --with-ccache --enable-gczeal --enable-debug-symbols --disable-tests

autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   https://hg.mozilla.org/mozilla-central/rev/0fd815999686
user:        Shu-yu Guo
date:        Wed Oct 29 19:41:42 2014 -0700
summary:     Bug 1089761 - Fix initializing lexicals to throw on touch on CallObject. (r=jandem,Waldo)

Shu-yu, is bug 1089761 a possible regressor?

Comment 1

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Attachment: stack

(lldb) bt 5
* thread #1: tid = 0x2cc601, 0x0000000100307b2e js-dbg-opt-64-dm-nsprBuild-darwin-0e631971b841`js::jit::MacroAssembler::initGCSlots(js::jit::Register, js::jit::Register, js::NativeObject*, bool) [inlined] JSObject::lastProperty(this=<unavailable>, clasp=<unavailable>) const + 28 at jsobj.h:129, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=1, address=0x0)
  * frame #0: 0x0000000100307b2e js-dbg-opt-64-dm-nsprBuild-darwin-0e631971b841`js::jit::MacroAssembler::initGCSlots(js::jit::Register, js::jit::Register, js::NativeObject*, bool) [inlined] JSObject::lastProperty(this=<unavailable>, clasp=<unavailable>) const + 28 at jsobj.h:129
    frame #1: 0x0000000100307b12 js-dbg-opt-64-dm-nsprBuild-darwin-0e631971b841`js::jit::MacroAssembler::initGCSlots(this=<unavailable>, templateObj=<unavailable>, initFixedSlots=<unavailable>, obj=<unavailable>, slots=<unavailable>) + 994 at IonMacroAssembler.cpp:996
    frame #2: 0x0000000100306d63 js-dbg-opt-64-dm-nsprBuild-darwin-0e631971b841`js::jit::MacroAssembler::initGCThing(this=0x000000010482b060, templateObj=0x0000000101f31100, initFixedSlots=true, obj=<unavailable>, slots=<unavailable>) + 403 at IonMacroAssembler.cpp:1074
    frame #3: 0x000000010025b4a8 js-dbg-opt-64-dm-nsprBuild-darwin-0e631971b841`js::jit::CodeGenerator::visitNewCallObject(this=0x000000010482b000, lir=<unavailable>) + 344 at CodeGenerator.cpp:4507
    frame #4: 0x0000000100259807 js-dbg-opt-64-dm-nsprBuild-darwin-0e631971b841`js::jit::CodeGenerator::generateBody(this=0x000000010482b000) + 935 at CodeGenerator.cpp:4108
(lldb)

Comment 2

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Just realised that GC is on the stack, so setting s-s and assuming sec-high as a start.

Comment 3

[Shu-yu Guo [:shu]]

Attachment: Deal with uninitialized slots in MacroAssembler::initGCSlots.

Basically, initGCSlots wasn't dealing with CallObjects on functions with
aliased lexicals correctly. Luckily, the way uninitialized lexicals appear in
the slot vector is also segmented, like undefineds.

Terrence, you did this optimization so flagging you
for review.

Comment 4

[Shu-yu Guo [:shu]]

(In reply to Gary Kwong [:gkw] [:nth10sd] GMT+8 after Oct 9 from comment #2)
> Just realised that GC is on the stack, so setting s-s and assuming sec-high
> as a start.

GC isn't on the stack, that's just the helper method name. I don't think this is s-s; ISTM both copySlotsFromTemplate and fillSlotsWithUndefined wouldn't do anything for values of startOfUndefined >= nfixed.

Comment 5

[Terrence Cole [:terrence]]

Comment on attachment 8515666 [details] [diff] [review]
Deal with uninitialized slots in MacroAssembler::initGCSlots.

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

Thanks!

::: js/src/jit/IonMacroAssembler.cpp
@@ +996,5 @@
>      MOZ_ASSERT(nslots > 0);
> +    uint32_t first = nslots;
> +    for (; first != 0; --first) {
> +        if (!IsUninitializedLexical(templateObj->getSlot(first - 1))) {
> +            *startOfUninitialized = first;

What is guaranteeing that startOfUninitialized will actually be initialized here? Do CallObject have a reserved slot?

@@ +1033,5 @@
> +    // The template object may be a CallObject, in which case we need to
> +    // account for uninitialized lexical slots as well as undefined
> +    // slots. Unitialized lexical slots always appear at the very end of
> +    // slots, after undefined.
> +    uint32_t startOfUndefined, startOfUninitialized;

Please initialize both of these to nslots to be on the safe side and to ensure changing compiler dataflow analysis doesn't intermittently cause warnings.

@@ +1064,4 @@
>          fillSlotsWithUndefined(Address(obj, 0), slots, 0, ndynamic);
> +
> +        // Fill uninitialized slots if necessary.
> +        if (startOfUninitialized >= nfixed) {

I think this branch and the prior one are equivalent to |if (start >= end) return;| at the top of FillSlotsWithConstantValue. Please move this branch there to clean up the duplicated and less straightforward branches here.

Comment 6

[Shu-yu Guo [:shu]]

(In reply to Terrence Cole [:terrence] from comment #5)
> Comment on attachment 8515666 [details] [diff] [review]
> Deal with uninitialized slots in MacroAssembler::initGCSlots.
> 
> Review of attachment 8515666 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thanks!
> 
> ::: js/src/jit/IonMacroAssembler.cpp
> @@ +996,5 @@
> >      MOZ_ASSERT(nslots > 0);
> > +    uint32_t first = nslots;
> > +    for (; first != 0; --first) {
> > +        if (!IsUninitializedLexical(templateObj->getSlot(first - 1))) {
> > +            *startOfUninitialized = first;
> 
> What is guaranteeing that startOfUninitialized will actually be initialized
> here? Do CallObject have a reserved slot?
> 
> @@ +1033,5 @@
> > +    // The template object may be a CallObject, in which case we need to
> > +    // account for uninitialized lexical slots as well as undefined
> > +    // slots. Unitialized lexical slots always appear at the very end of
> > +    // slots, after undefined.
> > +    uint32_t startOfUndefined, startOfUninitialized;
> 
> Please initialize both of these to nslots to be on the safe side and to
> ensure changing compiler dataflow analysis doesn't intermittently cause
> warnings.
> 
> @@ +1064,4 @@
> >          fillSlotsWithUndefined(Address(obj, 0), slots, 0, ndynamic);
> > +
> > +        // Fill uninitialized slots if necessary.
> > +        if (startOfUninitialized >= nfixed) {
> 
> I think this branch and the prior one are equivalent to |if (start >= end)
> return;| at the top of FillSlotsWithConstantValue. Please move this branch
> there to clean up the duplicated and less straightforward branches here.

The first branch of |startOfUninitialized < nfixed| is, but not this one. If we have |startOfUninitialized < nfixed| (which is certainly possible, if some of the fixed slots are lexicals), |start| is going to be |startOfUninitialized - nfixed|, which will overflow to something huge, and |start >= end| is going to be true inside FillSlotsWithConstantValue.

Comment 7

[Shu-yu Guo [:shu]]

(In reply to Shu-yu Guo [:shu] from comment #6)
> (In reply to Terrence Cole [:terrence] from comment #5)
> > Comment on attachment 8515666 [details] [diff] [review]
> > Deal with uninitialized slots in MacroAssembler::initGCSlots.
> > 
> > Review of attachment 8515666 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > Thanks!
> > 
> > ::: js/src/jit/IonMacroAssembler.cpp
> > @@ +996,5 @@
> > >      MOZ_ASSERT(nslots > 0);
> > > +    uint32_t first = nslots;
> > > +    for (; first != 0; --first) {
> > > +        if (!IsUninitializedLexical(templateObj->getSlot(first - 1))) {
> > > +            *startOfUninitialized = first;
> > 
> > What is guaranteeing that startOfUninitialized will actually be initialized
> > here? Do CallObject have a reserved slot?
> > 
> > @@ +1033,5 @@
> > > +    // The template object may be a CallObject, in which case we need to
> > > +    // account for uninitialized lexical slots as well as undefined
> > > +    // slots. Unitialized lexical slots always appear at the very end of
> > > +    // slots, after undefined.
> > > +    uint32_t startOfUndefined, startOfUninitialized;
> > 
> > Please initialize both of these to nslots to be on the safe side and to
> > ensure changing compiler dataflow analysis doesn't intermittently cause
> > warnings.
> > 
> > @@ +1064,4 @@
> > >          fillSlotsWithUndefined(Address(obj, 0), slots, 0, ndynamic);
> > > +
> > > +        // Fill uninitialized slots if necessary.
> > > +        if (startOfUninitialized >= nfixed) {
> > 
> > I think this branch and the prior one are equivalent to |if (start >= end)
> > return;| at the top of FillSlotsWithConstantValue. Please move this branch
> > there to clean up the duplicated and less straightforward branches here.
> 
> The first branch of |startOfUninitialized < nfixed| is, but not this one. If
> we have |startOfUninitialized < nfixed| (which is certainly possible, if
> some of the fixed slots are lexicals), |start| is going to be
> |startOfUninitialized - nfixed|, which will overflow to something huge, and
> |start >= end| is going to be true inside FillSlotsWithConstantValue.

Oh, I'm tired. I don't know why I typed out that explanation to say you're wrong, when in fact you're right.

Comment 8

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

(In reply to Shu-yu Guo [:shu] from comment #4)
> GC isn't on the stack, that's just the helper method name. I don't think
> this is s-s; ISTM both copySlotsFromTemplate and fillSlotsWithUndefined
> wouldn't do anything for values of startOfUndefined >= nfixed.

Ok, opening up then.

Comment 9

[Lukas Blakk [:lsblakk] use ?needinfo]

tracking to ensure this gets backported as per https://bugzilla.mozilla.org/show_bug.cgi?id=1089761#c13

Comment 10

[Shu-yu Guo [:shu]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5e6d8b6023e3

Comment 11

[Shu-yu Guo [:shu]]

Attachment: Deal with uninitialized slots in MacroAssembler::initGCSlots. (r=terrence)

Carrying r=terrence, applied comments.

Comment 12

[Shu-yu Guo [:shu]]

Approval information same as bug 1089761.

Comment 13

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/3c88f1fc617c under suspicion of ggc bustage:

https://treeherder.mozilla.org/ui/logviewer.html#?job_id=3601006&repo=mozilla-inbound

Comment 14

[Shu-yu Guo [:shu]]

(In reply to Wes Kocher (:KWierso) from comment #13)
> Backed out in
> https://hg.mozilla.org/integration/mozilla-inbound/rev/3c88f1fc617c under
> suspicion of ggc bustage:
> 
> https://treeherder.mozilla.org/ui/logviewer.
> html#?job_id=3601006&repo=mozilla-inbound

Pretty sure this is not my fault? https://treeherder.mozilla.org/ui/index.html#/jobs?repo=mozilla-inbound&revision=370927f1465a has that failure, and it's after your backout.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

Nope, it wasn't.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/105570fa25f3

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/105570fa25f3

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/7af882da53e7