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

[Emanuel Peter]

On Mon, 20 Jan 2025 01:27:18 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:
> 
>   move test to a new file, add block_comment

Just a few more comments :)

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

> 670:           "have more verification coverage")                                \
> 671:           range(0, 2)                                                       \
> 672:                                                                             \

Nit: I think the alignment could be a little improved, maybe also the comments.
Here a quick draft:

Suggestion:

  develop(uint, VerifyConstraintCasts, 0,                                   \
          "Perform runtime checks to verify the value of a ConstraintCast " \
          "lies inside its type"                                            \
          "0: No verification."                                             \
          "1: Verify (types are widened, same as without verification)."  \
          "2: Verify, where types are not widened, for better  " \
          "   verification coverage"). \
          range(0, 2)                                                       \
                                                                            \

I'm too lazy to fix the alignment of the ``. This here is just a suggestion, I leave it up to you in the end ;)

src/hotspot/share/opto/castnode.cpp line 528:

> 526:   }
> 527: 
> 528:   // Keep these casts for verification

You could write about `VerifyConstraintCasts` with its 2 modes, and say why they are both useful.

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

Suggestion:

 * @run main/othervm/timeout=300 -Xbatch -Xcomp -XX:VerifyConstraintCasts=2 compiler.c2.TestVerifyConstraintCasts

I would have left the `StressGCM` out - we already add that in our CI testing for some runs anyway. But I suppose it does not super hurt to have it in either.

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

Changes requested by epeter (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/22880#pullrequestreview-2566454868
PR Review Comment: https://git.openjdk.org/jdk/pull/22880#discussion_r1924865651
PR Review Comment: https://git.openjdk.org/jdk/pull/22880#discussion_r1924867315
PR Review Comment: https://git.openjdk.org/jdk/pull/22880#discussion_r1924869630