Closed Bug 1092833 Opened 5 years ago Closed 5 years ago

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

Categories

(Core :: JavaScript Engine: JIT, defect, critical)

x86_64
macOS
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla36
Tracking Status
firefox35 + fixed
firefox36 + fixed

People

(Reporter: gkw, Assigned: shu)

References

(Blocks 1 open bug)

Details

(Keywords: assertion, regression, testcase, Whiteboard: [jsbugmon:update])

Attachments

(2 files, 1 obsolete file)

(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?
Flags: needinfo?(shu)
Attached file 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)
Just realised that GC is on the stack, so setting s-s and assuming sec-high as a start.
Group: core-security, javascript-core-security
Keywords: sec-high
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.
Attachment #8515666 - Flags: review?(terrence)
(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.
Flags: needinfo?(shu)
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.
Attachment #8515666 - Flags: review?(terrence) → review+
(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.
(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.
(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.
Group: core-security, javascript-core-security
Keywords: sec-high
Carrying r=terrence, applied comments.
Attachment #8515666 - Attachment is obsolete: true
Attachment #8517090 - Flags: review+
Attachment #8517090 - Flags: approval-mozilla-aurora?
Approval information same as bug 1089761.
(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.
Flags: needinfo?(shu)
Nope, it wasn't.
Assignee: nobody → shu
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/105570fa25f3
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
Attachment #8517090 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.