On Tue, 12 Aug 2025 08:17:58 GMT, Volkan Yazici <vyaz...@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... > > Volkan Yazici has updated the pull request incrementally with one additional > commit since the last revision: > > Remove `@apiNote` in `encodeISOArray()` Javadoc > > Those who are touching to these methods should well be > aware of the details elaborated in the `@apiNote`, no > need to put it on a display. Some more parts to the precondition intrinsic story, FTR: Apart from inlining, one of the goals of the intrinsic preconditions is to allow them to more reliably interact with elimination of range checks. The JVM knows its own intrinsic checks in the `aaload` bytecode (and its brothers), and it also knows its own intrinsic precondition method. Both kinds of checks are fodder for RCE (range check elimination) and specifically the iteration range splitting performed when a loop is factored into pre/main/post loops. The main part is statically proven to traverse an index range which is incapable of failing any of the checks (within the loop body). This RCE in turn unlocks power moves like vectorization. If the precondition check in question works OK for these use cases, it can be marked force-inline, but if there is also evidence that it would unlock more loop optimizations, then it should be made an intrinsic (or else built on top of another intrinsics, with force-inline). ------------- PR Comment: https://git.openjdk.org/jdk/pull/25998#issuecomment-3189804162
