742188 - Verify whether the new binding unwrapping code for int64/uint64 does the right thing per webidl

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Boris Zbarsky [:bzbarsky]]

I bet it doesn't.

Hard to add tests until something uses those types in a binding....  Maybe we should have some test-only object files and IDL to exercise some of this stuff?

Comment 1

[Boris Zbarsky [:bzbarsky]]

The answer was that it certainly did not.  See bug 766796.

The non-finite handling is fixed, but we still don't do the mod thing.

Comment 2

[Eric Faust [:efaust]]

Is "the mod thing" specified when the target is an {u,}int64? It looks like ECMA has made no judgement on that.

Comment 3

[Boris Zbarsky [:bzbarsky]]

> Is "the mod thing" specified when the target is an {u,}int64?

Yes.  See http://dev.w3.org/2006/webapi/WebIDL/#es-long-long step 6 and http://dev.w3.org/2006/webapi/WebIDL/#es-unsigned-long-long step 6.

Comment 4

[Eric Faust [:efaust]]

Attachment: Glimmers of refactor

Oh my god does this feel ugly. Code comment cleanup (to come) will certainly clarify somewhat.

A couple of questions:

1) Where is this monstrosity to live? I guess it can live where it stood, but it seems weird for xpcpublic.h to include vm/ANYTHING.

2) Is the templated system actually cleaner, in particular in the non-x86 case? Comments will certainly help.

3) Assuming anyone who reads the patch doesn't turn to stone, who should review it when the time comes?

Comment 5

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 646056 [details] [diff] [review]
Glimmers of refactor

Seems reasonable to me, but I'm not an expert on this code....

Comment 6

[Eric Faust [:efaust]]

Attachment: Refactor and Fix

Move ToIntWidth to jsfriendapi.h, to allow vision from xpconnect. Add callsites.

Comment 7

[Jason Orendorff [:jorendorff]]

Comment on attachment 647356 [details] [diff] [review]
Refactor and Fix

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

Eric, below I recommend a different approach that would leave most of the code in NumericConversions.h.

If you agree with that, please post an updated patch -- I think the template-ification will be easier to review in that patch than this one.

If not, just ping me on IRC.

Clearing review flag.

::: js/xpconnect/src/xpcpublic.h
@@ +290,4 @@
>          int32_t intval;
>          if (!JS_ValueToECMAInt32(cx, v, &intval))
>              return false;
>          *result = static_cast<int64_t>(intval);

Instead of calling JS_ValueToECMAInt32, this could just say

    *result = JSVAL_TO_INT(v);

Same thing in ValueToUint64.

@@ +294,5 @@
>      } else {
>          double doubleval;
>          if (!JS_ValueToNumber(cx, v, &doubleval))
>              return false;
> +        *result = static_cast<int64_t>(js::ToIntWidth<64>(doubleval));

I don't think the inline here is necessary for performance. So it'd be a lot better to add entry points JS_ValueToInt64 and Uint64 in jsapi.h (right next to JS_ValueToECMAInt32) and put the implementation in jsapi.cpp (ditto). Then all the goofy numeric conversion code could remain internal to the JS engine and all together in one header.

Comment 8

[Eric Faust [:efaust]]

Attachment: Integrate into JS Engine

Comment 9

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 647819 [details] [diff] [review]
Integrate into JS Engine

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

::: js/src/jsnum.h
@@ +167,5 @@
>      return ToUint32Slow(cx, v, out);
>  }
>  
>  /*
> + * Convert a value to an int64_t, according to the WEBIDL rules for long long

s/WEBIDL/WebIDL/g

::: js/src/vm/NumericConversions.h
@@ +32,5 @@
>  };
>  
>  } /* namespace detail */
>  
> +/* Numeric Conversion base. Found doubles to Ints according to ECMA or WEBIDL standards. */

Found?

::: js/xpconnect/src/xpcpublic.h
@@ +285,5 @@
>   */
>  inline bool
>  ValueToInt64(JSContext *cx, JS::Value v, int64_t *result)
>  {
>      if (JSVAL_IS_INT(v)) {

While you're here... v.isInt32()

@@ +287,5 @@
>  ValueToInt64(JSContext *cx, JS::Value v, int64_t *result)
>  {
>      if (JSVAL_IS_INT(v)) {
> +        /* Fastpath the common case */
> +        *result = JSVAL_TO_INT(v);

and v.toInt32(), please

@@ -312,5 @@
>  ValueToUint64(JSContext *cx, JS::Value v, uint64_t *result)
>  {
>      if (JSVAL_IS_INT(v)) {
> -        uint32_t intval;
> -        if (!JS_ValueToECMAUint32(cx, v, &intval))

Did this turn -1 into 2**32 - 1 instead of 2**64 - 1? I wish we could test this more easily...

Comment 10

[Eric Faust [:efaust]]

Attachment: Fix some nits, as well as a nasty casting bug.

Fix some style nits and a great catch on int32_t to uint64_t casting based on Ms2ger's comments.

Comment 11

[Jason Orendorff [:jorendorff]]

Comment on attachment 647819 [details] [diff] [review]
Integrate into JS Engine

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

Eric, I'm really really sorry to move the finish line again, but I just noticed that all the existing code we have in this area is a gross mess.

I filed bug 779601 to clean that stuff up so that all the int conversion functions work the same way. The patch there has landed.

Please take a look at that patch. Then it should be obvious what to add. I'm sorry for the mess.

::: js/src/vm/NumericConversions.h
@@ +32,5 @@
>  };
>  
>  } /* namespace detail */
>  
> +/* Numeric Conversion base. Found doubles to Ints according to ECMA or WEBIDL standards. */

s/Found/Round/

@@ +35,5 @@
>  
> +/* Numeric Conversion base. Found doubles to Ints according to ECMA or WEBIDL standards. */
> +template<size_t width>
> +inline int64_t
> +ToIntWidth(double d)

Make the return type a template parameter.

It's important not to change the code we currently get for the 32-bit case -- though the path from XPConnect to here is not hot, this *is* a hot function (thanks to other callers) and what we've got now was hand-tuned by an Intel engineer.

@@ +63,5 @@
>  
>      u_tmp = (di_h & 0x7ff00000) - 0x3ff00000;
> +    if (u_tmp >= ((width + 52) << 20)) {
> +        // d is Nan, +/-Inf or +/-0, or |d|>=2^(width+52) or |d|<1, in which case result=0
> +        // If we need to shift be more than (width + 52), there are no data bits

s/be/by/

@@ +66,5 @@
> +        // d is Nan, +/-Inf or +/-0, or |d|>=2^(width+52) or |d|<1, in which case result=0
> +        // If we need to shift be more than (width + 52), there are no data bits
> +        // to preserve, and the mod will turn out 0. If |d|<1, subtracting the
> +        // bias will make the result negative, which appears as a large positive
> +        // uint.

I don't understand the last sentence. If |d| < 1, then simply rounding toward zero causes the result to be 0. Right?

@@ +72,5 @@
>      }
>  
> +    if (u_tmp < ((width - 1) << 20)) {
> +        // |d|<2^(width - 1)
> +        return int64_t(d);

return ResultType(d);  or whatever you call the type parameter. Same thing in several places.

@@ +77,5 @@
>      }
>  
> +    if (u_tmp > ((width - 1) << 20)) {
> +        // |d|>=2^width. Save all the relevant bits that would be saved after
> +        // doing the mod, with 11 extra shift to cover sign and exponent.

This comment is unclear to me.

Perhaps:
        // |d|>=2^width
        // Throw away multiples of 2^width.
        //
        // That is, compute du.d = the value in (-2^width, 2^width)
        // that has the same sign as d and is equal to d modulo 2^width.
        //
        // This can't be done simply by masking away bits of du because
        // the implicit one-bit of the mantissa is one of the ones we want to
        // eliminate. So instead we compute duh.d = the appropriate multiple
        // of 2^width, which *can* be computed by masking, and then we
        // subtract that from du.d.

If this is too much, delete stuff from the end (like a newspaper editor).

@@ +105,2 @@
>          expon = u_tmp >> 20;
> +        // same idea as before, except save everything non-fractional.

Style nits: Blank line before this comment, since it isn't at the top of a block. Capitalize the first word.

@@ +142,5 @@
> +
> +    // XXXefaust Should we rely on pow here? Is bit encoding through a union
> +    // better?
> +    twoWidth = width == 32 ? 4294967296.0 : pow(2.0, (double) width);
> +    twoWidthMin1 = width == 32 ? 2147483648.0 : (double)(1 << (width - 1));

Well, you can always cheat:

template <class int32_t>
struct ToIntConstants {
    static const int width = 32;
    static double twoToTheN() { return 4294967296.0; }
    static double twoToTheNMinusOne() { return 2147483648.0; }
};

template <class int64_t>
struct ToIntConstants {
    static const int width = 64;
    static double twoToTheN() { return 18446744073709551616.0; }
    static double twoToTheNMinusOne() { return 9223372036854775808.0; }
};

but since this branch of the #if runs approximately nowhere, I'm not super worried about it.

I don't know enough about how pow() is implemented to know if it can be expected to give precise results for powers of 2. Ditto fmod.

@@ +286,5 @@
>  {
>      return uint32_t(ToInt32(d));
>  }
>  
> +/* WEBIDL 4.2.10 (specialized for doubles). */

Does "(specialized for doubles)" mean anything to you? if not, kill it

::: js/xpconnect/src/xpcpublic.h
@@ +283,5 @@
>  /**
>   * Convert a jsval to PRInt64. Return true on success.
>   */
>  inline bool
>  ValueToInt64(JSContext *cx, JS::Value v, int64_t *result)

Just remove these two functions entirely and change its callers to call JS::ToInt64 instead.

Comment 12

[Eric Faust [:efaust]]

Attachment: Once more, with feeling.

Comment 13

[Jason Orendorff [:jorendorff]]

Comment on attachment 648602 [details] [diff] [review]
Once more, with feeling.

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

r=me with the one nit picked. Thanks.

::: js/src/jsapi.h
@@ +2867,5 @@
> +        MaybeCheckStackRoots(cx);
> +    }
> +
> +    if (v.isInt32()) {
> +        // Account for sign extension of negatives into the longer 64bit space.

Nice comment.

::: js/src/vm/NumericConversions.h
@@ +275,5 @@
>      : "cc"
>          );
>      return i;
>  #else
> +    return int32_t(ToIntWidth<32, int32_t>(d));

Remove the cast; it's no longer necessary.

Comment 14

[Eric Faust [:efaust]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/83c9437ff26d

Comment 15

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/83c9437ff26d