JDK-8299688: Adopt C++14 compatible std::bit_cast introduced in C++20 #11865

jcking · 2023-01-05T18:04:29Z

Adopts a C++14 compatible implementation of std::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.

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue

Issue

JDK-8299688: Adopt C++14 compatible std::bit_cast introduced in C++20

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk pull/11865/head:pull/11865
$ git checkout pull/11865

Update a local copy of the PR:
$ git checkout pull/11865
$ git pull https://git.openjdk.org/jdk pull/11865/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 11865

View PR using the GUI difftool:
$ git pr show -t 11865

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/11865.diff

Signed-off-by: Justin King <jcking@google.com>

bridgekeeper · 2023-01-05T18:05:45Z

👋 Welcome back jcking! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

…_cast Signed-off-by: Justin King <jcking@google.com>

openjdk · 2023-01-05T18:10:17Z

@jcking The following label will be automatically applied to this pull request:

hotspot

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

Signed-off-by: Justin King <jcking@google.com>

mlbridge · 2023-01-05T18:40:39Z

Webrevs

Signed-off-by: Justin King <jcking@google.com>

stefank

Leaving this review for Kim ... but saw a few nits that could be fixed in the meantime.

src/hotspot/os_cpu/linux_ppc/atomic_linux_ppc.hpp

src/hotspot/os_cpu/windows_aarch64/atomic_windows_aarch64.hpp

Signed-off-by: Justin King <jcking@google.com>

jcking · 2023-01-17T15:57:14Z

@kimbarrett Friendly poke. Benefit is when we eventually get to C++20 we can just do using std::bit_cast.

kimbarrett

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/utilities/bitCast.hpp

kimbarrett · 2023-01-23T07:37:49Z

src/hotspot/share/utilities/bitCast.hpp

+  return __builtin_bit_cast(To, from);
+#else
+  // Most modern compilers will produce optimal code for memcpy.
+  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...

Removed the requirement, since I am strictly only supported what PrimitiveConversions::cast supported for now.

kimbarrett · 2023-01-23T07:41:38Z

src/hotspot/share/utilities/bitCast.hpp

+                    std::is_integral<To>::value)>
+constexpr To bit_cast(const From& from) {
+#if HAS_BUILTIN(__builtin_bit_cast) && !defined(__APPLE__)
+  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?

I also don't like the direct call to __builtins either, gcc to my knowledge automatically replaces the appropriate builtins when optimization is specified

Apple Clang seemed to have a bug with the version we are using in GHA. I bumped them to 12.5.1 and the error disappeared.

__builtin_bit_cast is used to implement std::bit_cast for Clang/GCC. It is purely a compiler construct IIRC and has no runtime function equivalent. It's always replaced with machine code for performing a byte-wise copy and ensures no shenanigans occur. Additionally std::bit_cast is the only way to convert between floating point and integral without invoking undefined behavior, though the union trick does currently work. The other option is std::memcpy.

I know what __builtin_bit_cast is. I don't think we need to use it, so shouldn't uglify the code to do so.
There is also a risk of different semantics depending whether the builtin is used or not. For example,
std::bit_cast between a floating point type and an integral type is constexpr, so presumably the builtin
is similarly constexpr. But the non-builtin implementation isn't.

src/hotspot/share/utilities/bitCast.hpp

kimbarrett · 2023-01-23T07:44:10Z

src/hotspot/share/utilities/bitCast.hpp

+
+// Trivial implementation when To and From are the same.
+template <typename To, typename From, ENABLE_IF(std::is_same<From, To>::value)>
+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; }

The need for this overload to match the Standard specification is one of the reasons why I had refrained
from doing something like this change. This overload's existence (slightly) complicates others by requiring
an additional !std::is_same<To, From> constraint. Since such a nop conversion hasn't arisen in our code
(yet), there isn't currently a need for this. I also don't see any tests for it.

I removed the is_same. It technically isn't needed to conform. I also decided to not conform to the full standard and restrict uses based on the existing PrimitiveConversions::cast restrictions.

src/hotspot/share/utilities/bitCast.hpp

kimbarrett · 2023-01-23T08:21:03Z

src/hotspot/share/services/heapDumper.cpp

-  memcpy(&to, &from, sizeof(to));
-  return to;
-}
-


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

kimbarrett · 2023-01-23T08:34:30Z

src/hotspot/share/utilities/globalDefinitions.hpp

-inline jdouble jdouble_cast (jlong   x)  { return ((DoubleLongConv*)&x)->d;  }
+inline jlong   jlong_cast   (jdouble x)  { return bit_cast<jlong>(x);  }
+inline julong  julong_cast  (jdouble x)  { return bit_cast<julong>(x); }
+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

Reverted changes to globalDefinitions.hpp.

kimbarrett · 2023-01-23T09:59:12Z

src/hotspot/share/utilities/globalDefinitions.hpp

@@ -25,6 +25,7 @@
 #ifndef SHARE_UTILITIES_GLOBALDEFINITIONS_HPP
 #define SHARE_UTILITIES_GLOBALDEFINITIONS_HPP

+#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.

Reverted changes to globalDefinitions.hpp.

Signed-off-by: Justin King <jcking@google.com>

kimbarrett · 2023-02-04T22:32:39Z

src/hotspot/share/utilities/bitCast.hpp

+                    std::is_integral<To>::value)>
+constexpr To bit_cast(const From& from) {
+#if HAS_BUILTIN(__builtin_bit_cast) && !defined(__APPLE__)
+  return __builtin_bit_cast(To, from);


I know what __builtin_bit_cast is. I don't think we need to use it, so shouldn't uglify the code to do so.
There is also a risk of different semantics depending whether the builtin is used or not. For example,
std::bit_cast between a floating point type and an integral type is constexpr, so presumably the builtin
is similarly constexpr. But the non-builtin implementation isn't.

kimbarrett · 2023-02-04T22:38:49Z

src/hotspot/share/utilities/bitCast.hpp

+//
+// Casts from type From to type To without changing the underlying bit representation. This is
+// partially compatible with std::bit_cast introduced in C++20, but is more restrictive on the type
+// of conversions allowed.


If it's not compatible (or at least trying to be, in so far as possible within the restrictions we operate under)
with std::bit_cast (and I've suggested there are reasons why we shouldn't be compatible), then using the
name bit_cast seems like a mistake that will only cause confusion.

PrimitiveConversions::cast might not be the best name either (John Rose certainly expressed dislike), but
discussing a name change is probably better done outside the context of a PR.

In the meantime, I don't think this part of the change should be made. There are various places that should
be using PrimitiveConversions::cast (under whatever name is ultimately chosen) but aren't, and they can be
changed to do so. Later, once we've picked the final name, we can do a global rename.

kimbarrett · 2023-02-04T22:43:30Z

.github/workflows/main.yml

@@ -235,7 +235,7 @@ jobs:
    uses: ./.github/workflows/build-macos.yml
    with:
      platform: macos-x64
-      xcode-toolset-version: '11.7'
+      xcode-toolset-version: '12.5.1'


This and the other toolchain update proposed here are to avoid the
mentioned bug in Apple Clang, so that __builtin_bit_cast can be used
with that compiler. Since I don't think we should be using the
builtin, I don't think this is needed either. There may be other
reasons to move the GHA toolchain version forward, but that should be
a separate discussion.

jcking added 10 commits January 5, 2023 07:10

Replace PrimitiveConversions::cast with C++14 compatible std::bit_cast

3ba1d56

Signed-off-by: Justin King <jcking@google.com>

Update copyright header

9b55d0c

Signed-off-by: Justin King <jcking@google.com>

Remove <memory> include and fix another include

f2122c1

Signed-off-by: Justin King <jcking@google.com>

Add additional test cases

3ba7537

Signed-off-by: Justin King <jcking@google.com>

Use bit_cast in utilities/globalDefinitions.hpp

dff628d

Signed-off-by: Justin King <jcking@google.com>

Do not use bit_cast for function pointer conversion

34608fa

Signed-off-by: Justin King <jcking@google.com>

Fix interdependency

39d9477

Signed-off-by: Justin King <jcking@google.com>

Fix interdependency

24ebcbf

Signed-off-by: Justin King <jcking@google.com>

Ensure operator& overloading is ignored

bcb53fa

Signed-off-by: Justin King <jcking@google.com>

Minor cleanup

77943bf

Signed-off-by: Justin King <jcking@google.com>

Use __builtin_bit_cast when available for every specialization of bit…

d64d281

…_cast Signed-off-by: Justin King <jcking@google.com>

openjdk bot added the hotspot hotspot-dev@openjdk.org label Jan 5, 2023

Correctly handle const/volatile pointers

4cff62b

Signed-off-by: Justin King <jcking@google.com>

jcking marked this pull request as ready for review January 5, 2023 18:34

openjdk bot added the rfr Pull request is ready for review label Jan 5, 2023

Apple Clang thinks __builtin_bit_cast is present when its not

281a915

Signed-off-by: Justin King <jcking@google.com>

stefank reviewed Jan 9, 2023

View reviewed changes

src/hotspot/os_cpu/linux_ppc/atomic_linux_ppc.hpp Outdated Show resolved Hide resolved

src/hotspot/os_cpu/windows_aarch64/atomic_windows_aarch64.hpp Outdated Show resolved Hide resolved

jcking added 2 commits January 9, 2023 07:40

Sort includes and fix macro indentation

e592125

Signed-off-by: Justin King <jcking@google.com>

Remove addressof as nobody should be overloading operator&

ccb8f28

Signed-off-by: Justin King <jcking@google.com>

kimbarrett reviewed Jan 23, 2023

View reviewed changes

jcking added 6 commits January 23, 2023 07:48

Restrict supported use cases

30675c5

Signed-off-by: Justin King <jcking@google.com>

Restrict supported use cases

5547e9e

Signed-off-by: Justin King <jcking@google.com>

Merge remote-tracking branch 'upstream/master' into bit-cast

07648dd

Rollback changes to globalDefinitions.hpp

b9fed0f

Signed-off-by: Justin King <jcking@google.com>

Fix pointer casts

f66bc62

Signed-off-by: Justin King <jcking@google.com>

ALWAYSINLINE

9f245c9

Signed-off-by: Justin King <jcking@google.com>

jcking added 2 commits January 23, 2023 11:03

Restore comment regarding integral casting

ec85baf

Signed-off-by: Justin King <jcking@google.com>

Documentation updates

73b87d2

Signed-off-by: Justin King <jcking@google.com>

kimbarrett suggested changes Feb 4, 2023

View reviewed changes

jcking closed this Feb 7, 2023

dafedafe mentioned this pull request Feb 13, 2024

8279143: Undefined behaviours in globalDefinitions.hpp #17832

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

JDK-8299688: Adopt C++14 compatible std::bit_cast introduced in C++20 #11865

JDK-8299688: Adopt C++14 compatible std::bit_cast introduced in C++20 #11865

jcking commented Jan 5, 2023 •

edited by openjdk bot

Loading

bridgekeeper bot commented Jan 5, 2023

openjdk bot commented Jan 5, 2023

mlbridge bot commented Jan 5, 2023 •

edited

Loading

stefank left a comment

jcking commented Jan 17, 2023

kimbarrett left a comment

kimbarrett Jan 23, 2023

jcking Jan 23, 2023

kimbarrett Jan 23, 2023

TheShermanTanker Jan 23, 2023

jcking Jan 23, 2023

kimbarrett Feb 4, 2023

kimbarrett Jan 23, 2023

kimbarrett Jan 23, 2023

jcking Jan 23, 2023

kimbarrett Jan 23, 2023

kimbarrett Jan 23, 2023

jcking Jan 23, 2023

kimbarrett Jan 23, 2023

jcking Jan 23, 2023

kimbarrett Feb 4, 2023

kimbarrett Feb 4, 2023

kimbarrett Feb 4, 2023

JDK-8299688: Adopt C++14 compatible std::bit_cast introduced in C++20 #11865

JDK-8299688: Adopt C++14 compatible std::bit_cast introduced in C++20 #11865

Conversation

jcking commented Jan 5, 2023 • edited by openjdk bot Loading

Progress

Issue

Reviewing

bridgekeeper bot commented Jan 5, 2023

openjdk bot commented Jan 5, 2023

mlbridge bot commented Jan 5, 2023 • edited Loading

Webrevs

stefank left a comment

Choose a reason for hiding this comment

jcking commented Jan 17, 2023

kimbarrett left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcking commented Jan 5, 2023 •

edited by openjdk bot

Loading

mlbridge bot commented Jan 5, 2023 •

edited

Loading