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

[Vladimir Ivanov]

On Thu, 6 Feb 2025 19:17:03 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 with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains seven additional commits since the last revision:
> 
>  - Merge branch 'master' into verifycast
>  - better comments
>  - move test to a new file, add block_comment
>  - add tests
>  - make VerifyConstraintCast uint, better debug info
>  - Merge branch 'master' into verifycast
>  - Introduce VerifyConstraintCasts

Very nice!

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

> 838: 
> 839: #ifdef ASSERT
> 840: void C2_MacroAssembler::checked_cast_int(const TypeInt* type, Register dst) {

Naming is a bit confusing here. It is a register which holds the value being range checked, not a register where new value is put.

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

> 842:   Label fail;
> 843:   Label succeed;
> 844:   cmpl(dst, type->_lo);

Optimization idea: some range checks may be redundant (when `lo`/`hi` hold min/max values).

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

> 850:   movl(rcx, type->_lo);
> 851:   movl(rdx, type->_hi);
> 852:   hlt(); // hlt so we have the stack trace

That's interesting. Sounds like a problem in `NativeStackPrinter::print_stack()`.

Speaking of debugging output, a call into a local helper function (encapsulating pretty printing logic) followed by a hlt call will do the job. But, considering the usages are in-line and quite common I suggest to make it conditional (guarded by a flag). It is possible to recover all 3 values from generated code if needed and turn on error reporting (specify the diagnostic flag) when reproducing failures.

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

> 7027: %}
> 7028: 
> 7029: instruct castLL_checked(rRegL dst, rRegL tmp, rFlagsReg cr)

Optimization idea: considering the range is statically known, `tmp` is not needed when range boundaries fit into signed int. Worth adding an extra AD instruction to benefit from that.

src/hotspot/share/opto/c2_globals.hpp line 666:

> 664:           "perform extra checks on the results of alias analysis")          \
> 665:                                                                             \
> 666:   develop(uint, VerifyConstraintCasts, 0,                                   \

Any downsides in making the flag diagnostic? It'll make it available in product builds.

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

PR Review: https://git.openjdk.org/jdk/pull/22880#pullrequestreview-2602312430
PR Review Comment: https://git.openjdk.org/jdk/pull/22880#discussion_r1946868607
PR Review Comment: https://git.openjdk.org/jdk/pull/22880#discussion_r1946871722
PR Review Comment: https://git.openjdk.org/jdk/pull/22880#discussion_r1946882298
PR Review Comment: https://git.openjdk.org/jdk/pull/22880#discussion_r1946887184
PR Review Comment: https://git.openjdk.org/jdk/pull/22880#discussion_r1946881494