RFR: 8346836: C2: Introduce a way to verify the correctness of ConstraintCastNodes at runtime [v7]

[Vladimir Ivanov]

On Sat, 5 Apr 2025 06:51:34 GMT, Quan Anh Mai <qamai at openjdk.org> wrote:

>> Hi,
>> 
>> This patch adds a develop flag `VerifyConstraintCasts`, which will verify the correctness of `CastIINode`s and `CastLLNode`s at runtime and crash the VM if the dynamic value lies outside the type value range.
>> 
>> Please take a look, thanks a lot.
>
> Quan Anh Mai has updated the pull request incrementally with one additional commit since the last revision:
> 
>   make the flag diagnostic

src/hotspot/cpu/x86/c2_MacroAssembler_x86.cpp line 860:

> 858:   movl(c_rarg1, type->_lo);
> 859:   movl(c_rarg2, type->_hi);
> 860:   call(RuntimeAddress(CAST_FROM_FN_PTR(address, abort_checked_cast_int)));

Do you need to align stack pointer before the call? It may be the reason why stack traces aren't printed.

src/hotspot/cpu/x86/c2_MacroAssembler_x86.cpp line 867:

> 865: 
> 866: static void abort_checked_cast_long(jlong val, jlong lo, jlong hi) {
> 867:   fatal("Invalid CastLL, val: %lld, lo: %lld, hi: %lld", (long long)val, (long long)lo, (long long)hi);

There's `JLONG_FORMAT` to pretty-print jlongs.

src/hotspot/cpu/x86/c2_MacroAssembler_x86.hpp line 47:

> 45:   void fast_unlock_lightweight(Register obj, Register reg_rax, Register t, Register thread);
> 46: 
> 47:   void checked_cast_int(const TypeInt* type, Register val);

There's already some ambiguity around what "checked cast" really means (think of "CastPP" vs "CheckCastPP" vs "checkcast"). Let's not make it worse. I suggest to just name it as `verify_int[_in]_range`/`verify_long[_in]_range`.

Also, unpacking `type` (and pass low/upper bounds as arguments) would make code on receiving end a bit simpler.

And some invariants to assert: 
  * no constants (lo == hi);
  * no empty ranges (lo > hi).

src/hotspot/cpu/x86/x86_64.ad line 7646:

> 7644: %{
> 7645:   predicate(VerifyConstraintCasts > 0 &&
> 7646:             Assembler::is_simm32(static_cast<const CastLLNode*>(n)->type()->is_long()->_lo) &&

I suggest to extract the range check into a helper method on CastLL and call it from  `castLL_checked_L32` and `castLL_checked` predicates.

Also, `static_cast<...>(n)` can be replaced with `n->as_CastLL()`.

test/hotspot/jtreg/compiler/c2/TestVerifyConstraintCasts.java line 39:

> 37:  * @summary Run with -Xcomp to test -XX:+StressGCM -XX:VerifyConstraintCasts=2 in debug builds.
> 38:  *
> 39:  * @run main/othervm/timeout=300 -Xbatch -Xcomp -XX:+StressGCM -XX:VerifyConstraintCasts=2 compiler.c2.TestVerifyConstraintCasts

You can have multiple `@run` directives under the same `@test`. No need to duplicate it.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/22880#discussion_r2032175201
PR Review Comment: https://git.openjdk.org/jdk/pull/22880#discussion_r2032182576
PR Review Comment: https://git.openjdk.org/jdk/pull/22880#discussion_r2032208822
PR Review Comment: https://git.openjdk.org/jdk/pull/22880#discussion_r2032178151
PR Review Comment: https://git.openjdk.org/jdk/pull/22880#discussion_r2032180851