RFR: 8330157: C2: Add a stress flag for bailouts [v12]

[Christian Hagedorn]

On Tue, 24 Sep 2024 16:53:51 GMT, Daniel Skantz <duke at openjdk.org> wrote:

>> This patch adds a diagnostic/stress flag for C2 bailouts. It can be used to support testing of existing bailouts to prevent issues like [JDK-8318445](https://bugs.openjdk.org/browse/JDK-8318445), and can test for issues only seen at runtime such as [JDK-8326376](https://bugs.openjdk.org/browse/JDK-8326376). It can also be useful if we want to add more bailouts ([JDK-8318900](https://bugs.openjdk.org/browse/JDK-8318900)).
>> 
>> We check two invariants.
>> a) Bailouts should be successful starting from any given `failing()` check.
>> b) The VM should not record a bailout when one is pending (in which case we have continued to optimize for too long).
>> 
>> a), b) are checked by randomly starting a bailout at calls to `failing()` with a user-given probability.
>> 
>> The added flag should not have any effect in debug mode.
>> 
>> Testing:
>> 
>> T1-5, with flag and without it. We want to check that this does not cause any test failures without the flag set, and no unexpected failures with it. Tests failing because of timeout or because an error is printed to output when compilation fails can be expected in some cases.
>
> Daniel Skantz has updated the pull request incrementally with one additional commit since the last revision:
> 
>   left over

Nice stress mode! Some small comments but otherwise, looks good to me, too.

src/hotspot/share/opto/compile.hpp line 792:

> 790: 
> 791: #ifdef ASSERT
> 792:   bool       phase_verify_ideal_loop() { return _phase_verify_ideal_loop; }

can be made `const`:
Suggestion:

  bool       phase_verify_ideal_loop() const { return _phase_verify_ideal_loop; }

src/hotspot/share/opto/compile.hpp line 838:

> 836:   const CompilationFailureInfo* first_failure_details() const { return _first_failure_details; }
> 837: 
> 838:   bool failing(DEBUG_ONLY(bool no_stress_bailout = false)) {

It's somehow difficult to read what `failing(false/true)` now exactly mean. When having `failing(true)`, don't we get the same behavior as if we call `failing_internal()`? If `failing_internal()` is false, then we would only return false because we are not entering

    if (StressBailout && !no_stress_bailout) {
      return fail_randomly();
    }

So, I'm wondering if we cannot just use `failing_internal()` instead of `failing(true)` and remove the parameter completely?

src/hotspot/share/opto/compile.hpp line 843:

> 841:     }
> 842: #ifdef ASSERT
> 843:     // Disable stress code for PhaseIdealLoop verification

Can you expand the comment here and add the reason why? From a comment above, you mentioned that it is not easy to make it work. I guess it's fine to just mention that here.

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

PR Review: https://git.openjdk.org/jdk/pull/19646#pullrequestreview-2342682921
PR Review Comment: https://git.openjdk.org/jdk/pull/19646#discussion_r1784348743
PR Review Comment: https://git.openjdk.org/jdk/pull/19646#discussion_r1784363994
PR Review Comment: https://git.openjdk.org/jdk/pull/19646#discussion_r1784351478