Closed Bug 1304191 Opened 4 years ago Closed 4 years ago

Fold jsval_layout into JS::Value.

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla52
Tracking Status
firefox52 --- fixed

People

(Reporter: arai, Assigned: arai)

References

Details

Attachments

(13 files, 5 obsolete files)

9.48 KB, patch
Waldo
: review+
Details | Diff | Splinter Review
16.21 KB, patch
Waldo
: review+
Details | Diff | Splinter Review
4.99 KB, patch
Waldo
: review+
Details | Diff | Splinter Review
4.41 KB, patch
Waldo
: review+
Details | Diff | Splinter Review
7.25 KB, patch
Waldo
: review+
Details | Diff | Splinter Review
2.04 KB, patch
Waldo
: review+
Details | Diff | Splinter Review
2.68 KB, patch
Waldo
: review+
Details | Diff | Splinter Review
2.91 KB, patch
Waldo
: review+
Details | Diff | Splinter Review
9.30 KB, patch
Waldo
: review+
Details | Diff | Splinter Review
16.22 KB, patch
Waldo
: review+
Details | Diff | Splinter Review
1.37 KB, patch
Waldo
: review+
Details | Diff | Splinter Review
37.44 KB, patch
Waldo
: review+
Details | Diff | Splinter Review
1.17 KB, patch
Waldo
: review+
Details | Diff | Splinter Review
derived from bug 1290337 comment #44.
Here's rough plan (draft patch is ready):
  * Add JS::Value static methods to create JS::Value from:
    * tag and payload
    * raw bits
    * int32_t
    * double
  * Add public JS::Value.toTag() to make it accessible from JIT etc

that way, we can avoid having jsval_layout as input/output type outside JS::Value implementation,

Then:
  * Move/embed whole jsval_layout related functions into JS::Value methods
  * Fold jsval_layout into JS::Value
Assignee: nobody → arai.unmht
Status: NEW → ASSIGNED
Apparently we need JS::Value.toPayload() for 32bit, for JIT.
try run passed.
will post patches after bug 1290337 patches.
all those patches will be landed at the same time.
Parts 1-20 in bug 1290337 won't harm too much without remaining parts.
will land them after test.
Now all requirements for JS_VALUE_IS_CONSTEXPR are available on all compilers (am I correct?)
We can remove macros related to it ans simplify the code.
Attachment #8797704 - Flags: review?(jwalden+bmo)
remaining parts depend on Part 1, so will post them later.
Comment on attachment 8797704 [details] [diff] [review]
Part 1: Remove JS_VALUE_IS_CONSTEXPR macro.

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

::: js/public/Value.h
@@ +329,3 @@
>  BUILD_JSVAL(JSValueTag tag, uint32_t payload)
>  {
> +    return (jsval_layout) { .asBits = (((uint64_t)(uint32_t)tag) << 32) | payload };

`.asBits` is non-standard syntax and MSVC doesn't accept it. And MSVC also doesn't like `(jsval_layout) { ... }`, it only accepts the initializer form without parentheses: `jsval_layout { ... }`.

@@ +1386,1 @@
>  CanonicalizedDoubleValue(double d)

I don't think `constexpr` is actually valid here (bug 1301864).

@@ +1534,1 @@
>  NumberValue(uint32_t i)

Same issue here (bug 1301864).
Comment on attachment 8797704 [details] [diff] [review]
Part 1: Remove JS_VALUE_IS_CONSTEXPR macro.

Thanks :)
I'll rewrite patches to keep JS_VALUE_IS_CONSTEXPR
Attachment #8797704 - Flags: review?(jwalden+bmo)
BUILD_JSVAL is used to create a bit pattern for given value.
Added the following functions to do the same things without having temporary jsval_layout:
  * JS::Value constructor with uint64_t parameter
    constructs JS::Value from asBits value
    this is private, called from JS::Value::fromRawBits

  * JS::Value::bitsFromTagAndPayload
    create asBits member value from tag and payload
  * JS::Value::fromTagAndPayload
    create JS::Value from tag and payload
  * JS::Value::fromRawBits
    create JS::Value from asBits value
  * JS::Value::fromInt32
    create Int32 JS::Value from int32_t value

and replaced INT32_TO_JSVAL_IMPL with JS::Value::fromInt32 to use it from Int32Value.
Attachment #8797704 - Attachment is obsolete: true
Attachment #8798414 - Flags: review?(jwalden+bmo)
Comment on attachment 8798414 [details] [diff] [review]
Part 1: Change BUILD_JSVAL to JS::Value::fromRawBits and JS::Value::fromTagAndPayload.

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

::: js/public/Value.h
@@ +1342,5 @@
>      jsval_layout data;
>  
>    private:
>  #if defined(JS_VALUE_IS_CONSTEXPR)
> +    explicit JS_VALUE_CONSTEXPR Value(uint64_t asBits) : data({ .asBits = asBits }) {}

These days we can depend on unrestricted unions, according to the canonical doc.  Run with what's in this patch for now, but let's have a followup patch that eliminates JS_VALUE_IS_CONSTEXPR entirely.  \o/

@@ +1358,5 @@
>      friend Value JS_VALUE_CONSTEXPR (::IMPL_TO_JSVAL)(const jsval_layout& l);
>      friend Value JS_VALUE_CONSTEXPR (JS::UndefinedValue)();
> +
> +  public:
> +    static inline JS_VALUE_CONSTEXPR uint64_t

No need for |inline| noise on this, or the others: functions defined in the class definition are implicitly inline.  And I'd put the whole signature on one line.
Attachment #8798414 - Flags: review?(jwalden+bmo) → review+
Added JS::Value::toTag, to check tag in JS::Value::is* methods.
Attachment #8800070 - Flags: review?(jwalden+bmo)
Changed JS::Value::set* methods to use JS::Value::bitsFromTagAndPayload or simply set members directly.
Attachment #8800071 - Flags: review?(jwalden+bmo)
Just moved JSVAL_SAME_TYPE_IMPL body to SameType,
and removed JSVAL_IS_DOUBLE_IMPL.
Attachment #8800072 - Flags: review?(jwalden+bmo)
Similarly, just moved JSVAL_TRACE_KIND_IMPL body to JS::Value::traceKind.
Attachment #8800074 - Flags: review?(jwalden+bmo)
Moved JSVAL_TO_*_IMPL body to JS::Value::to* methods.
Attachment #8800075 - Flags: review?(jwalden+bmo)
Moved JSVAL_EXTRACT_NON_DOUBLE_TYPE_IMPL body to JS::Value::extractNonDoubleType.
Attachment #8800076 - Flags: review?(jwalden+bmo)
* Added JS::Value::toPayload (only on 32bit)
  * Changed JS::Value::toTag to public
to make them available in JIT code.
Attachment #8800077 - Flags: review?(jwalden+bmo)
(In reply to Tooru Fujisawa [:arai] from comment #18)
> Created attachment 8800077 [details] [diff] [review]
> Part 8: Stop using jsval_layout in JIT.
> 
>   * Added JS::Value::toPayload (only on 32bit)
>   * Changed JS::Value::toTag to public
> to make them available in JIT code.

Also:
  * Added JS::Value::toMarkablePointer
as an complementary for JS::Value::isMarkable.
it's almost same as toGCThing, but asserts isMarkable() instead of isGCThing, so null is also allowed.
To hide jsval_layout from CanonicalizedDoubleValue, added JS::Value::fromDouble,
and replaced the CanonicalizedDoubleValue body with JS::Value::fromRawBits and JS::Value::fromDouble.
Attachment #8800078 - Flags: review?(jwalden+bmo)
Removed now unused JSVAL_TO_IMPL and IMPL_TO_JSVAL.
Attachment #8800079 - Flags: review?(jwalden+bmo)
Moved jsval_layout union into JS::Value.
Attachment #8800080 - Flags: review?(jwalden+bmo)
Comment on attachment 8800070 [details] [diff] [review]
Part 2: Move JSVAL_IS_*_IMPL into JS::Value methods.

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

::: js/public/Value.h
@@ +908,5 @@
>          data.asBits = tmp;
>      }
>  
> +private:
> +

Kill the extra blank line, indent private two spaces.  Same for the other access modifier sections.

@@ +925,5 @@
> +    /*
> +     * N.B. GCC, in some but not all cases, chooses to emit signed comparison
> +     * of JSValueTag even though its underlying type has been forced to be
> +     * uint32_t.  Thus, all comparisons should explicitly cast operands to
> +     * uint32_t.

Is there good reason to believe this isn't simply because JSValueTag is defined using JS_ENUM_{HEADER,FOOTER}, and those macros do *not* force type to uint32_t on non-MSVC compilers?

#if defined(_MSC_VER)
# define JS_ENUM_HEADER(id, type)              enum id : type
# define JS_ENUM_FOOTER(id)
#else
# define JS_ENUM_HEADER(id, type)              enum id
# define JS_ENUM_FOOTER(id)                    __attribute__((packed))
#endif

Now consider the spec:

"""
For an enumeration whose underlying type is not fixed, the underlying type is an integral type that can represent all the enumerator values defined in the enumeration.  It is implementation-defined which integral type is used as the underlying type except that the underlying type shall not be larger than int unless the value of an enumerator cannot fit in an int or unsigned int.
"""

The NUNBOX32 case looks like it'd pick unsigned int for the underlying type, because 0xFFFFFF80 won't fit in int, but does fit in unsigned int, so a type not larger than int -- but large enough to hold that value -- must be selected.  Ergo, unsigned int.  So that looks okay:

JS_ENUM_HEADER(JSValueTag, uint32_t)
{
    JSVAL_TAG_CLEAR                = 0xFFFFFF80,
    JSVAL_TAG_INT32                = JSVAL_TAG_CLEAR | JSVAL_TYPE_INT32,
    JSVAL_TAG_UNDEFINED            = JSVAL_TAG_CLEAR | JSVAL_TYPE_UNDEFINED,
    JSVAL_TAG_STRING               = JSVAL_TAG_CLEAR | JSVAL_TYPE_STRING,
    JSVAL_TAG_SYMBOL               = JSVAL_TAG_CLEAR | JSVAL_TYPE_SYMBOL,
    JSVAL_TAG_BOOLEAN              = JSVAL_TAG_CLEAR | JSVAL_TYPE_BOOLEAN,
    JSVAL_TAG_MAGIC                = JSVAL_TAG_CLEAR | JSVAL_TYPE_MAGIC,
    JSVAL_TAG_NULL                 = JSVAL_TAG_CLEAR | JSVAL_TYPE_NULL,
    JSVAL_TAG_OBJECT               = JSVAL_TAG_CLEAR | JSVAL_TYPE_OBJECT,
    JSVAL_TAG_PRIVATE_GCTHING      = JSVAL_TAG_CLEAR | JSVAL_TYPE_PRIVATE_GCTHING
} JS_ENUM_FOOTER(JSValueTag);

But the PUNBOX64 case looks problematic:

JS_ENUM_HEADER(JSValueTag, uint32_t)
{
    JSVAL_TAG_MAX_DOUBLE     	   = 0x1FFF0,
    JSVAL_TAG_INT32                = JSVAL_TAG_MAX_DOUBLE | JSVAL_TYPE_INT32,
    JSVAL_TAG_UNDEFINED            = JSVAL_TAG_MAX_DOUBLE | JSVAL_TYPE_UNDEFINED,
    JSVAL_TAG_STRING               = JSVAL_TAG_MAX_DOUBLE | JSVAL_TYPE_STRING,
    JSVAL_TAG_SYMBOL               = JSVAL_TAG_MAX_DOUBLE | JSVAL_TYPE_SYMBOL,
    JSVAL_TAG_BOOLEAN        	   = JSVAL_TAG_MAX_DOUBLE | JSVAL_TYPE_BOOLEAN,
    JSVAL_TAG_MAGIC                = JSVAL_TAG_MAX_DOUBLE | JSVAL_TYPE_MAGIC,
    JSVAL_TAG_NULL                 = JSVAL_TAG_MAX_DOUBLE | JSVAL_TYPE_NULL,
    JSVAL_TAG_OBJECT               = JSVAL_TAG_MAX_DOUBLE | JSVAL_TYPE_OBJECT,
    JSVAL_TAG_PRIVATE_GCTHING      = JSVAL_TAG_MAX_DOUBLE | JSVAL_TYPE_PRIVATE_GCTHING
} JS_ENUM_FOOTER(JSValueTag);

JSVAL_TAG_MAX_DOUBLE fits in int, and the JSVAL_TYPE_* constants are all <= 0x0c.  So for PUNBOX64, there's no reason that on non-MSVC, JSValueTag couldn't have an underlying signed type.

Maaaaaybe this runs into those gcc bugs about fixed-type enums.  Or maybe it doesn't, because the fixed type we're using is a big type, not uint8_t or so.  Once all these patches have landed, we should try to make JSValueTag into uint32_t for real, then remove all the random casts for this stuff.

@@ -1111,4 @@
>      }
>  
>      bool isDouble() const {
> -        return JSVAL_IS_DOUBLE_IMPL(data);

You didn't actually remove JSVAL_IS_DOUBLE_IMPL.  (If you removed it later in the stack, fine.  Just something I noted in going through all the removals-and-replacements, then seeing this changed without it having been removed elsewhere.)

Oh, maybe because there are other uses of it.  And same for JSVAL_IS_PRIVATE_GCTHING_IMPL and JSVAL_IS_MARKABLE_IMPL, and maybe one more.  Carry on!
Attachment #8800070 - Flags: review?(jwalden+bmo) → review+
Comment on attachment 8800071 [details] [diff] [review]
Part 3: Move *_TO_JSVAL_IMPL into JS::Value methods.

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

::: js/public/Value.h
@@ +686,1 @@
>      }

Can this function be made private?  Having this public looks super-scary.

@@ +1009,5 @@
> +
> +        MOZ_ASSERT(uintptr_t(cell) > 0x1000);
> +#if defined(JS_PUNBOX64) && defined(DBEUG)
> +        uintptr_t cellBits = uintptr_t(cell);
> +        MOZ_ASSERT((cellBits >> JSVAL_TAG_SHIFT) == 0);

Mild preference for

#if defined(JS_PUNBOX64)
        MOZ_ASSERT((uintptr_t(cell) >> JSVAL_TAG_SHIFT) == 0);
#endif

Actually, major preference for that -- DBEUG ain't gonna work.  :-)

::: js/src/jsutil.h
@@ +352,5 @@
>  # if defined(JS_PUNBOX64)
>      obj = obj & ((uintptr_t(1) << JSVAL_TAG_SHIFT) - 1);
>  # endif
> +    JS::Value v;
> +    v.setObjectNoCheck((JSObject*)obj);

Oh, hm.  Can we add a

static inline Value
PoisonedObjectValue(JSObject* obj)
{
    Value v;
    v.setObjectNoCheck(obj);
    return v;
}

to Value.h, make PoisonedObjectValue a friend of Value, then have this code use that, instead of this being public?
Attachment #8800071 - Flags: review?(jwalden+bmo) → review+
Comment on attachment 8800072 [details] [diff] [review]
Part 4: Move JSVAL_SAME_TYPE_IMPL into SameType.

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

::: js/public/Value.h
@@ +625,1 @@
>          data.asDouble = d;

e_e

@@ +1376,5 @@
> +    JSValueTag ltag = lhs.toTag(), rtag = rhs.toTag();
> +    return ltag == rtag || (ltag < JSVAL_TAG_CLEAR && rtag < JSVAL_TAG_CLEAR);
> +#elif defined(JS_PUNBOX64)
> +    return (lhs.isDouble() && rhs.isDouble()) ||
> +           (((lhs.data.asBits ^ rhs.data.asBits) & 0xFFFF800000000000LL) == 0);

Make that ULL, in a separate patch if you're feeling paranoid.  You can't represent that number in |long long| (at least when that type is 64-bit), so really that means |unsigned long long|, but then you might as well use the unambiguous suffix.

::: js/src/vm/StructuredClone.cpp
@@ +1605,5 @@
>  JSStructuredCloneReader::checkDouble(double d)
>  {
> +    JS::Value v;
> +    v.setDoubleNoCheck(d);
> +    if (!v.isDouble()) {

In a separate patch, I think we could/should write a

static inline bool
IsCanonicalized(double d);

function and use that here.  Having to write into a Value and test it is not quite the right thing, especially as it requires us to expose a no-check function.
Attachment #8800072 - Flags: review?(jwalden+bmo) → review+
Comment on attachment 8800074 [details] [diff] [review]
Part 5: Move JSVAL_TRACE_KIND_IMPL into JS::Value methods.

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

::: js/public/Value.h
@@ +809,5 @@
> +            return JS::GCThingTraceKind(JSVAL_TO_GCTHING_IMPL(data));
> +#if defined(JS_NUNBOX32)
> +        return JS::TraceKind(toTag() & 0x03);
> +#elif defined(JS_PUNBOX64)
> +        return JS::TraceKind(uint32_t(data.asBits >> JSVAL_TAG_SHIFT) & 0x03);

Given that toTag() is just data.s.tag if JS_NUNBOX32, and toTag() is (data.asBits >> tagshift) if JS_PUNBOX64, I think you can just have

  return JS::TraceKind(toTag() & 0x03);

replacing this entire #if/#elif/#endif.  Feel free to punt it into a followup patch if you're leery of making the change as part of this exact refactoring.
Attachment #8800074 - Flags: review?(jwalden+bmo) → review+
Comment on attachment 8800075 [details] [diff] [review]
Part 6: Move JSVAL_TO_*_IMPL into JS::Value methods.

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

::: js/public/Value.h
@@ +772,5 @@
>          MOZ_ASSERT(isString());
> +#if defined(JS_NUNBOX32)
> +        return data.s.payload.str;
> +#elif defined(JS_PUNBOX64)
> +        return (JSString*)(data.asBits & JSVAL_PAYLOAD_MASK);

reinterpret_cast<>, please.  Also for the other C-style casts in this patch, in the other functions.

@@ +793,5 @@
> +#elif defined(JS_PUNBOX64)
> +        uint64_t ptrBits = data.asBits & JSVAL_PAYLOAD_MASK;
> +        MOZ_ASSERT((ptrBits & 0x7) == 0);
> +        return *(JSObject*)ptrBits;
> +#endif

Seems like it'd be better for this function to be

  MOZ_ASSERT(isObject());
  return *toObjectOrNull();

so we don't have the gunk repeated.

@@ +869,5 @@
>          MOZ_ASSERT(isDouble());
> +#if defined(JS_NUNBOX32)
> +        return data.s.payload.ptr;
> +#elif defined(JS_PUNBOX64)
> +        MOZ_ASSERT((data.asBits & 0x8000000000000000LL) == 0);

ULL for this as well.
Attachment #8800075 - Flags: review?(jwalden+bmo) → review+
Comment on attachment 8800076 [details] [diff] [review]
Part 7: Move JSVAL_EXTRACT_NON_DOUBLE_TYPE_IMPL into JS::Value methods.

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

::: js/public/Value.h
@@ +821,5 @@
>  
>      JSValueType extractNonDoubleType() const {
> +        uint32_t type = toTag() & 0xF;
> +        MOZ_ASSERT(type > JSVAL_TYPE_DOUBLE);
> +        return (JSValueType)type;

JSValueType(type)
Attachment #8800076 - Flags: review?(jwalden+bmo) → review+
Comment on attachment 8800078 [details] [diff] [review]
Part 9: Add Value::fromDouble and use it in CanonicalizedDoubleValue.

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

::: js/public/Value.h
@@ +1058,5 @@
>  static inline JS_VALUE_CONSTEXPR Value
>  CanonicalizedDoubleValue(double d)
>  {
> +    return MOZ_UNLIKELY(mozilla::IsNaN(d))
> +           ? Value::fromRawBits(0x7FF8000000000000LL)

This is strange.  I would have expected this number (tack on a U while you're here, please) to be the same as the constant in JS::GenericNaN().  It seems pretty strange that it's not.  I don't especially doubt this is kosher -- but it does seem strange our generic NaN would not be the same as the canonical one, because that means munging a generic NaN isn't the identity operation.

Maybe we can investigate this in a followup and either add comments as to why the two constants are different, or unify them into one (ideally a single unified constant somewhere, even).

It's also *very* strange that this function has a non-delegating implementation, *and* JS::CanonicalizeNaN has a non-delegating implementation.  We should probably (followup) make this function use that one.  Or get rid of this function entirely and have all the users use JS::CanonicalizeNaN passed to DoubleValue.
Attachment #8800078 - Flags: review?(jwalden+bmo) → review+
Attachment #8800079 - Flags: review?(jwalden+bmo) → review+
Comment on attachment 8800080 [details] [diff] [review]
Part 11: Fold jsval_layout into JS::Value.

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

::: js/public/Value.h
@@ +993,5 @@
>  #endif
>      }
>  } JS_HAZ_GC_POINTER;
>  
> +JS_STATIC_ASSERT(sizeof(Value) == 8);

static_assert(..., "Value size must leave three tag bits, be a binary power, and is ubiquitously depended upon everywhere");

or somesuch explanation.  Let's kill off a JS_STATIC_ASSERT when we're here.

@@ +1511,5 @@
>  namespace detail {
>  
>  struct ValueAlignmentTester { char c; JS::Value v; };
>  static_assert(sizeof(ValueAlignmentTester) == 16,
>                "JS::Value must be 16-byte-aligned");

Hmm.  Is this necessary any more now that JS::Value is alignas(8), or is this just detritus from that bug because it's not fully landed yet?  We should remove it if we can.
Attachment #8800080 - Flags: review?(jwalden+bmo) → review+
Comment on attachment 8800077 [details] [diff] [review]
Part 8: Stop using jsval_layout in JIT.

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

There's one very pervasive, design question/concern I have with this patch.  Namely, as far as I can tell, all the 32-bit JIT components use the |toTag()| function, *and then they rely on its JS_NUNBOX32 implementation being used*.  That is, they *assume* that |toTag()| will just return |data.s.tag| -- they assume implementation details of how |toTag()| works -- even though this is not entirely obvious unless you understand the nunbox/punbox naming.  At least |toPayload()| is *only* defined when nunbox32.  So if someone mistakenly called it on the wrong platform (even though it doesn't make sense there), perhaps because they mis-copypasta'd, it wouldn't compile.  The same is not true for |toTag()|.

It seems to me that, for these uses that access nunbox32/punbox64 layout-specific details to wreak violence upon them, it would be slightly better to have, similar to |toPayload()|, functions *only* defined when nunbox32, and for 64-bit functions *only* defined when punbox64.  I think it's also a good idea to encode the 32/64 nunbox/punbox information into the function name, as an extra tripwire for someone writing code that might use this stuff (to maybe warn them away from trying to use it in the first place).

So then, I think we should add to Value:

class Value
{
    ...

    /*** Interface for the JITs to pick apart, interact with, and construct raw Values ***/
#if defined(JS_NUNBOX32)
    PayloadType toNunboxPayload() const {
        return data.s.payload.i32;
    }

    JSValueTag toNunboxTag() const {
        return data.s.tag;
    }
#elif defined(JS_PUNBOX64)
    void* bitsAsPunboxPointer() {
        return static_cast<void*>(data.s.asBits);
    }
#endif

    ...
};

Also, if we add this, we can go back to |toTag()| being private, as it should be.  And then these functions, clearly named for their low-level access, are the only public things that expose such low-level detail.

Anyway.  This is a bit design-y, so throwing back to you for comment in response, 'cause I'm not 100% confident this is exactly how it should be.

::: js/public/Value.h
@@ +806,5 @@
> +#elif defined(JS_PUNBOX64)
> +        uint64_t ptrBits = data.asBits & JSVAL_PAYLOAD_MASK;
> +        MOZ_ASSERT((ptrBits & 0x7) == 0);
> +        return reinterpret_cast<js::gc::Cell*>(ptrBits);
> +#endif

Can't you just have as body

  MOZ_ASSERT(isMarkable());
  return toGCThing();

and not repeat this mess'o'code?

::: js/src/jit/JitFrames.cpp
@@ +1068,5 @@
>  
>  #ifdef JS_NUNBOX32
>      LAllocation type, payload;
>      while (safepoint.getNunboxSlot(&type, &payload)) {
> +        JSValueTag tag = (JSValueTag)ReadAllocation(frame, &type);

JSValueTag(ReadAllocation(...))

::: js/src/jit/mips32/MacroAssembler-mips32.cpp
@@ +1530,2 @@
>      if (val.isMarkable())
> +        ma_li(data, ImmGCPtr(val.toGCThing()));

Shouldn't/can't this be |val.toMarkablePointer()|?  Seems more specific than toGCThing(), matches with the immediately preceding test, &c.

::: js/src/jit/x86-shared/Assembler-x86-shared.cpp
@@ +64,3 @@
>                  // Only update the code if the Value changed, because the code
>                  // is not writable if we're not moving objects.
> +                X86Encoding::SetPointer(buffer + offset, (void*)v.asRawBits());

These various |(void*)v.asRawBits()| seem like they would be better as a function in Value that performs the cast for you.

It *happens* that toUnmarkedPtr() exists (and setUnmarkedPtr beside it) and does that, but I'm not sure that name is really the right name for what's wanted in these various parts.  Also that function is currently entirely unused (likewise for setUnmarkedPtr).  :-)  So, how about we rename it (and remove setUnmarkedPtr) to what we want for these cases, then use it?  (Subject to the overview-comment I wrote, about having nunboxing/punboxing 32/64 be clearer in the function names, and the names suggested there.)
Sorry, I forgot to investigate/mention about the MOZ_ASSERT part.

Unfortunately, looks like we're hitting VisualStudio's bug or maybe a kind of restriction.

this code hits syntax error:
  AssertionConditionType<decltype((int(x) >> 1))>::isValid;

here's output:
  a.cc(14): error C2059: syntax error: '>'
  a.cc(14): error C3553: decltype expects an expression not a type
  a.cc(14): error C2059: syntax error: 'constant'
  a.cc(14): error C2955: 'AssertionConditionType': use of class template requires template argument list
  a.cc(5): note: see declaration of 'AssertionConditionType'

but this works:
  AssertionConditionType<decltype(int(x) >> 1)>::isValid;

This might be some language restriction that I'm not aware of tho...

Anyway, we cannot use parenthesized C++ style cast + shift inside decltype in template parameter,
and MOZ_ASSERT is expanded to pass given expression to the decltype.


To avoid the issue, we need to use one of the following workaround:
  A. use C style cast
  B. use reinterpret_cast or something (haven't tested)
  C. move cast outside of the MOZ_ASSERT
  D. move entire expression outside of the MOZ_ASSERT

I'd choose C, as in previous patch.
(of cousrse I fixed "DBEUG" :P

I've added a comment that describes the issue, can you review that part?
Attachment #8800929 - Flags: review?(jwalden+bmo)
Added JS::IsCanonicalized that checks if the given double value isn't non-canonicalized NaN.

0x7FF8000000000000ULL is the same bit pattern as GenericNaN.
but I added JSVAL_NAN_BITS macro, since GenericNaN is not constexpr.
Attachment #8800932 - Flags: review?(jwalden+bmo)
Attachment #8800071 - Attachment is obsolete: true
I agree that using Nunbox/Punbox in method name is safer.
Added toNunboxPayload/toNunboxTag/bitsAsPunboxPointer, and used them instead.

Also, changed toGCThing to toMarkablePointer.
Attachment #8800077 - Attachment is obsolete: true
Attachment #8800077 - Flags: review?(jwalden+bmo)
Attachment #8800934 - Flags: review?(jwalden+bmo)
I'm not sure if this is a good change.
this surely makes the code clearer, but this changes constant (JSVAL_NAN_BITS) to non-constant (GenericNaN()).
Attachment #8800936 - Flags: feedback?(jwalden+bmo)
Attachment #8800935 - Flags: review?(jwalden+bmo) → review+
Attachment #8800933 - Flags: review?(jwalden+bmo) → review+
Comment on attachment 8800929 [details] [diff] [review]
Part 3: Move *_TO_JSVAL_IMPL into JS::Value methods.

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

::: js/public/Value.h
@@ +678,5 @@
> +#if defined(JS_PUNBOX64) && defined(DEBUG)
> +        // VisualStudio cannot contain parenthesized C++ style cast and shift
> +        // inside decltype in template parameter:
> +        //   AssertionConditionType<decltype((uintptr_t(x) >> 1))>
> +        // It throws syntax error.

Hmm.  This probably changes rarely enough that it's not necessary to discuss the MSVC bug.  IMO.

I'd go for the C-style cast to avoid the extra #ifdef -- particularly one it's possible to get wrong -- but eh.
Attachment #8800929 - Flags: review?(jwalden+bmo) → review+
Comment on attachment 8800934 [details] [diff] [review]
Part 8: Stop using jsval_layout in JIT.

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

It looks like almost all asRawBits function calls and asBits references are in punbox-only code.  Not all, but most.  We should probably have toPunboxBits and use that in the overwhelming proportion of places where bits-access depends upon punbox layout.  But, please do that as a separate patch -- this is landable, it's big enough already, and quite possibly the super-few places that legitimately depending on raw bits in architecture-agnostic code should have a different solution than the existing asRawBits() anyway.

::: js/public/Value.h
@@ +567,5 @@
>      }
>  
>    public:
> +    /*** Interface for the JITs to pick apart, interact with, and construct raw
> +     *** Values ***/

Eeugh, dat comment-syntax.  Dat wrapping.  Grotesque.  Let's go with

    /*** JIT-only interfaces to interact with and create raw Values ***/

which doesn't transgress in line wrapping.  (Although it's pretty tempting to just kill this bizarre commenting style entirely in this file.)

::: js/src/jit/arm/MacroAssembler-arm.cpp
@@ +3273,2 @@
>      if (val.isMarkable())
> +        ma_mov(ImmGCPtr(reinterpret_cast<gc::Cell*>(val.toMarkablePointer())), data);

No cast.

@@ +5299,5 @@
>      // equal, short circuit false (NotEqual).
>      ScratchRegisterScope scratch(*this);
>  
> +    if (rhs.isMarkable()) {
> +        ma_cmp(lhs.payloadReg(), ImmGCPtr(reinterpret_cast<gc::Cell*>(rhs.toMarkablePointer())),

No cast (and so no bracing).

::: js/src/jit/arm/MacroAssembler-arm.h
@@ +911,3 @@
>          ma_str(scratch, ToType(dest), scratch2);
>          if (val.isMarkable())
> +            ma_mov(ImmGCPtr(reinterpret_cast<gc::Cell*>(val.toMarkablePointer())), scratch);

No cast.

@@ +938,5 @@
>  
>          // Store the payload, marking if necessary.
>          if (payloadoffset < 4096 && payloadoffset > -4096) {
>              if (val.isMarkable())
> +                ma_mov(ImmGCPtr(reinterpret_cast<gc::Cell*>(val.toMarkablePointer())), scratch2);

No cast.

@@ +945,5 @@
>              ma_str(scratch2, DTRAddr(scratch, DtrOffImm(payloadoffset)));
>          } else {
>              ma_add(Imm32(payloadoffset), scratch, scratch2);
>              if (val.isMarkable())
> +                ma_mov(ImmGCPtr(reinterpret_cast<gc::Cell*>(val.toMarkablePointer())), scratch2);

No cast.

@@ +974,2 @@
>          if (val.isMarkable())
> +            push(ImmGCPtr(reinterpret_cast<gc::Cell*>(val.toMarkablePointer())));

No cast.

::: js/src/jit/arm64/MacroAssembler-arm64.h
@@ +1847,5 @@
>              dataRelocations_.writeUnsigned(load.getOffset());
>      }
>      void writeDataRelocation(const Value& val, BufferOffset load) {
>          if (val.isMarkable()) {
> +            gc::Cell* cell = reinterpret_cast<gc::Cell*>(val.toMarkablePointer());

No cast.

::: js/src/jit/mips64/MacroAssembler-mips64.h
@@ +221,5 @@
>      }
>  
>      void writeDataRelocation(const Value& val) {
>          if (val.isMarkable()) {
> +            gc::Cell* cell = reinterpret_cast<gc::Cell *>(val.toMarkablePointer());

No cast.

::: js/src/jit/x64/MacroAssembler-x64.h
@@ +58,5 @@
>      // X64 helpers.
>      /////////////////////////////////////////////////////////////////
>      void writeDataRelocation(const Value& val) {
>          if (val.isMarkable()) {
> +            gc::Cell* cell = reinterpret_cast<gc::Cell*>(val.toMarkablePointer());

No cast.

::: js/src/jit/x86/MacroAssembler-x86.cpp
@@ +506,2 @@
>      if (rhs.isMarkable())
> +        cmpPtr(lhs.payloadReg(), ImmGCPtr(reinterpret_cast<gc::Cell*>(rhs.toMarkablePointer())));

No cast.

::: js/src/jit/x86/MacroAssembler-x86.h
@@ +97,2 @@
>          if (val.isMarkable())
> +            movl(ImmGCPtr(reinterpret_cast<gc::Cell*>(val.toMarkablePointer())), data);

No cast.

@@ +216,2 @@
>          if (val.isMarkable())
> +            push(ImmGCPtr(reinterpret_cast<gc::Cell*>(val.toMarkablePointer())));

No cast.

@@ +238,2 @@
>          if (val.isMarkable())
> +            movl(ImmGCPtr(reinterpret_cast<gc::Cell*>(val.toMarkablePointer())), ToPayload(dest));

No cast.
Attachment #8800934 - Flags: review?(jwalden+bmo) → review+
Comment on attachment 8800932 [details] [diff] [review]
Part 4.1: Add IsCanonicalized and use it instead of setDoubleNoCheck + isDouble.

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

I'm going to partly rescind my request for this change.

These changes aren't necessary for this patch to land, and writing the one comment that's absolutely required to finish up this patch is going to be tedious and time-consuming.  And reviewing it (and the code change that it justifies) is going to be very finicky work.

Please move this patch into a new bug.  If decide to be ambitious and go the extra mile by updating this patch, even when I say you don't need to -- which you have a habit of doing :-) -- I will actively r- and require it be in a new bug, after this lands.  :-)

::: js/public/Value.h
@@ +1134,5 @@
>      v.setDouble(dbl);
>      return v;
>  }
>  
> +#define JSVAL_NAN_BITS 0x7FF8000000000000ULL

Instead of this and the current JS::GenericNaN, have

namespace detail {

constexpr int CanonicalizedNaNSignBit = 0;
constexpr uint64_t CanonicalizedNaNSignificand = 0x8000000000000;

constexpr uint64_t CanonicalizedNaNBits =
    mozilla::SpecificNaNBits<double,
                             detail::CanonicalizedNaNSignBit,
                             detail::CanonicalizedNaNSignificand>::value;

} // namespace detail

static MOZ_ALWAYS_INLINE double
GenericNaN()
{
  return mozilla::SpecificNaN<double>(detail::CanonicalizedNaNSignBit,
                                      detail::CanonicalizedNaNSignificand);
}

and then in mfbt/FloatingPoint.h add (above the first SpecificNaN in that file)

/**
 * Computes the bit pattern for a NaN with the specified sign bit and
 * significand bits.
 */
template<typename T,
         int SignBit,
         typename FloatingPoint<T>::Bits Significand>
struct SpecificNaNBits
{
  using Traits = FloatingPoint<T>;

  static_assert(SignBit == 0 || SignBit == 1, "bad sign bit");
  static_assert((Significand & ~Traits::kSignificandBits) == 0,
                "significand must only have significand bits set");
  static_assert(Significand & Traits::kSignificandBits,
                "significand must be nonzero");

  static constexpr typename Traits::Bits value =
    (signbit * Traits::kSignBit) | Traits::kExponentBits | significand;
}

and then below use |detail::CanonicalizedNaNBits| in place of |JSVAL_NAN_BITS|.

@@ +1167,5 @@
> +        uint64_t asBits;
> +        double asDouble;
> +    };
> +    DoubleOrBits x;
> +    x.asDouble = d;

Instead of this, use

  uint64_t bits;
  BitwiseCast<double>(d, &bits);

and then use |bits| below.

@@ +1173,5 @@
> +#ifdef DEBUG
> +    DoubleOrBits y;
> +    y.asDouble = GenericNaN();
> +    MOZ_ASSERT(y.asBits == JSVAL_NAN_BITS);
> +#endif

Don't need any of this #ifdef with the changes described earlier.

@@ +1175,5 @@
> +    y.asDouble = GenericNaN();
> +    MOZ_ASSERT(y.asBits == JSVAL_NAN_BITS);
> +#endif
> +
> +    return (x.asBits & ~mozilla::DoubleTypeTraits::kSignBit) <= JSVAL_NAN_BITS;

I honestly can't say whether this is a correct implementation of what's canonical or not.  I've *wanted* a comment that enumerated all the cases for years, like the comment we have by the flags/bits constants in vm/String.h.  But we don't have one now.

This refactoring desperately needs to land before someone bitrots it.  These changes and this comment aren't important enough to hold it up.  And this function's behavior (and a comment by Value::data that explains why the implementation of this is correct) is extremely security-critical -- far too important to rush review of it, because of its holding up this bug's patches, which *would* cause me to do to some extent.

So remove this patch from the queue, land the queue with setDoubleNoCheck still existing, and file a new bug to remove setDoubleNoCheck.  We can iron out the last bits of this patch, and the super-important comment by Value::data, in that bug, and not hold this one up.

::: js/src/vm/StructuredClone.cpp
@@ +1609,5 @@
>                                    "unrecognized NaN");
>          return false;
>      }
> +    JS::Value v;
> +    v.setDouble(d);

I don't think you need these lines.  Most callers shove the double into a value anyway, and then they'll get the same assertion happening.

Well, actually, *every* caller -- if we remove the |!checkDouble(d)| in |case SCTAG_DATE_OBJECT| code that's unnecessary.  JS::TimeClip, by design, doesn't require that it only be passed canonicalized doubles.  And creating a Date from a TimeClip will do the right thing in the presence of a non-canonical double.  (Dates can *only* be created from TimeClips, unless you do insane things like build it from scratch and manually fill in its reserved slots and a bunch of other gunk.  Which is enough work that no one will do it but Date creation code.)

So please remove that call, and remove these two lines.
Attachment #8800932 - Flags: review?(jwalden+bmo) → review-
Comment on attachment 8800936 [details] [diff] [review]
Part 12: Use CanonicalizeNaN in CanonicalizedDoubleValue.

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

::: js/public/Value.h
@@ +1042,3 @@
>  CanonicalizedDoubleValue(double d)
>  {
> +    return Value::fromDouble(CanonicalizeNaN(d));

For whether we could dump constexpr on this: there's one CDV in generated code but ~1100+ NumberValues.  I imagine someone might complain if we added 1100+ static initializers.  ;-)  So the patch doesn't look practical.

But, we don't really need a CanonicalizedDoubleValue that's constexpr *that handles NaN*.  Everything passing a constant, is passing an integral constant (possibly via JS::NumberValue).  What if we had JS::IntegerConstantValue that doesn't have to address canonicalization?

constexpr Value
IntegerConstantValue(int8_t n)
{
  return Int32Value(n);
}

constexpr Value
IntegerConstantValue(uint8_t n)
{
  return Int32Value(n);
}

constexpr Value
IntegerConstantValue(int16_t n)
{
  return Int32Value(n);
}

constexpr Value
IntegerConstantValue(uint16_t n)
{
  return Int32Value(n);
}

constexpr Value
IntegerConstantValue(int32_t n)
{
  return Int32Value(n);
}

constexpr Value
IntegerConstantValue(uint32_t n)
{
  return n < uint32_t(INT32_MAX)
         ? Int32Value(int32_t(n))
         : Value::fromDouble(double(n));
}

namespace detail {

constexpr int64_t MaxSafeInteger = uint64_t(1) << 53;
constexpr int64_t MinSafeInteger = -int64_t(MaxSafeInteger - 1) - 1;

} // namespace detail

constexpr Value
IntegerConstantValue(int64_t n)
{
  return (INT32_MIN <= n && n <= INT32_MAX)
         ? Int32Value(int32_t(n))
         : (detail::MinSafeInteger <= n && n <= detail::MaxSafeInteger)
         ? Value::fromDouble(double(n))
         : (1 / 0);
}

constexpr Value
IntegerValue(uint64_t n)
{
  return n <= INT32_MAX
         ? Int32Value(int32_t(n))
         : n <= detail::MaxSafeInteger
         ? Value::fromDouble(double(n))
         : (1 / 0);
}

Then we could have this function do something nice, without losing needed constexpr capabilities.
Attachment #8800936 - Flags: feedback?(jwalden+bmo)
Comment on attachment 8800932 [details] [diff] [review]
Part 4.1: Add IsCanonicalized and use it instead of setDoubleNoCheck + isDouble.

Thank you for reviewing :)
I'll land other parts, and will file a bug for this.
Attachment #8800932 - Attachment is obsolete: true
Attachment #8800936 - Attachment is obsolete: true
Pushed by arai_a@mac.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2aa92c90865e
Part 1: Change BUILD_JSVAL to JS::Value::fromRawBits and JS::Value::fromTagAndPayload. r=jwalden
https://hg.mozilla.org/integration/mozilla-inbound/rev/3187c8a0b8a0
Part 2: Move JSVAL_IS_*_IMPL into JS::Value methods. r=jwalden
https://hg.mozilla.org/integration/mozilla-inbound/rev/5f5518aec3c8
Part 3: Move *_TO_JSVAL_IMPL into JS::Value methods. r=jwalden
https://hg.mozilla.org/integration/mozilla-inbound/rev/e2dfcf43893a
Part 4: Move JSVAL_SAME_TYPE_IMPL into SameType. r=jwalden
https://hg.mozilla.org/integration/mozilla-inbound/rev/2d9592f9ac97
Part 5: Move JSVAL_TRACE_KIND_IMPL into JS::Value methods. r=jwalden
https://hg.mozilla.org/integration/mozilla-inbound/rev/6f622578cc38
Part 5.1: Always use JS::Value::toTag() to get tag in JS::Value::traceKind. r=jwalden
https://hg.mozilla.org/integration/mozilla-inbound/rev/f128d23cb2b1
Part 6: Move JSVAL_TO_*_IMPL into JS::Value methods. r=jwalden
https://hg.mozilla.org/integration/mozilla-inbound/rev/965b70c459ae
Part 7: Move JSVAL_EXTRACT_NON_DOUBLE_TYPE_IMPL into JS::Value methods. r=jwalden
https://hg.mozilla.org/integration/mozilla-inbound/rev/b9e36eed9fb4
Part 8: Stop using jsval_layout in JIT. r=jwalden
https://hg.mozilla.org/integration/mozilla-inbound/rev/ad697c43b8d9
Part 8.1: Remove JS::Value::setUnmarkedPtr and JS::Value::toUnmarkedPtr. r=jwalden
https://hg.mozilla.org/integration/mozilla-inbound/rev/4dbf95d9e454
Part 9: Add Value::fromDouble and use it in CanonicalizedDoubleValue. r=jwalden
https://hg.mozilla.org/integration/mozilla-inbound/rev/061e30111c0f
Part 10: Remove JSVAL_TO_IMPL and IMPL_TO_JSVAL. r=jwalden
https://hg.mozilla.org/integration/mozilla-inbound/rev/463a01990213
Part 11: Fold jsval_layout into JS::Value. r=jwalden
See Also: → 1311088
will remove JS_VALUE_IS_CONSTEXPR in bug 1243617
See Also: → 1243617
Depends on: 1312488
You need to log in before you can comment on or make changes to this bug.