On Thu, 14 Aug 2025 21:55:48 GMT, John R Rose <[email protected]> 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
