Bug ID: JDK-8279143 Undefined behaviours in globalDefinitions.hpp

Type: Bug
Component: hotspot
Sub-Component: compiler
Affected Version: 19,20,21

Priority: P4
Status: In Progress
Resolution: Unresolved

Submitted: 2021-12-22
Updated: 2024-04-10

Other
tbdUnresolved

FloatIntConv and DoubleLongConv usage lead to undefined behaviours due to type aliasing violations. The conformed way is to implement a bit cast using memcpy.

jlong_accessor usage leads to undefined behaviours due to reading from inactive members of unions. While type punning is legal in C, it is not in C++, and may break in the future. Using a struct combined with memcpy would be sufficient here.

[~dfenacci] That's not what I have in mind. None of these new files should be included by globalDefinitions.hpp. Part of the point is to reduce the weight of globalDefinitions.hpp by only having things there that are really widespread in usage, not to make the include situation around it more complex. I did some deeper excavation and prototyping, and came up with this series of changes that accomplishes those while also permitting the changes to the j<type>_cast functions to use PrimitiveConversions::cast. (1) Move TosState, as_TosState, as_BasicType to new file(s): utilities/tosState.hpp, and .inline.hpp or .cpp files if needed. New file includes utilities/globalDefinitions.hpp. Update includes to account for that move. ~50 files (2) Move JavaValue.hpp to new file(s): utilities/javaValue.hpp, and .inline.hpp or .cpp files if needed. New file includes utilities/globalDefinitions.hpp. Update includes to account for that move. ~50 files (I wondered if those two sets of files might be related, but they seem to actually be very nearly disjoint.) (3) Move BasicType and related stuff to new file(s): utilities/basicType.hpp, and .inline.hpp or .cpp files if needed. New file includes utilities/globalDefinitions.hpp. Update includes to account for that move, including updating the above files. (4) Finally, move (and reimplement) the j<type>_cast functions and other stuff related to j<type>'s to new file(s): utilities/javaTypes.hpp, and .inline.hpp or .cpp files if needed. While doing the "move", also do lots of fixing up of the code. I have a number of ideas here. Talk to me when you get to this part. Update includes to account for that move, including the above files.
10-04-2024
I've updated the draft PR (https://github.com/openjdk/jdk/pull/17832) with an attempt at the refactoring. Let me know what you think. [~kbarrett] is this more or less what you had in mind?
04-04-2024
Yes, that "alias" would be one of the changes. The "moving to their own file" would just be to start reducing the size and scope of globalDefinition.hpp, which, according to many, has become a receptacle for a lot of unrelated stuff but I still need to figure out the exact details of what that would involve.
26-02-2024
If e.g. jdouble_cast can be implemented as an "alias" for PrimitiveConversions::cast<jdouble>, then that seems far less intrusive. I'm unclear what the "moving to their own file" would actually involve.
26-02-2024
Following [~coleenp] suggestion I tried to eliminate jXXX_casts and replace them with PrimitiveConversions::cast and adapted the includes to avoid the circularity problem (draft PR https://github.com/openjdk/jdk/pull/17832). I have to admit that trying to use PrimitiveConversions::cast everywhere resulted in some awkward “contortions” (and makes the syntax definitely more error-prone). Also [~kbarrett] pointed out that this might be kind of premature and suggested a less "invasive" change that involves just moving the jfloat/jdouble stuff out of globalDefinitions, i.e. limiting the change to moving JavaValue (and possibly BasicTypes) to its own file and moving jXXX_casts to their own file along with min/max constants. Before moving in that direction I wanted to get a few more opinions on the matter: [~coleenp], [~dholmes] do you think such a solution would be OK?
23-02-2024
Moving this to JDK 22 for now. Please update the fix version if a patch is ready in time for JDK 21.
03-05-2023
Kim's comment in the discussion: https://github.com/openjdk/jdk/pull/11865#discussion_r1083761246 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
29-03-2023
Makes sense. Is the jlong_accessor fix proposed in https://github.com/openjdk/jdk/pull/6930 fine? [~dfenacci] please have a look at this when you get a chance.
29-03-2023
I agree with Coleen's assessment. But if there is a desire to keep jint_cast and friends (rather than use PrimitiveConversions::cast), then move them to their own header and reimplement them using PrimitiveConversions::cast.
28-03-2023
ILW = Undefined behavior with casts, no misbehavior observed so far?, no workaround = MLH = P4
21-03-2023
I'm reassigning this to the compiler group. If you look for jint_cast() they're mostly in the compiler (opto) source code. The recommendation is to change these directly to primitiveConversions::cast(). Making GlobalDefinitions ::jint_cast() use PrimitiveConversion::cast() runs into a header file circularity problem.
20-03-2023
int<->float and long<->double conversion is covered in JDK-8297539.
02-02-2023
Moving into hotspot/runtime for initial triage.
23-12-2021
A pull request was submitted for review. URL: https://git.openjdk.java.net/jdk/pull/6930 Date: 2021-12-23 15:58:53 +0000
23-12-2021

Relates :	JDK-8233144 - undefined behavior: signed integer overflow
Relates :	JDK-8299688 - Adopt C++14 compatible std::bit_cast introduced in C++20
Relates :	JDK-8297539 - Use PrimitiveConversions::cast for local uses of the int<->float union conversion trick