677073 - IonMonkey: Double box generated during SSA construction

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Andrew Drake [:adrake]]

Attachment: Test case

In the attached test case, we end up with a phi with two inputs: one is a constant 0, the other is a copy of the parameter (not the unboxed data). The output of this phi is then boxed and returned, resulting in an incorrect function result (specifically, an integer value -127) on 32-bit.

Comment 1

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

Attachment: fix

Recent regression, which also caused snapshots to not get specialized inputs. I've added an assert to catch this for phis.

Comment 2

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

Attachment: better fix

Comment 3

[Andrew Drake [:adrake]]

Attachment: Bad snapshot test case

With the current patch applied, snapshots can be produced with invalid virtual register uses (i.e. IDs larger than the number of virtual registers). This test case caused a crash in the linear scan allocator due to an out-of-bounds access into the virtual register list (just checked in http://hg.mozilla.org/projects/ionmonkey/rev/4ea28afe22cf which turned that into an assert).

Comment 4

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

Attachment: fix v3

Argh. Nice find. I'm not totally pleased with this fix but it's not too horrible.

First, the hack around parameters is gone, parameters now just get the entry snapshots.

Second, there's a hack for Phis instead. We could prevent this by giving phis snapshots, but that doesn't make a whole lot of sense, so I don't mind the two-line check.

Comment 5

[Ryan Pearl [:rpearl]]

Comment on attachment 551377 [details] [diff] [review]
fix v3

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

::: js/src/ion/IonAnalysis.cpp
@@ +357,5 @@
> +                return false;
> +        } else if (box->isPhi()) {
> +            if (box->block()->entrySnapshot() == snapshot)
> +                return false;
> +        }

In terms of personal preference, I feel like it would be cleaner to fold all the returns together, something like:

MSnapshot *defSnapshot;
if (box->isInstruction())
    defSnapshot = box->toInstruction()->snapshot();
else if (box->isPhi())
    defSnapshot = box->block()->entrySnapshot();

return (snapshot == defSnapshot);

I don't know whether that's house style though.

@@ +367,5 @@
> +    MDefinition *def = use->toDefinition();
> +
> +    // Only replace nodes that have a type policy. Otherwise, we would
> +    // replace an unbox into its own input.
> +    if (def->typePolicy())

phis don't have a typePolicy, but if someone unfamiliar with that reads this bit of code, it's a little unclear that phis will always fall through to the |if (def->isPhi()...| line. Maybe move lines 3750-376 above line 368.

@@ +370,5 @@
> +    // replace an unbox into its own input.
> +    if (def->typePolicy())
> +        return true;
> +
> +    // Except for phis, which if are specialized need specialized inputs.

typo here: "if are specialized" => "if specialized"

::: js/src/jit-test/tests/ion/bug677073-2.js
@@ +6,5 @@
> +        v0 = v1 + v1;
> +    }
> +    return v0;
> +}
> +print(f0());

Comment expected output of the test, or at least "don't assert"?

::: js/src/jit-test/tests/ion/bug677073.js
@@ +4,5 @@
> +		x = p0;
> +	}
> +	return x;
> +}
> +print(a(1));

Likewise, comment expected output.

Comment 6

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

(In reply to Ryan Pearl [:rpearl] from comment #5)
> MSnapshot *defSnapshot;
> if (box->isInstruction())
>     defSnapshot = box->toInstruction()->snapshot();
> else if (box->isPhi())
>     defSnapshot = box->block()->entrySnapshot();
> 
> return (snapshot == defSnapshot);
> 
> I don't know whether that's house style though.

I like this better, too.

http://hg.mozilla.org/projects/ionmonkey/rev/aad4f90fd9b5