RFR: 8361842: Move input validation checks to Java for String-related intrinsics

[Damon Fenacci]

On Thu, 26 Jun 2025 10:48:37 GMT, Volkan Yazici <vyazici at openjdk.org> wrote:

> Validate input in `java.lang.StringCoding` intrinsic Java wrappers, improve their documentation, enhance the checks in the associated IR or assembly code, and adapt them to cause VM crash on invalid input.
> 
> ## Implementation notes
> 
> The goal of the associated umbrella issue [JDK-8156534](https://bugs.openjdk.org/browse/JDK-8156534) is to, for `java.lang.String*` classes,
> 
> 1. Move `@IntrinsicCandidate`-annotated `public` methods<sup>1</sup> (in Java code) to `private` ones, and wrap them with a `public` ["front door" method](https://github.com/openjdk/jdk/pull/24982#discussion_r2087493446)
> 2. Since we moved the `@IntrinsicCandidate` annotation to a new method, intrinsic mappings – i.e., associated `do_intrinsic()` calls in `vmIntrinsics.hpp` – need to be updated too
> 3. Add necessary input validation (range, null, etc.) checks to the newly created public front door method
> 4. Place all input validation checks in the intrinsic code (add if missing!) behind a `VerifyIntrinsicChecks` VM flag
> 
> Following preliminary work needs to be carried out as well:
> 
> 1. Add a new `VerifyIntrinsicChecks` VM flag
> 2. Update `generate_string_range_check` to produce a `HaltNode`.  That is, crash the VM if `VerifyIntrinsicChecks` is set and a Java wrapper fails to spot an invalid input.
> 
> <sup>1</sup>  `@IntrinsicCandidate`-annotated constructors are not subject to this change, since they are a special case.
> 
> ## Functional and performance tests
> 
> - `tier1` (which includes `test/hotspot/jtreg/compiler/intrinsics/string`) passes on several platforms. Further tiers will be executed after integrating reviewer feedback.
> 
> - Performance impact is still actively monitored using `test/micro/org/openjdk/bench/java/lang/String{En,De}code.java`, among other tests. If you have suggestions on benchmarks, please share in the comments.
> 
> ## Verification of the VM crash
> 
> I've tested the VM crash scenario as follows:
> 
> 1. Created the following test program:
> 
> public class StrIntri {
>     public static void main(String[] args) {
>         Exception lastException = null;
>         for (int i = 0; i < 1_000_000; i++) {
>             try {
>                 jdk.internal.access.SharedSecrets.getJavaLangAccess().countPositives(new byte[]{1,2,3}, 2, 5);
>             } catch (Exception exception) {
>                 lastException = exception;
>             }
>         }
>         if (lastException != null) {
>             lastException.printStackTrace();
>         } else {
>             System.out.println("completed");
>         }
>     }
> }
> ...

Thanks a lot for looking into this Volkan!
I left a couple of minor comments.
I also noticed that you haven't yet added the benchmark results to the description: do you want to run them again after the reviews?

src/hotspot/cpu/x86/macroAssembler_x86.cpp line 6014:

> 6012: }
> 6013: 
> 6014: // Encode given `char[]`/`byte[]` to `byte[]` in ISO_8859_1 or ASCII

Thanks for updating the comments! Do we need the markdown back quotes here (and in other `macroAssembler-*` file comments)?

src/hotspot/share/classfile/vmIntrinsics.hpp line 417:

> 415:                                                                                                                         \
> 416:   do_class(java_lang_StringCoding,        "java/lang/StringCoding")                                                     \
> 417:   do_intrinsic(_countPositives,           java_lang_StringCoding, countPositives_name, countPositives_signature,   F_S) \

It is a matter of taste but it might be better not to change the whitespaces (it might make searching for changes (and possibly backports) harder. The rest of the file is not too consistent anyway).

src/hotspot/share/opto/c2_globals.hpp line 666:

> 664:                                                                             \
> 665:   develop(bool, VerifyIntrinsicChecks, false,                               \
> 666:           "Verify that Java level checks in intrinsics work as expected")   \

To make it clearer I think we might want to move "in intrinsic" after "Verify" or at the end.

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

PR Review: https://git.openjdk.org/jdk/pull/25998#pullrequestreview-3015376813
PR Review Comment: https://git.openjdk.org/jdk/pull/25998#discussion_r2204312397
PR Review Comment: https://git.openjdk.org/jdk/pull/25998#discussion_r2204505403
PR Review Comment: https://git.openjdk.org/jdk/pull/25998#discussion_r2204256430