[IonGraph] crash with Assertion failure: block_, at /inbound/js/src/jit/MIR.h:548

RESOLVED FIXED in mozilla36

Status

()

Core
JavaScript Engine: JIT
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: bbouvier, Assigned: bbouvier)

Tracking

Trunk
mozilla36
x86_64
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

3 years ago
function a(b,c) {
    if (b!=c) throw new Error;
}

var b=new Int32Array(16);
for(var i=2000;i--;)
    a(b instanceof Int32Array, true);


Run with IONFLAGS=logs $js --ion-offthread-compile=off. Contemplate assertion failure.
I'll work on it unless somebody steals it beforehand.
(Assignee)

Comment 1

3 years ago
Created attachment 8512550 [details] [diff] [review]
Only assert in MDefinition::id() if the instruction isn't emitted at uses

So the "true" in this test case flows into an int32. In
LIRGenerator::visitToInt32, as we can, we just replace the boolean true by the
constant int32 1, which is emitted at uses. When doing such a replacement, we
don't update the block_ field of the newer instruction, so it stays null. In
the spewing phase, we call id() on the newer instruction, which has no blocks,
and thus hits the assertion.

Another fix would be to update the block_ field of the newer instruction just
after creatin it, but it seems it would be strictly more work in all kind of
builds (not only debug builds) and issues would have been found by the fuzzers
if it ever was a problem, so this fix looked fine. Thoughts?
Attachment #8512550 - Flags: review?(nicolas.b.pierron)
(Assignee)

Updated

3 years ago
Assignee: nobody → benj
Status: NEW → ASSIGNED
(In reply to Benjamin Bouvier [:bbouvier] from comment #1)
> Created attachment 8512550 [details] [diff] [review]
> Only assert in MDefinition::id() if the instruction isn't emitted at uses
> 
> So the "true" in this test case flows into an int32. In
> LIRGenerator::visitToInt32, as we can, we just replace the boolean true by
> the
> constant int32 1, which is emitted at uses. When doing such a replacement, we
> don't update the block_ field of the newer instruction, so it stays null. In
> the spewing phase, we call id() on the newer instruction, which has no
> blocks,
> and thus hits the assertion.
> 
> Another fix would be to update the block_ field of the newer instruction just
> after creatin it, but it seems it would be strictly more work in all kind of
> builds (not only debug builds) and issues would have been found by the
> fuzzers
> if it ever was a problem, so this fix looked fine. Thoughts?

The reason we assert for the basic block in the function id(), is that the basic block is used to assign the id_ of the instruction.  Thus, we are probably loading some grambled value read out of the heap, which is then likely to show up in valgrind as uninitialized value.

Also, for sanity, we should probably do an insertBefore into the block of the definition from which we are replacing all uses.  In fact we should probably assert in replace all uses, that the instruction got added to the graph before replacing any uses.

Making the code sane is a good thing, it avoid thinking about things that we forget about.  Thus, it avoid making wrong assumption which are later leading to security issues ;)  I do not think that adding an instruction to a basic block are that costly to be noticeable.
Comment on attachment 8512550 [details] [diff] [review]
Only assert in MDefinition::id() if the instruction isn't emitted at uses

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

The instruction being added by the basic block, I do not think this is the right approach to take here.
Unless we are setting the id_ when we set the flag EmittedAtUses.  In which case this would better be documented.
Attachment #8512550 - Flags: review?(nicolas.b.pierron)
(Assignee)

Comment 4

3 years ago
Created attachment 8514967 [details] [diff] [review]
Emitted at uses instructions should be added to the MIR graph in redefine

This implements what you've suggested.
Attachment #8514967 - Flags: review?(nicolas.b.pierron)
(Assignee)

Updated

3 years ago
Attachment #8512550 - Attachment is obsolete: true
Attachment #8514967 - Flags: review?(nicolas.b.pierron) → review+
https://hg.mozilla.org/mozilla-central/rev/9bddfe99b757
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
You need to log in before you can comment on or make changes to this bug.