RFR: JDK-8299688: Adopt C++14 compatible std::bit_cast introduced in C++20 [v4]

[Kim Barrett]

On Fri, 13 Jan 2023 16:19:51 GMT, Justin King <jcking at openjdk.org> wrote:

>> Adopts a C++14 compatible implementation of [std::bit_cast](https://en.cppreference.com/w/cpp/numeric/bit_cast).
>> 
>> `PrimitiveConversions::cast` is refactored into `bit_cast` and extended to support all compatible types. Additionally it makes use of `__builtin_bit_cast` when available, which is strictly well defined compared to fallback approaches which are sometimes lurking in undefined behavior territory.
>> 
>> Lastly some legacy casting is updated to use `bit_cast` in `utilities/globalDefinitions.hpp`.
>
> Justin King has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Remove addressof as nobody should be overloading operator&
>   
>   Signed-off-by: Justin King <jcking at google.com>

std::bit_cast covers cases beyond what's covered by PrimitiveConversions::cast.  

Trying to be compatible means we should be covering those additional cases,
which this change is doing. However, it's not obvious those additional cases
are useful to us. The additional constraints for PrimitiveConversions::cast
also simplify the implementation (and testing).

Indeed, there are some conversions provided by std::bit_cast and this change
that I think I would prefer to actively not support. The pointer to pointer
case is an example. I don't want such potentially dangerous casts "hidden" by
using bit_cast. I'd very much prefer explicit static_cast or reinterpret_cast
as appropriate.  (I'm ambivalent about the integer<->pointer casts provided by
PrimitiveConversions::cast, now thinking they were probaby a mistake.)

And using bit_casts between pointers and floating point types?  Yuck!

At some point when we support C++20 we can consider globally replacing
PrimitiveConversions::cast with std::bit_cast, but I'm not convinced this
interim replacement should be done. Given the less restrictive conversions
supported by std::bit_cast I'm not even entirely sure I'd want to do such a
renaming. I know in the past I've suggested that as a possibility, but having
now looked at the new function more carefully, I'm not so sure.

src/hotspot/share/services/heapDumper.cpp line 1011:

> 1009:   return to;
> 1010: }
> 1011: 

I didn't know about this.  The two uses should have been using PrimitiveConversions::cast rather than adding this.

src/hotspot/share/utilities/bitCast.hpp line 41:

> 39: // Trivial implementation when To and From are the same.
> 40: template <typename To, typename From, ENABLE_IF(std::is_same<From, To>::value)>
> 41: constexpr To bit_cast(const From& from) {

This is missing the tivially copiable requirement for std::bit_cast.  Also, it seems like the is_same
SFINAE should not be needed here.  Why not just

template <typename T, ENABLE_IF(std::is_trivially_copyable<T>::value)>
constexpr T bit_cast(const T& value) {
  return value;
}

src/hotspot/share/utilities/bitCast.hpp line 46:

> 44: 
> 45: // From and To are integrals of the same size. We can simply static_cast without changing the bit
> 46: // representation.

The PrimitiveConversions version has a longer comment providing more justification for this implementation.
Please retain that.

src/hotspot/share/utilities/bitCast.hpp line 54:

> 52: constexpr To bit_cast(const From& from) {
> 53: #if HAS_BUILTIN(__builtin_bit_cast) && !defined(__APPLE__)
> 54:   return __builtin_bit_cast(To, from);

Throughout, I see no good reason to uglify the code with platform-specific conditional code
when the portable version is just fine.  And then the supporting infrastructure to define that
macro isn't needed.  The `!defined(__APPLE__)` makes this seem particularly problematic.
Is the new `HAS_BUILTIN` just broken?  Or is `__builtin_bit_cast` broken for Macs?  Or what?

src/hotspot/share/utilities/bitCast.hpp line 63:

> 61: template <typename To, typename From,
> 62:           ENABLE_IF(sizeof(To) == sizeof(From) &&
> 63:                     !std::is_same<From, To>::value &&

No need for this, since an integral type and an enum type are by definition different.  Similarly for the
overload for the other direction.

src/hotspot/share/utilities/bitCast.hpp line 191:

> 189:   return __builtin_bit_cast(To, from);
> 190: #else
> 191:   // Most modern compilers will produce optimal code for memcpy.

This comment is at odds with the comment in the preceding integer<->float conversion.  I think the
comment there is correct and this one is not.

src/hotspot/share/utilities/bitCast.hpp line 192:

> 190: #else
> 191:   // Most modern compilers will produce optimal code for memcpy.
> 192:   To to;

This default constructs To, which is potentially problematic, and is also an extra requirement.  I do see
the corresponding restriction in the enabling constraints, but that isn't part of the Standard function's
contract.  The problem can be avoided, but since I'm not sure we should be making this change at all...

src/hotspot/share/utilities/globalDefinitions.hpp line 28:

> 26: #define SHARE_UTILITIES_GLOBALDEFINITIONS_HPP
> 27: 
> 28: #include "utilities/bitCast.hpp"

I think including bitCast here is problematic.  globalDefinitions is included nearly everywhere, while
bitcasting is only needed in a few.  There is also a high risk of circular include dependencies when
adding anything to this file's include list.  It looks like this addition happens to work today, but...
And as noted elsewhere, I'm not sure some of the changes being made here to use bit_cast should
be made anyway.  And if they should, perhaps the relevant code ought to be split out of this file.

src/hotspot/share/utilities/globalDefinitions.hpp line 669:

> 667: inline jlong   jlong_cast   (jdouble x)  { return bit_cast<jlong>(x);  }
> 668: inline julong  julong_cast  (jdouble x)  { return bit_cast<julong>(x); }
> 669: inline jdouble jdouble_cast (jlong   x)  { return bit_cast<jdouble>(x);  }

I think the changes to the jXXX_cast's should not be made.  Instead, I think these should be eliminated
and the relatively few uses should be changed to use PrimitiveConversions::cast / bit_cast directly.
These functions have potentially unintended and problematic implicit conversions of the arguments.
Such a change should have it's own PR.  There's already a bug related to this:
https://bugs.openjdk.org/browse/JDK-8297539

-------------

PR: https://git.openjdk.org/jdk/pull/11865