RFR: 8361842: Move input validation checks to Java for java.lang.StringCoding intrinsics [v15]

Fri Aug 15 08:39:24 UTC 2025

On Thu, 14 Aug 2025 21:55:48 GMT, John R Rose <jrose at openjdk.org> wrote:

>> 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.
>
> src/hotspot/share/opto/library_call.cpp line 946:
> 
>> 944:                                                  Node* count,
>> 945:                                                  bool char_count,
>> 946:                                                  bool halt_on_oob) {
> 
> Adding halt_on_oob is a pro move.  It makes it very clear that something very bad is afoot.
> 
> Now I see why A/B testing was harder to do in the jtreg tests, especially for the "throwing" case.
> 
> Just a note for the future (no change requested):  The InternalError exception is sometimes used for these kinds of "crash and burn" conditiosn.   (See the code in java.lang.invoke.)  Users can try to catch IE, but they are advised not to, and it will therefore crash the thread (at least), as an unhandled throw.  The advantage of using IE is that a jtreg harness can catch it much more easily (as opposed to a hard VM halt).

Thanks for the remark. I will share this with @dafedafe and @TobiHartmann.

> src/java.base/share/classes/sun/nio/cs/ISO_8859_1.java line 159:
> 
>> 157:         private static int encodeISOArray(char[] sa, int sp,
>> 158:                                           byte[] da, int dp, int len) {
>> 159:             if ((sp | dp | len) < 0 ||
> 
> I think this change is OK; I understand it removes uncertainty about inlining of the checking helper method.
> 
> In general, though, I think we should be using the Preconditions methods, instead of hand-rolling checks from random-looking Java operators.  (Sorry, but they will look random to the person reading this years later!)
> 
> (I think Objects::rNN is overkill; I do believe that the implicit null checks are easy to reason about.  I guess tastes vary here.  O::rNN is marked force-inline, so that should not be a problem to throw it into the checking logic, as an explicit null check.)
> 
> Anyway, I would like to a see a comment here, in the code, saying why the Preconditions methods are not used at this point.  If there was an inlining problem, let's acknowledge it.  It's OK to make String code be a special case (with hand-rolled checks, carefully tuned).  Let's document how we did that.
> 
> One point:  I don't think we expect these bulk string operations to occur in a containing hot loop, so the intrinsic value of Precondition methods (to contribute to RCE optimizations) is not required.  Still, on the whole, it is usually better to use a named method than a tricky Java operator expression – much as we all love those!
> 
> P.S. For regular code (not the very hottest hand-optimized string methods) I do prefer to put the checking logic in a separate method.  This is (again) a matter of taste.  The point is to make the method that does the work be as compact as possible, preferably below about 35 bytecodes (which, sadly, is favored by our inlining policy ATM).  If the method which does the work is 10% work-doing bytecodes and 90% condition-checking and exception-formatting and exception-throwing code, then the JIT will get a bad view of it, and will not inline as generously.  In fact, I often prefer to make the checks method small also, and save all the exception reporting junk for a THIRD method, which is never called, and does not need to be inlined at all.  So, for me, the sweet spot is one small method to do the work, another small method to do the checks (perhaps force-inline, if it is medium sized), and a third method to throw the exception, which is as horrible as it needs to be.

Very valid point. In 62350347, documented why `Preconditions` is not used.

> test/hotspot/jtreg/compiler/intrinsics/TestVerifyIntrinsicChecks.java line 90:
> 
>> 88:     }
>> 89: 
>> 90:     private static void violateIntrinsicMethodContract() {
> 
> Ah, here is the positive test for the extra range checking in mode +VerifyIntrinsicChecks.
> In the other place where I put a comment, maybe point at this test (in the comment)?

Yes, and no. Yes, this test verifies mode `+VerifyIntrinsicChecks` employed by _a_ VM intrinsic that triggers a VM halt on constraint violation – but only that. No, it doesn't _exhaustively_ verify extra range checks of `StringCoding::encodeAsciiArray` in mode `+VerifyIntrinsicChecks`.

Since `SC:eAA` is just used as a vehicle to reach to a VM halt triggered by `halt_on_oob = true`, I prefer to _not_ refer to this test as the positive test for the extra range checking in mode `+VerifyIntrinsicChecks` of `TestEncodeIntrinsics`, which tests `StringCoding::encodeAsciiArray`.

> test/hotspot/jtreg/compiler/intrinsics/string/TestCountPositives.java line 56:
> 
>> 54:  * @summary Verify `StringCoding::countPositives` intrinsic Java wrapper checks
>> 55:  *          by enabling the ones in the compiler intrinsic using
>> 56:  *          `-XX:+VerifyIntrinsicChecks`
> 
> Is this only a negative test for the optional extra range checks?
> 
> That is to say, do we win if there are no additional throws, when all index inputs are valid (in range)?
> 
> Or is there some corner of these tests (there are three files here) which intentionally instigates out-of-range errors, by passing out-of-range index inputs, and then makes sure that the expected exceptions occur?
> 
> It would be good to put in a brief comment saying, "This test does not trigger out-of-range errors.  The +VerifyIntrinsicChecks version of this test simply ensures that the assembly code will produce no spurious errors."
> 
> But it would be nice, someday, to test lots of edge conditions which ARE out-of-range, and make sure (again) that the behavior is the same with and without the +VerifyIntrinsicChecks mode.

Your assessment is correct. In 2ba4ba6f, I've added `@comment` blocks as requested.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/25998#discussion_r2278536177
PR Review Comment: https://git.openjdk.org/jdk/pull/25998#discussion_r2278534999
PR Review Comment: https://git.openjdk.org/jdk/pull/25998#discussion_r2278535750
PR Review Comment: https://git.openjdk.org/jdk/pull/25998#discussion_r2278535192