TM: Can't use JSVAL_IS_BOOLEAN on trace since boolean type contains undefined.

VERIFIED FIXED in mozilla1.9.1b1

Status

()

Core
JavaScript Engine
P1
critical
VERIFIED FIXED
10 years ago
9 years ago

People

(Reporter: gal, Assigned: gal)

Tracking

({verified1.9.1})

unspecified
mozilla1.9.1b1
x86
Mac OS X
verified1.9.1
Points:
---
Bug Flags:
blocking1.9.1 +
in-testsuite +
in-litmus -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

Comment hidden (empty)
(Assignee)

Updated

10 years ago
Priority: -- → P1
(Assignee)

Comment 1

10 years ago
Created attachment 340684 [details] [diff] [review]
patch
Attachment #340684 - Flags: review?(brendan)
Comment on attachment 340684 [details] [diff] [review]
patch

>diff --git a/js/src/jstracer.cpp b/js/src/jstracer.cpp
>--- a/js/src/jstracer.cpp
>+++ b/js/src/jstracer.cpp
>@@ -65,16 +65,22 @@
> #include "jsopcode.h"
> #include "jsregexp.h"
> #include "jsscope.h"
> #include "jsscript.h"
> #include "jsdate.h"
> #include "jstracer.h"
> 
> #include "jsautooplen.h"        // generated headers last
>+
>+/* Never use JSVAL_IS_BOOLEAN because it restricts the value (true, false) and the type. What
>+   you want to use is JSVAL_TAG(x) == JSVAL_BOOLEAN and then handle the undefined case
>+   properly. */

..12345678901234567890123456789012345678901234567890123456789012345678901234567890

Cite bug on type-specializing undefined not as boolean and null not as object (if filed) here, also wrap before 80? Ragged right seems worth fixing no matter where you wrap. 

>-    } else if (JSVAL_IS_BOOLEAN(v)) {
>+    } else if (JSVAL_TAG(v) == JSVAL_BOOLEAN) {
>         guard(true,
>               addName(lir->ins2(LIR_eq, v_ins, lir->insImm(JSVAL_TO_BOOLEAN(v))),
>                       "guard(switch on boolean)"),
>               BRANCH_EXIT);
>     } else {
>         ABORT_TRACE("switch on object, null, or undefined");

Fix this ABORT_TRACE to say "... object or null".

>+    } else if ((JSVAL_TAG(l) == JSVAL_BOOLEAN) && (JSVAL_TAG(r) == JSVAL_BOOLEAN)) {
>         // The well-known values of JSVAL_TRUE and JSVAL_FALSE make this very easy.
>         // In particular: JSVAL_TO_BOOLEAN(0) < JSVAL_TO_BOOLEAN(1) so all of these comparisons do
>         // the right thing.
>         cond = evalCmp(op, l, r);
>+        // For ==, !=, ===, and !=== the result is magically correct even if undefined (2) is
>+        // involed. For the relational operations we need some additional cmov magic to make

"involved"

>+        // For boolean comparison we need a bit post processing to make the result false if

"bit of post-processing"

>+        // either side is undefined.
>+        if (op != LIR_eq && (JSVAL_TAG(l) == JSVAL_BOOLEAN) && (JSVAL_TAG(r) == JSVAL_BOOLEAN)) {

/me whines in vain against over-parenthesized == vs. && and kin.

Looks good otherwise, r=me with fixes.

/be
Attachment #340684 - Flags: review?(brendan) → review+
(Assignee)

Comment 3

10 years ago
http://hg.mozilla.org/tracemonkey/rev/3bdea835df9c
Status: NEW → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED

Updated

10 years ago
Flags: blocking1.9.1? → blocking1.9.1+
test also included in js1_8_1/trace/trace-test.js
Flags: in-testsuite+
Flags: in-litmus-
Keywords: fixed1.9.1
v 1.9.1, 1.9.2
Status: RESOLVED → VERIFIED
Keywords: fixed1.9.1 → verified1.9.1
You need to log in before you can comment on or make changes to this bug.