On Wed, 14 Jan 2026 09:38:40 GMT, Volkan Yazici <[email protected]> wrote:
>> src/hotspot/share/opto/library_call.hpp line 170:
>>
>>> 168: Node* length, bool char_count,
>>> 169: bool halt_on_oob = false,
>>> 170: bool is_opaque = false);
>>
>> Do we really need to introduce two new toggles: `halt_on_oob` and
>> `is_opaque`? At all call-sites either one of the following is used:
>>
>> 1. `halt_on_oob=true, is_opaque=!VerifyIntrinsicChecks`
>> 2. defaults (i.e., `halt_on_oob=is_opaque=false`)
>>
>> Can we instead only settle one, e.g., `halt_on_oob=VerifyIntrinsicChecks`?
>
> Giving this a second thought, do we need these two flags anyway? That is,
>
> 1. We can remove `if (is_opaque)` add the `OpaqueGuard` anyway, since it is
> ineffective for `!ASSERT`. (This is what `must_be_not_null` does too.)
> 2. We can replace `if (halt_on_oob) { ... } else { ... }` with `#ifdef ASSERT
> ...`.
1. I'm not sure we can always do that:
`LibraryCallKit::generate_string_range_check` is called from places that don't
yet have Java range checks and we must not add an opaque node in those cases
(or we end up without checks in prod builds).
2. For a similar reason I'd leave `if (halt_on_oob)` condition: for calls to
`LibraryCallKit::generate_string_range_check` that don't yet have Java range
checks the method behaves like it did before. For "new" calls it adds the
`Halt` node (which will then be removed together with the guard in prod builds).
So, on the one hand we can keep `halt_on_oob` alone as discriminant between
"new" and "old" call sites. On the other we can get rid of
`VerifyIntrinsicChecks` because we implicitly add the additional range check in
debug builds (always).
I've modified the code accordingly. What do you think?
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/29164#discussion_r2694787303
