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

[Christian Hagedorn]

On Tue, 27 Jan 2026 16:26:19 GMT, Damon Fenacci <dfenacci 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
>
> Damon Fenacci has updated the pull request incrementally with one additional commit since the last revision:
> 
>   JDK-8374852: fix star layout
>   
>   Co-authored-by: Christian Hagedorn <christian.hagedorn at oracle.com>

Thanks for unifying the two opaque nodes! I have some more comments.

src/hotspot/share/opto/macro.cpp line 2559:

> 2557: #else
> 2558:         bool is_positive = n->as_OpaqueCheck()->is_positive();
> 2559:         _igvn.replace_node(n, _igvn.intcon(is_positive?1:0));

Suggestion:

        _igvn.replace_node(n, _igvn.intcon(is_positive ? 1 : 0));

src/hotspot/share/opto/opaquenode.hpp line 146:

> 144: // builds, we keep the actual checks as additional verification code (i.e. removing OpaqueCheckNodes and use the
> 145: // BoolNode inputs instead).
> 146: class OpaqueCheckNode : public Node {

I've also thought about the name. `OpaqueCheck` is already a good indication what the node is about. Maybe we could go a step further and call it `OpaqueConstantBoolNode` to emphasize more that it is belonging to a `BoolNode` whose result we already know. What do you think?

Then we could also think about changing `_positive` to `_constant` (still can be a boolean to just pass true and false which seems more intuitive then passing in 1 and 0).

src/hotspot/share/opto/opaquenode.hpp line 148:

> 146: class OpaqueCheckNode : public Node {
> 147:  private:
> 148:   bool _positive;

Now that we define a field, we also need to override `size_of()` (see for example `OpaqueMultiversioningNode`).

src/hotspot/share/opto/opaquenode.hpp line 150:

> 148:   bool _positive;
> 149:  public:
> 150:   OpaqueCheckNode(Compile* C, Node* tst, bool positive) : Node(nullptr, tst), _positive(positive) {

`tst` is probably almost always a `BoolNode`. I'm wondering if it could also be a constant because we already folded the `BoolNode`? But then it's probably also useless to create the opaque node in the first place.

src/hotspot/share/opto/opaquenode.hpp line 159:

> 157:   virtual const Type* Value(PhaseGVN* phase) const;
> 158:   virtual const Type* bottom_type() const { return TypeInt::BOOL; }
> 159:   bool is_positive() { return _positive; }

When going with `_constant`, we could turn this into

int constant() const { return _constant ? 1 : 0; }

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

PR Review: https://git.openjdk.org/jdk/pull/29164#pullrequestreview-3715097474
PR Review Comment: https://git.openjdk.org/jdk/pull/29164#discussion_r2735306919
PR Review Comment: https://git.openjdk.org/jdk/pull/29164#discussion_r2735376625
PR Review Comment: https://git.openjdk.org/jdk/pull/29164#discussion_r2735315675
PR Review Comment: https://git.openjdk.org/jdk/pull/29164#discussion_r2735369034
PR Review Comment: https://git.openjdk.org/jdk/pull/29164#discussion_r2735392835