On Tue, 10 Mar 2026 13:12:18 GMT, Patricio Chilano Mateo
<[email protected]> wrote:
>> Markus Grönlund has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> normalized_bci
>
> src/hotspot/share/jfr/recorder/stacktrace/jfrStackTrace.cpp line 213:
>
>> 211: assert(nm != nullptr, "invariant");
>> 212: assert(nm->needs_stack_repair(), "invariant");
>> 213: if (nm->is_native_method()) {
>
> I think this case should not be possible because we don't mark native methods
> with `needs_stack_repair`:
> https://github.com/openjdk/valhalla/blob/9024b8ee454e103248fc24cd95fd7543253600f4/src/hotspot/share/runtime/sharedRuntime.cpp#L2999
Ok, great.
> src/hotspot/share/jfr/recorder/stacktrace/jfrStackTrace.cpp line 227:
>
>> 225:
>> 226: static inline JfrSampleRequestFrameType frame_type(const
>> JfrSampleRequest& request) {
>> 227: const intptr_t value = p2i(request._sample_bcp);
>
> I see we have the following code when building the sampling request (also
> further down in `build_from_context`):
>
> https://github.com/openjdk/valhalla/blob/9024b8ee454e103248fc24cd95fd7543253600f4/src/hotspot/share/jfr/periodic/sampling/jfrSampleRequest.cpp#L133-L135
>
> We are relying on this constant not matching
> `JfrSampleRequestFrameType::NEEDS_STACK_REPAIR`. Maybe we should use
> `JfrSampleRequestFrameType::INTERPRETER` now?
Good idea.
> test/jdk/jdk/jfr/event/profiling/TestNeedStackRepair.java line 41:
>
>> 39: /*
>> 40: * @test
>> 41: * @comment The purpose of this test is to verify that
>> jdk.ExecutionSample events taken inside non-scalarized frames
>
> Not sure whether there is established terminology for this, but referring to
> this c2 frame as "non-scalarized" is confusing to me since c2 uses the
> scalarized calling convention. The caller might be interpreted, but the
> arguments will still be unpacked at the entry point following the scalarized
> convention, and the return value will be returned as fields when possible.
> Why not simply refer to these as frames that need stack repair? (Note: frame
> extension could also happen with a c2 caller and a c1 callee)
Let's use "frames that need stack repair" to make this clearer.
-------------
PR Review Comment:
https://git.openjdk.org/valhalla/pull/2176#discussion_r2912661403
PR Review Comment:
https://git.openjdk.org/valhalla/pull/2176#discussion_r2912660240
PR Review Comment:
https://git.openjdk.org/valhalla/pull/2176#discussion_r2912668193
