consolidate union DoublePun definitions

RESOLVED FIXED

Status

()

defect
RESOLVED FIXED
7 years ago
9 months ago

People

(Reporter: froydnj, Assigned: Waldo)

Tracking

unspecified
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(5 attachments, 2 obsolete attachments)

Reporter

Description

7 years ago
There are a surprising number of:

union DoublePun {
  uint64_t i;
  double d;
} ...

definitions floating around the tree, with or without trickery for accessing the uint32_t halves separately.  This is Not Good.
Reporter

Comment 1

6 years ago
Now that this header is all C++, we can move things into the mozilla namespace
if they are generally useful.  Which I think this is.
Attachment #756728 - Flags: review?(jwalden+bmo)
Reporter

Comment 2

6 years ago
I am not overly fond of dragging in Endian.h for this one little definition,
but that's how things go.  Maybe in a followup we can split Endian.h?
Attachment #756730 - Flags: review?(jwalden+bmo)
Reporter

Comment 3

6 years ago
There's one unaddressed DoublePun: assembler/assembler/AssemblerBufferWithConstantPool.h.
I guessed that wasn't worth handling since it's imported code (AFAIK) from JSC,
and the fewer changes made to that, the better.
Attachment #756731 - Flags: review?(jwalden+bmo)
Assignee

Comment 4

6 years ago
Part of the reason I didn't touch the lo/hi punning initially when adding the double-stuff was extra work/complexity, versus getting something done to be improved upon later.

But part of it is also that I'm not sure I like exposing access to hi/lo at all.  All the bit-level documentation of IEEE-754 will discuss the full 64 bits -- not a high half and a low half.  Code should be clearer to readers if it considers this, and only works with all 64 bits.  And the places that want high bits, or low bits, can just extract them from the uint64_t manually.

I have somewhat different ideas for how to do this, which I'll post in a second.  Oddly enough, I think we probably don't want to expose a DoublePun-like structure, but we *do* want to start exposing the double-format constants, which I hadn't thought likely before starting the patches.  I don't think we can really expect to expose all the operations on the bit-patterns that people will want, so we might as well give them the fundamental tools to do so if they want, and expect that most people won't touch them because it's too much trouble doing so.
Assignee

Comment 5

6 years ago
WTF has had this for awhile, and it seems the right way to expose this stuff.  Not a commonly-used way, to be sure, but probably better than one-off unions everywhere, or having to have publicly-exposed unions for commoned-up stuff.
Attachment #759912 - Flags: review?(nfroyd)
Assignee

Comment 6

6 years ago
The ToIntWidth changes need MakeUnsigned, so might as well do both.
Attachment #759914 - Flags: review?(nfroyd)
Assignee

Comment 7

6 years ago
The C++11 templates for this and the other also handle enums, but there's no way to tell that a type is an enum without <type_traits> is_enum.  Seems simplest just to tell people not to use it with enums, to me.
Attachment #759916 - Flags: review?(nfroyd)
Assignee

Comment 8

6 years ago
The URL in the ToUintWidth comment, linking to the WebKit version of this, is probably worth reading.  What's happening here is similar, but generalized to all integer widths, as ToIntWidth was.  This passes all jstests and jit-tests, so I'm reasonably confident in it.  Ideally I'd run the tests with a clang -fsanitize=undefined build, but I'm on an old laptop without clang on it, so it's not easy to do just yet.  :-\  Hopefully the new laptop will arrive so I can do that soon.
Attachment #759924 - Flags: review?(nfroyd)
Reporter

Comment 9

6 years ago
Comment on attachment 759912 [details] [diff] [review]
Alt1 - Add To mozilla::BitwiseCast<To>(From)

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

A test or two would be nice.

::: mfbt/Casting.h
@@ +22,5 @@
> + * size differences, or this might fail to compile on some but not all
> + * platforms.
> + */
> +template<typename To, typename From>
> +inline typename EnableIf<sizeof(From) == sizeof(To), To>::Type

What do you think about making this a MOZ_STATIC_ASSERT so that the compiler error messages are saner?
Attachment #759912 - Flags: review?(nfroyd) → review+
Reporter

Updated

6 years ago
Attachment #759914 - Flags: review?(nfroyd) → review+
Reporter

Comment 10

6 years ago
Comment on attachment 759914 [details] [diff] [review]
Alt2 - Implement mozilla::MakeSigned

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

::: mfbt/TypeTraits.h
@@ +528,5 @@
> + * Otherwise, the integral type of the same size as T, with the lowest rank,
> + * with T's const/volatile qualifiers, is produced.  (This basically only acts
> + * to produce signed char when T = char.)
> + *
> + * mozilla::MakeSigned<volatile int>::Type is int;

Whoops.  |volatile int| here.
Reporter

Comment 11

6 years ago
Comment on attachment 759916 [details] [diff] [review]
Alt3 - Implement mozilla::MakeUnsigned

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

::: mfbt/TypeTraits.h
@@ +597,5 @@
> + * acts to produce unsigned char when T = char.)
> + *
> + * mozilla::MakeUnsigned<volatile int>::Type is int;
> + * mozilla::MakeUnsigned<const unsigned int>::Type is const signed int;
> + * mozilla::MakeUnsigned<const char>::Type is const signed char;

These three examples should be |volatile unsigned int|, |const unsigned int|, and |const unsigned char|, respectively.
Attachment #759916 - Flags: review?(nfroyd) → review+
Reporter

Comment 12

6 years ago
Comment on attachment 759924 [details] [diff] [review]
Alt4 - Simplify the ToIntWidth implementation, akin to how WebKit implements it

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

\o/ for generalization and removing those #ifdefs.

::: js/src/vm/NumericConversions.h
@@ +46,5 @@
> +    // Extract the exponent component.  (Be careful here!  It's not technically
> +    // the exponent in NaN, infinities, and subnormals.)
> +    int_fast16_t exp =
> +        int_fast16_t((bits & mozilla::DoubleExponentBits) >> mozilla::DoubleExponentShift) -
> +        int_fast16_t(mozilla::DoubleExponentBias);

This is mozilla::ExponentComponent.  No restriction on mozilla::ExponentComponent's input values, so it can be used instead.  (May want to document the lack of restriction.)

@@ +109,5 @@
> +    return (bits & mozilla::DoubleSignBit) ? ~result + 1 : result;
> +}
> +
> +template<typename ResultType>
> +inline typename mozilla::EnableIf<mozilla::IsSigned<ResultType>::value, ResultType>::Type

Any reason to use EnableIf instead of the slightly-more user-friendly MOZ_STATIC_ASSERT(mozilla::IsSigned<ResultType>::value ...) here?

@@ +112,5 @@
> +template<typename ResultType>
> +inline typename mozilla::EnableIf<mozilla::IsSigned<ResultType>::value, ResultType>::Type
> +ToIntWidth(double d)
> +{
> +    const ResultType MaxValue = (1ULL << (CHAR_BIT * sizeof(ResultType) - 1)) - 1;

This is coming up often enough that we may just want to reimplement bits of numeric_limits.
Attachment #759924 - Flags: review?(nfroyd) → review+
Assignee

Comment 13

6 years ago
I added a couple tests for BitwiseCast, although they're slightly tricky because punning on types like that is so under-defined.

I vaguely tend to use EnableIf more than MOZ_STATIC_ASSERT because EnableIf means the overload doesn't even exist, whereas with the latter it does, just fails to compile if you use it.  In theory this helps IDEs with autocomplete and the like, but probably there aren't any IDEs smart enough to take advantage of it, so probably I shouldn't be doing it for that reason.  :-)  I actually had a couple of these switched to static-asserts post-review after a second thought, but it was hardly worth the bugspam to mention it.

I didn't use mozilla::ExponentComponent because of fears about the compiler possibly not recognizing that it could common up the two BitwiseCasts that would be involved.  In theory the acting-upon-uint64_t parts could be combined into a method and exposed, but the uses for it would be so esoteric (FloatingPoint.h and this only) that it didn't seem worth it.

Yeah, reimplementing bits of numeric_limits is probably something to do at some point.

https://hg.mozilla.org/integration/mozilla-inbound/rev/57c346bd9ec3
https://hg.mozilla.org/integration/mozilla-inbound/rev/e4473d563e0f
https://hg.mozilla.org/integration/mozilla-inbound/rev/094d54c0c9ea
https://hg.mozilla.org/integration/mozilla-inbound/rev/b9c7d27d4b43
https://hg.mozilla.org/integration/mozilla-inbound/rev/98586d2cb452

Interestingly "check" is a macro defined in OS X system headers, so the patches crashed and burned there til I renamed the "check" method in SafeCast internals.  Blech.
Whiteboard: [leave open]
Reporter

Updated

6 years ago
Attachment #756728 - Attachment is obsolete: true
Attachment #756728 - Flags: review?(jwalden+bmo)
Reporter

Updated

6 years ago
Attachment #756730 - Attachment is obsolete: true
Attachment #756730 - Flags: review?(jwalden+bmo)
Assignee

Comment 14

6 years ago
Comment on attachment 756731 [details] [diff] [review]
part 3 - change DoublePun definitions in the JS engine to use mozilla::DoublePun

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

Yeah, this stuff's out of date with the previous patches here having landed -- probably wants redoing in terms of BitwiseCast in various places.

::: js/src/vm/Xdr.h
@@ +162,5 @@
>          return true;
>      }
>  
>      bool codeDouble(double *dp) {
> +        mozilla::DoublePun pun;

Hmm, BitwiseCast will definitely make the punning clearer here, almost makes it read as if it were sane.  :-)

uint64_t bits;
if (mode == XDR_ENCODE)
    bits = BitwiseCast<uint64_t>(*dp);
if (!codeUint64(&bits))
    return false;
if (mode == XDR_DECODE)
    *dp = BitwiseCast<double>(bits);
return true;
Attachment #756731 - Flags: review?(jwalden+bmo)
Assignee: general → nobody
Assignee

Updated

2 years ago
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Whiteboard: [leave open]
Assignee: nobody → jwalden+bmo
You need to log in before you can comment on or make changes to this bug.