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

[Quan Anh Mai]

On Tue, 8 Apr 2025 00:11:04 GMT, Vladimir Ivanov <vlivanov at openjdk.org> wrote:

>> 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.

I believe C2 stacks are 16-byte aligned so we should not need to manually do the alignment?

> 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).

Thanks for the suggestions, I have changed this to `verify_int_in_range`. Expanding the `Type` before passing into the function would be really bad after #17508, though.

> 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()`.

Done, I added it to `ad_x86.hpp`

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

PR Review Comment: https://git.openjdk.org/jdk/pull/22880#discussion_r2033946419
PR Review Comment: https://git.openjdk.org/jdk/pull/22880#discussion_r2033950065
PR Review Comment: https://git.openjdk.org/jdk/pull/22880#discussion_r2033947292