604297 - TM: don't allow non-conditions to be passed to TraceRecorder::guard()

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Nicholas Nethercote [inactive]]

Attachment: patch (against TM 55602:0b754642eedb)

TraceRecorder::guard() gets passed a condition, which is an LIns*.  Usually this is a boolean-esque value -- a comparison, 0, or 1.  But if it's not a boolean-esque value it's converted to one -- it gets equated with zero and the sense of the guard (ie. xt or xf) is inverted.

Bug 600127 improves optimization of comparisons, causing more of them to end up as 0 or 1.  It's causing me to hit a "guard always exits" assertion via emitIf().  The problem is that emitIf() checks if the condition is constant, and calls guard() if not.  But the condition may not be boolean-esque, in which case guard() will make it boolean-esque, and in that process the condition may end up constant, causing the assert.

This patch changes emitIf() to do the equate-with-zero before doing the is-constant check, thus avoiding the assertion.  I also changed guard() so it no longer accepts non-boolean-esque values as I always found this distasteful, and it'll make this problem less likely in the future;  this required changing a few other places where guard() is called.  (I checked every call to guard() to make sure I got them all.)

Comment 1

[Nicholas Nethercote [inactive]]

A shorter description:  emitIf() constructs a condition in two steps;  it handles the case where the result of the first step is a constant, but does not handle the case where the result of the second step is a constant.

Comment 2

[David Mandelin [:dmandelin]]

Comment on attachment 483095 [details] [diff] [review]
patch (against TM 55602:0b754642eedb)

r+. There is a tiny bit of code duplication:

>+    if (!isCond(x)) {
>+        cond = !cond;
>+        x = x->isI() ? lir->insEqI_0(x) : lir->insEqP_0(x);
>+    }

>+        if (!isCond(cond)) {
>+            expected = !expected;
>+            cond = cond->isI() ? lir->insEqI_0(cond) : lir->insEqP_0(cond);
>+        }

I assume you didn't want to return 2 values via outparams? I'd probably still
pull these into a function, but it's not a big deal, especially since there
are only 2.

Comment 3

[Nicholas Nethercote [inactive]]

Pushed with the duplicated code factored out into a function:

http://hg.mozilla.org/tracemonkey/rev/11b2f7a76d0f

Comment 4

[Nicholas Nethercote [inactive]]

Backed out due to suspected dromaeo perf regression (even though I'll be highly surprised and puzzled if it turns out to the be cause):

http://hg.mozilla.org/tracemonkey/rev/e9eb93193afc

Comment 5

[Nicholas Nethercote [inactive]]

I think I found the problem.  I put the call to ensureCond() at the start of emitIf().  This could invert the value of 'cond', which could cause an inversion of the value of 'pendingLoop', which would change how loop ends are generated.  I can imagine this might hurt some test cases badly.

I'll try pushing a new version that moves the ensureCond() call to near the end of emitIf(), which is where it's really needed.

http://hg.mozilla.org/tracemonkey/rev/51ca3657de5b

Comment 6

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/51ca3657de5b