On Thu, 14 Aug 2025 21:55:48 GMT, John R Rose <jr...@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