RFR: 8347463: jdk/jfr/threading/TestManyVirtualThreads.java crashes with assert(oopDesc::is_oop_or_null(val)) [v2]

[Roberto Castañeda Lozano]

On Mon, 27 Oct 2025 06:35:27 GMT, Anton Seoane Ampudia <aseoane at openjdk.org> wrote:

>> This PR introduces a fix for a intermittent assert crash due to a non-oop found in the stack when deoptimizing.
>> 
>> The `inline_native_GetEventWriter` JFR intrinsic performs a call into the runtime, which can safepoint, to write a checkpoint for the vthread. This call returns a global handle (`jobject`) that then gets resolved to a raw oop.
>> 
>> However, the corresponding `jfr_write_checkpoint_Type` does not set any return, modelling the call as `void`. If a safepoint hits in the small window after the stub returns but before the writer oop is used, and the GC moves the object in that window, the deoptimization path cannot resolve a handle that it never recorded, leading to the subsequent crash.
>> 
>> An IR Framework test is introduced to exercise the error explicitly. Additionally, related documentation in form of comments in the appropriate file (`runtime.hpp`) is added to hopefully prevent similar cases in the future.
>> 
>> **Testing:** passes tiers 1-5
>
> Anton Seoane Ampudia 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 'openjdk:master' into JDK-8347463
>  - Merge branch 'JDK-8347463' of github.com:anton-seoane/jdk into JDK-8347463
>  - Merge branch 'openjdk:master' into JDK-8347463
>  - Documentation for future similar cases
>  - Test for JDK-8347463
>  - Change to a more specific type
>  - Runtime call had void type but actually returned an object

Thanks for getting to the bottom of this, Antón! The changeset looks good to me, modulo a few documentation and test comments.
It would be good if someone from the JFR team (@mgronlun or @egahlin?) could have a look at this change as well.

src/hotspot/share/opto/runtime.hpp line 55:

> 53: // signature. Even if you don't plan on consuming the output of the call, C2
> 54: // needs this information to correctly track returned oops and avoid strange
> 55: // deoptimization crashes (JDK-8347463).

I agree with the intent of this comment, but the "strange deoptimization crashes" part could be made a bit more precise. Also, the first sentence is grammatically incorrect. Here is my suggestion:

Suggestion:

//
// Please ensure the return type of the runtime call matches its signature,
// even if the return value is unused. This is crucial for correct handling
// of runtime calls that return an oop and may trigger deoptimization
// on return. See rematerialize_objects() in deoptimization.cpp.

test/hotspot/jtreg/compiler/intrinsics/TestReturnsOopSetForJFRWriteCheckpoint.java line 29:

> 27: 
> 28: import jdk.jfr.Event;
> 29: import jdk.jfr.Name;

Unused, please remove.

test/hotspot/jtreg/compiler/intrinsics/TestReturnsOopSetForJFRWriteCheckpoint.java line 34:

> 32: /**
> 33:  * @test
> 34:  * @summary Tests that the getEventWriter call to write_checkpoint correctly

Suggestion:

 * @summary Tests that the getEventWriter call to write_checkpoint correctly
 * @bug 8347463

test/hotspot/jtreg/compiler/intrinsics/TestReturnsOopSetForJFRWriteCheckpoint.java line 36:

> 34:  * @summary Tests that the getEventWriter call to write_checkpoint correctly
> 35:  *          reports returning an oop
> 36:  * @requires vm.hasJFR & vm.continuations

Do we need `vm.continuations`?

test/hotspot/jtreg/compiler/intrinsics/TestReturnsOopSetForJFRWriteCheckpoint.java line 38:

> 36:  * @requires vm.hasJFR & vm.continuations
> 37:  * @library /test/lib /
> 38:  * @modules jdk.jfr/jdk.jfr.internal

Do we need this?

test/hotspot/jtreg/compiler/intrinsics/TestReturnsOopSetForJFRWriteCheckpoint.java line 53:

> 51:     // for the write_checkpoint call. Instead of explicitly checking for
> 52:     // it, we look for a non-void return type (which comes hand-in-hand
> 53:     // with the returns_oop information)

No need to mention the bug number here, better to declare at the top using `@bug`:

Suggestion:

    // Crash was due to the returns_oop field not being set
    // for the write_checkpoint call. Instead of explicitly checking for
    // it, we look for a non-void return type (which comes hand-in-hand
    // with the returns_oop information).

test/hotspot/jtreg/compiler/intrinsics/TestReturnsOopSetForJFRWriteCheckpoint.java line 55:

> 53:     // with the returns_oop information)
> 54:     @Test
> 55:     @IR(failOn = { IRNode.STATIC_CALL_OF_METHOD, "write_checkpoint.*void"})

You could replace this check with a more precise, positive one:

Suggestion:

    @IR(counts = { IRNode.STATIC_CALL_OF_METHOD, "write_checkpoint\s+java/lang/Object\s+\*", "1" })

test/hotspot/jtreg/compiler/intrinsics/TestReturnsOopSetForJFRWriteCheckpoint.java line 63:

> 61:         }
> 62: 
> 63:     }

Please remove unnecessary whitespace.

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

Changes requested by rcastanedalo (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/27913#pullrequestreview-3382397915
PR Review Comment: https://git.openjdk.org/jdk/pull/27913#discussion_r2464866520
PR Review Comment: https://git.openjdk.org/jdk/pull/27913#discussion_r2464871994
PR Review Comment: https://git.openjdk.org/jdk/pull/27913#discussion_r2464870520
PR Review Comment: https://git.openjdk.org/jdk/pull/27913#discussion_r2464894276
PR Review Comment: https://git.openjdk.org/jdk/pull/27913#discussion_r2464903315
PR Review Comment: https://git.openjdk.org/jdk/pull/27913#discussion_r2464906522
PR Review Comment: https://git.openjdk.org/jdk/pull/27913#discussion_r2464909980
PR Review Comment: https://git.openjdk.org/jdk/pull/27913#discussion_r2464907436