On Mon, 2 Feb 2026 09:55:37 GMT, Damon Fenacci <[email protected]> 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-8374582: revert wrong copyright change Thanks for working on this Damon. I added a few comments, otherwise it looks good! src/hotspot/share/opto/library_call.cpp line 894: > 892: > 893: inline Node* LibraryCallKit::generate_negative_guard(Node* index, > RegionNode* region, > 894: Node** pos_index, > bool is_opaque) { As we discussed offline, I think `with_opaque` is better here. src/hotspot/share/opto/opaquenode.hpp line 145: > 143: // with false in product builds such that the actually unneeded guards > are folded and do not end up in the emitted code. > 144: // In debug builds, we keep the actual checks as additional verification > code (i.e. removing OpaqueConstantBoolNodes and > 145: // use the BoolNode inputs instead). Nice comment! src/hotspot/share/opto/opaquenode.hpp line 148: > 146: class OpaqueConstantBoolNode : public Node { > 147: private: > 148: bool _constant; Should this be `const`? src/hotspot/share/opto/opaquenode.hpp line 150: > 148: bool _constant; > 149: public: > 150: OpaqueConstantBoolNode(Compile* C, Node* tst, bool constant) : > Node(nullptr, tst), _constant(constant) { An alternative would be to have the `constant` be an actual input node instead of a field. In macro expansion, you could then do `_igvn.replace_node(n, n->in(2));` instead (maybe define an enum for the input indices). I don't have a strong opinion on this though and leave it up to you to decide 🙂 ------------- Marked as reviewed by thartmann (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/29164#pullrequestreview-3738450475 PR Review Comment: https://git.openjdk.org/jdk/pull/29164#discussion_r2753636949 PR Review Comment: https://git.openjdk.org/jdk/pull/29164#discussion_r2753548067 PR Review Comment: https://git.openjdk.org/jdk/pull/29164#discussion_r2753551976 PR Review Comment: https://git.openjdk.org/jdk/pull/29164#discussion_r2753586409
