On Fri, 29 May 2026 17:29:46 GMT, Vladimir Ivanov <[email protected]> wrote:
>> On bytecode level booleans are represented as ints and HotSpot JVM >> normalizes boolean values on memory accesses. It unconditionally applies >> normalization on boolean stores, but trusts on-heap boolean locations to >> hold normalized values. Normalization is applied on loads for off-heap and >> mismatched unsafe accesses . >> >> There are 2 normalization procedures used: (1) cast int to byte and test it >> against zero; and (2) truncation to least-significant bit. Truncation is >> preferred (due to performance considerations), but JNI mandates testing >> against zero and, historically, `#1` was used for off-heap unsafe accesses >> as well. It complicated the implementation (leading to subtle bugs) and >> introduced divergence in behavior at runtime (depending on execution mode >> and JIT-compilation peculiarities). >> >> The fix uses truncation uniformly across all execution modes. It simplifies >> implementation and eliminates possible divergence in behavior between >> execution modes. Also, it drastically simplifies future Unsafe API >> refactorings. >> >> There's one scenario left when it's possible to observe non-normalized >> values: when mismatched access pollutes the Java heap with a bogus boolean >> value, but then the value is read with a well-typed boolean access. >> >> Testing: hs-tier1 - hs-tier6 >> >> - [x] I confirm that I make this contribution in accordance with the >> [OpenJDK Interim AI Policy](https://openjdk.org/legal/ai). > > Vladimir Ivanov has updated the pull request incrementally with one > additional commit since the last revision: > > updates Thank you for working on this, @iwanowww! This makes boolean truncation a lot more understandable and well tested. The implementation looks good. I only have a handful of small suggestions. src/hotspot/share/opto/library_call.cpp line 2418: > 2416: bool can_access_non_heap = > TypePtr::NULL_PTR->higher_equal(_gvn.type(base)); > 2417: > 2418: assert(!is_non_heap_access || can_access_non_heap, "sanity"); // > is_non_heap_access implies can_access_non_heap Suggestion: assert(!is_non_heap_access || can_access_non_heap, "is_non_heap_access must imply can_access_non_heap"); Makes the assert a bit more descriptive. src/hotspot/share/opto/library_call.cpp line 2436: > 2434: > 2435: assert((alias_type->index() == Compile::AliasIdxRaw) == > 2436: (is_non_heap_access || (can_access_non_heap && > alias_type->field() == nullptr)), ""); A descriptive error message would be helpful. Perhaps: Suggestion: assert((alias_type->index() == Compile::AliasIdxRaw) == (is_non_heap_access || (can_access_non_heap && alias_type->field() == nullptr)), "mismatch between alias index and access location"); src/hotspot/share/opto/library_call.cpp line 2486: > 2484: } else if (type == T_BOOLEAN) { > 2485: if (mismatched || alias_type->index() == Compile::AliasIdxRaw) { > 2486: value_type = TypeInt::UBYTE; Why is this needed for mismatched or off heap accesses? src/java.base/share/classes/jdk/internal/misc/Unsafe.java line 1800: > 1798: * truncate them to least-significant bit, and then test > 1799: * against zero. It is uniformly performed for both Java heap > 1800: * and non-Java heap accesses. Suggestion: * <p>A number of Unsafe methods load boolean values as bytes, * truncate them to the least-significant bit, and then test * against zero. It is uniformly performed for both Java heap * and non-Java heap accesses. Intuitively, I feel the "the" is missing in that sentence, but take it with a grain of salt as I am not a native speaker. test/hotspot/jtreg/compiler/unsafe/UnsafeBooleanTest.java line 125: > 123: Short.MIN_VALUE, Short.MAX_VALUE, > 124: Character.MIN_VALUE , Character.MAX_VALUE, > 125: Integer.MIN_VALUE, Integer.MAX_VALUE }; I think it would be a good idea to add a random int input as well for additional coverage at least for the non-constant tests. ------------- Changes requested by mhaessig (Committer). PR Review: https://git.openjdk.org/jdk/pull/31249#pullrequestreview-4399335995 PR Review Comment: https://git.openjdk.org/jdk/pull/31249#discussion_r3332628625 PR Review Comment: https://git.openjdk.org/jdk/pull/31249#discussion_r3332677704 PR Review Comment: https://git.openjdk.org/jdk/pull/31249#discussion_r3332710578 PR Review Comment: https://git.openjdk.org/jdk/pull/31249#discussion_r3332719480 PR Review Comment: https://git.openjdk.org/jdk/pull/31249#discussion_r3332468053
