On Mon, 12 Jan 2026 13:03:58 GMT, Volkan Yazici <[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 > > 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
