644210 - Want APIs for Uint Manipulations

Status: RESOLVED WONTFIX

(Core :: JavaScript Engine, defect)

Description

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

There are a set of APIs for Ints, but the corresponding support for Uints is incomplete.

UINT_TO_JSVAL and JS_ValueToECMAUint32 exist, but that's about the extent of it.

For the V8 API, Value::IsInt32 corresponds nicely to JSVAL_IS_INT, but there's nothing so nice for Value::IsUint32 (JSVAL_IS_UINT doesn't exists - understandably since that gets stored as a double internally, bit it would be nice). Instead we are explicitly checking IS_NUMBER && >0 && (IS_INT || < UINT_MAX && floor == ceil) (https://github.com/zpao/v8monkey/blob/722e527e3b0873a7dee69e9a69eb4b9dffbace34/js/src/v8api/v8.cpp#L143-150)

That's the only one that was annoying so far, but I think there are other cases where there aren't matching APIs.

Comment 1

[David Mandelin [:dmandelin]]

Random thought: (In reply to comment #0)
> There are a set of APIs for Ints, but the corresponding support for Uints is
> incomplete.
> 
> UINT_TO_JSVAL and JS_ValueToECMAUint32 exist, but that's about the extent of
> it.
> 
> For the V8 API, Value::IsInt32 corresponds nicely to JSVAL_IS_INT, 

I don't think that's right. 

  Value::IsInt32 returns true iff the value can be represented as a 32-bit 
                                  signed integer

  JSVAL_IS_INT   returns true iff the value's actual representation is a 32-bit
                                  signed integer (i.e., the type tag is int)

> but there's nothing so nice for Value::IsUint32 

I think we need 2 new APIs here. But I have another random thought. Should these be C-style APIs like what we have, or should we add C++ methods to Value so that you can write |Valueify(v).isInt32)?

Comment 2

[Luke Wagner [:luke]]

(In reply to comment #1)
> I think we need 2 new APIs here. But I have another random thought. Should
> these be C-style APIs like what we have, or should we add C++ methods to Value
> so that you can write |Valueify(v).isInt32)?

You can write Valueify(v).isInt32().

Or do you mean isInt32 in the V8 "can be represented as" meaning?  For that purpose, it seems like we would want to define non-member queries/operations analogous to js_ValueToECMAUint32 where we carefully specify what exactly we are doing to derive said uint (which conversions will be used, how do they relate to the standard, etc).

Comment 3

[Shawn Wilsher :sdwilsh]

(In reply to comment #2)
> You can write Valueify(v).isInt32().
This is the first I've seen Value and Valueify.  Are these public APIs?

> Or do you mean isInt32 in the V8 "can be represented as" meaning?  For that
> purpose, it seems like we would want to define non-member queries/operations
> analogous to js_ValueToECMAUint32 where we carefully specify what exactly we
> are doing to derive said uint (which conversions will be used, how do they
> relate to the standard, etc).
v8 has a type that is designed to represent a Uint32.  You can construct it with a Uint32 value.  Currently, we store this as an int in the jsval if it fits, or store it as a double if it is too big.

Comment 4

[Luke Wagner [:luke]]

(In reply to comment #3)
> (In reply to comment #2)
> > You can write Valueify(v).isInt32().
> This is the first I've seen Value and Valueify.  Are these public APIs?

At the moment its private (it #include's jsprvtd.h, its in namespace 'js'), but that was more of a fastest-route-to-fatvals solution.  It could be made public, but, really, its just C++ syntactic sugar for jsapi.h value manipulation.

> > Or do you mean isInt32 in the V8 "can be represented as" meaning?  For that
> > purpose, it seems like we would want to define non-member queries/operations
> > analogous to js_ValueToECMAUint32 where we carefully specify what exactly we
> > are doing to derive said uint (which conversions will be used, how do they
> > relate to the standard, etc).
> v8 has a type that is designed to represent a Uint32.  You can construct it
> with a Uint32 value.  Currently, we store this as an int in the jsval if it
> fits, or store it as a double if it is too big.

Adding Uint32 to the fundamental value representation would be a huge deal, so your int32/double strategy seems reasonable.  The only scary thing is that this predicate would not be able to distinguish uint32's set via the v8-api that were converted to doubles from doubles that just happened to be whole numbers in the uint32 range.  From a JS semantic pov, this shouldn't matter, but one could imagine pathological v8-API usage...

So it seems like what is missing from the JSAPI is a JSDOUBLE_IS_UINT32 which would let you to define your own predicate:

 IsUint32(v) === JSVAL_IS_INT(v) && JSVAL_TO_INT(v) >= 0 ||
                 JSVAL_IS_DOUBLE(v) && JSDOUBLE_IS_UINT32(JSVAL_TO_DOUBLE(v))

Comment 5

[Shawn Wilsher :sdwilsh]

(In reply to comment #4)
>  IsUint32(v) === JSVAL_IS_INT(v) && JSVAL_TO_INT(v) >= 0 ||
>                  JSVAL_IS_DOUBLE(v) && JSDOUBLE_IS_UINT32(JSVAL_TO_DOUBLE(v))
I believe so, but I'll let zpao comment since he's been looking at this a lot more closely than I have been.

Comment 6

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

(In reply to comment #1)
> > For the V8 API, Value::IsInt32 corresponds nicely to JSVAL_IS_INT, 
> 
> I don't think that's right. 
> 
>   Value::IsInt32 returns true iff the value can be represented as a 32-bit 
>                                   signed integer
> 
>   JSVAL_IS_INT   returns true iff the value's actual representation is a 32-bit
>                                   signed integer (i.e., the type tag is int)

Are you sure? I know the V8 documentation isn't great, but I got the impression that v8::Value::IsInt32 returned true iff the value was a 32-bit signed int. The API tests don't actually make that clear though since they only seem to test with numbers.

> I think we need 2 new APIs here. But I have another random thought. Should
> these be C-style APIs like what we have, or should we add C++ methods to Value
> so that you can write |Valueify(v).isInt32)?

Honestly, I think we're managing fine with the C-style APIs, but if you're looking for a future generation of APIs to expose, the C++ stuff would be much nicer.

(In reply to comment #5)
> (In reply to comment #4)
> >  IsUint32(v) === JSVAL_IS_INT(v) && JSVAL_TO_INT(v) >= 0 ||
> >                  JSVAL_IS_DOUBLE(v) && JSDOUBLE_IS_UINT32(JSVAL_TO_DOUBLE(v))
> I believe so, but I'll let zpao comment since he's been looking at this a lot
> more closely than I have been.

Yea, I think that should work better here (I'm assuming JS_NumberValue for false would be 0 which like I mentioned, I don't think we want). It certainly makes the whole situation much more manageable (and indeed allows us to define our own specific conditions), but as somebody completely new to jsapi/v8, it still feels a bit awkward.

Comment 7

[Luke Wagner [:luke]]

JSDOUBLE_IS_UINT32 would just be a simple modification of JSDOUBLE_IS_INT32, which is in js/src/jsvalue.h.  Unfortunately, JSDOUBLE_IS_INT32 is currently in a private header.  It seems reasonable to hoist into jsapi.h, but to do that we'll need to #include math.h/float.h/ieeefp.h from jsapi.h.

Jason: can I just do that?

Comment 8

[Jason Orendorff [:jorendorff]]

Luke, see bug 640494. JSDOUBLE_IS_NEGZERO happens to be a bit of a headache, because using math.h macros causes conflicts with <cmath> under GCC with -stdc++0x. I think what I'm going to do there is reimplement signbit from scratch. :-P

Comment 9

[Steve Fink [:sfink] [:s:]]

There's only one -0 in any format we care about, right? So wouldn't this work?:

JS_ALWAYS_INLINE JSBool
JSDOUBLE_IS_NEGZERO(jsdouble d)
{
    static union {
        jsdouble d;
        uint64 bits;
    } NEG_ZERO = { -0.0f };
    union {
        jsdouble d;
        uint64 bits;
    } x = { d };
    return x.bits == NEG_ZERO.bits;
}

On x86_64:

(gdb) disass is_negzero
Dump of assembler code for function _Z10is_negzerod:
   0x0000000000400520 <+0>:	movsd  %xmm0,-0x8(%rsp)
   0x0000000000400526 <+6>:	mov    -0x8(%rsp),%rax
   0x000000000040052b <+11>:	cmp    %rax,0x15e(%rip)        # 0x400690 <_ZZ10is_negzerodE8NEG_ZERO>
   0x0000000000400532 <+18>:	sete   %al
   0x0000000000400535 <+21>:	retq

Comment 10

[Jason Orendorff [:jorendorff]]

Heh! That's pretty much exactly what I posted in bug 640494. Looking at the assembly under MSVC now...