RFR: 8374582: [REDO] Move input validation checks to Java for java.lang.StringCoding intrinsics

Tue Jan 20 18:54:10 UTC 2026

On Mon, 12 Jan 2026 13:03:58 GMT, Volkan Yazici <vyazici at openjdk.org> wrote:

>> ## Issue
>> 
>> This is a redo of [JDK-8361842](https://bugs.openjdk.org/browse/JDK-8361842) which was backed out by [JDK-8374210](https://bugs.openjdk.org/browse/JDK-8374210) due to C2-related regressions. The original change moved input validation checks for java.lang.StringCoding from the intrinsic to Java code (leaving the intrinsic check only with the `VerifyIntrinsicChecks` flag). Refer to the [original PR](https://github.com/openjdk/jdk/pull/25998) for details.
>> 
>> This additional issue happens because, in some cases, for instance when the Java checking code is not inlined and we give an out-of-range constant as input, we fold the data path but not the control path and we crash in the backend.
>> 
>> ## Causes
>> 
>> The cause of this is that the out-of-range constant (e.g. -1) floats into the intrinsic and there (assuming the input is valid) we add a constraint to its type to positive integers (e.g. to compute the array address) which makes it top.
>> 
>> ## Fix
>> 
>> A possible fix is to introduce an opaque node (OpaqueGuardNode) similar to what we do in `must_be_not_null` for values that we know cannot be null:
>> https://github.com/openjdk/jdk/blob/ce721665cd61d9a319c667d50d9917c359d6c104/src/hotspot/share/opto/graphKit.cpp#L1484
>> This will temporarily add the range check to ensure that C2 figures that out-of-range values cannot reach the intrinsic. Then, during macro expansion, we replace the opaque node with the corresponding constant (true/false) in product builds such that the actually unneeded guards are folded and do not end up in the emitted code.
>> 
>> # Testing
>> 
>> * Tier 1-3+
>> * 2 JTReg tests added
>>   * `TestRangeCheck.java` as regression test for the reported issue
>>   * `TestOpaqueGuardNodes.java` to check that opaque guard nodes are added when parsing and removed at macro expansion
>
> Verified that 3c466d372b7 is a clean revert of 7e18de137c3 delivered in [JDK-8374210].
> 
> [JDK-8374210]: https://bugs.openjdk.org/browse/JDK-8374210

Thanks for your review @vy.
In addition to the changes you suggested I also fixed the opaque node value in `LibraryCallKit::generate_limit_guard` which was wrong (I then removed the `is_positive` flag altogether since it was `false` in both cases) and added `TestOpaqueGuardNodes.java` to test that the opaque nodes are added and later removed.

> src/hotspot/share/opto/c2_globals.hpp line 680:
> 
>> 678:   develop(bool, VerifyIntrinsicChecks, false,                               \
>> 679:           "Verify in intrinsic that Java level checks work as expected")    \
>> 680:                                                                             \
> 
> I suggest removing the `VerifyIntrinsicChecks` flag. Given `OpaqueGuard` already verifies the value when `#ifdef ASSERT`, does `VerifyIntrinsicChecks` serve any purpose anymore?

Done.

> src/hotspot/share/opto/loopopts.cpp line 1:
> 
>> 1: /*
> 
> What is the reason that the new `OpaqueGuard` is not taken into account in `PhaseIdealLoop::clone_iff`?

Oversight 😉 Thanks! Fixed.

> src/hotspot/share/opto/macro.cpp line 2565:
> 
>> 2563:         // Tests with OpaqueGuard nodes are implicitly known to be true or false. Replace the node with appropriate value. In debug builds,
>> 2564:         // we leave the test in the graph to have an additional sanity check at runtime. If the test fails (i.e. a bug),
>> 2565:         // we will execute a Halt node.
> 
> *Nit:* Can we adhere to the max. 120 (or even better, 80!) characters per line limit of the file?

Fair enough (good to know: I wasn't aware of such limit).

> src/hotspot/share/opto/macro.cpp line 2569:
> 
>> 2567:         _igvn.replace_node(n, n->in(1));
>> 2568: #else
>> 2569:         _igvn.replace_node(n, _igvn.intcon(0));
> 
> Curious: why do we invoke `intcon(0)` for `OpaqueGuard`, whereas it was `intcon(1)` for `OpaqueNotNull` slightly above?

In `OpaqueGuard`'s case we know that the input is always "false" (so, we set 0 as its input). For `OpaqueNotNull` we know that the input is always "true" (so, we set 1 as its input).

> src/hotspot/share/opto/opaquenode.hpp line 160:
> 
>> 158: // we keep the actual checks as additional verification code (i.e. removing OpaqueGuardNode and use the BoolNode
>> 159: // inputs instead).
>> 160: class OpaqueGuardNode : public Node {
> 
> With the `OpaqueGuardNode::is_positive` flag gone, `OpaqueGuardNode` looks pretty much identical to `OpaqueNotNullNode`. Is there a code reuse opportunity we can take advantage of?

It is true that they do pretty much the same thing ("avoid" C2 optimisations for checks) but I'd argue they are semantically slightly different: one prevents optimisations where we know the value cannot be null, the other where we know the value is in range. We could actually have only one class (e.g. with a `positive` flag like before) but I'm not sure it would be a cleaner/nicer solution. 🤔

> test/hotspot/jtreg/compiler/intrinsics/TestVerifyIntrinsicChecks.java line 1:
> 
>> 1: /*
> 
> Since the `VerifyIntrinsicChecks` flag is gone, AFAICT, all following changes can be reverted:
> 
> 
> git rm test/hotspot/jtreg/compiler/intrinsics/TestVerifyIntrinsicChecks.java
> git checkout upstream/HEAD -- \
> test/hotspot/jtreg/compiler/intrinsics/string/TestCountPositives.java \
> test/hotspot/jtreg/compiler/intrinsics/string/TestEncodeIntrinsics.java \
> test/hotspot/jtreg/compiler/intrinsics/string/TestHasNegatives.java \
> test/hotspot/jtreg/compiler/patches/java.base/java/lang/Helper.java

Totally. Done.

> test/hotspot/jtreg/compiler/intrinsics/string/TestRangeCheck.java line 32:
> 
>> 30:  *      -XX:CompileCommand=inline,java.lang.StringCoding::*
>> 31:  *      -XX:CompileCommand=exclude,jdk.internal.util.Preconditions::checkFromIndexSize
>> 32:  *      -XX:CompileCommand=compileonly,compiler.intrinsics.string.TestRangeCheck::test
> 
> Is this necessary? (This wasn't used in `TestStringConstruction`.)

Nope (leftover from debugging). Removed

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

PR Comment: https://git.openjdk.org/jdk/pull/29164#issuecomment-3755585217
PR Review Comment: https://git.openjdk.org/jdk/pull/29164#discussion_r2694787876
PR Review Comment: https://git.openjdk.org/jdk/pull/29164#discussion_r2694785777
PR Review Comment: https://git.openjdk.org/jdk/pull/29164#discussion_r2694785429
PR Review Comment: https://git.openjdk.org/jdk/pull/29164#discussion_r2707280040
PR Review Comment: https://git.openjdk.org/jdk/pull/29164#discussion_r2707283139
PR Review Comment: https://git.openjdk.org/jdk/pull/29164#discussion_r2707272331
PR Review Comment: https://git.openjdk.org/jdk/pull/29164#discussion_r2694788432