RFR: 8339307: jhsdb jstack could not trace FFM upcall frame [v4]

[Yasumasa Suenaga]

On Mon, 2 Sep 2024 15:13:44 GMT, Jorn Vernee <jvernee at openjdk.org> wrote:

>>> I understand that adding the UpcallStub type to the SA agent code makes the WrongTypeException go away, and then we run into an assertion failure because the frame size is zero?
>> 
>> Yes.
>> 
>>> Note how there is also special handling for (JNI) entry frames in the SA.
>> 
>> Do you mean `JavaCallWrapper` (`X86Frame::senderForEntryFrame` in SA) ?
>> 
>>> I'm guessing because we end up walking the native frames until we get back to Java, and the native frames are simply ignored. I'm not sure if that will always work for arbitrary native code though.
>>> 
>>> I think the right fix here is to implement handling for upcall stub frames in the SA agent, since that's also how entry frames are handled. I don't think setting the frame size in hotspot is actually needed if we do that.
>> 
>> If we add some frame info (return address and FP) like `JavaCallWrapper` to `UpcallStub` and process it in SA, we do not need frame size of `UpcallStub` as you said. But I think it should be fixed in all of upcall implementation.
>> `UpcallStub` is "Stub", so it compliant native calling convention. Thus I believe native frame unwinder like `X86Frame` should always work if frame size is set in `UpcallStub`.
>> 
>> We need to fix all of upcall implementation in both case, and zero frame size is not nature. In addition adding frame size is simpler than add special handling for `UpcallStub` and SA. Thus I give +1 to add frame size to `UpcallStub`.
>
>> > Note how there is also special handling for (JNI) entry frames in the SA.
>> 
>> Do you mean `JavaCallWrapper` (`X86Frame::senderForEntryFrame` in SA) ?
> 
> Yes. Internally it loads the fields of `JavaFrameAnchor`, which points at the previous Java frame. `UpcallStub` frames also have a `JavaFrameAnchor`. The value can be retrieved through `upcall_stub` -> `frame_data` -> `jfa`. The byte offset of the frame data is stored in the `UpcallStub::_frame_data_offset` field. It can be added to the unextended SP.
> 
>> > I'm guessing because we end up walking the native frames until we get back to Java, and the native frames are simply ignored. I'm not sure if that will always work for arbitrary native code though.
>> > I think the right fix here is to implement handling for upcall stub frames in the SA agent, since that's also how entry frames are handled. I don't think setting the frame size in hotspot is actually needed if we do that.
>> 
>> If we add some frame info (return address and FP) like `JavaCallWrapper` to `UpcallStub` and process it in SA, we do not need frame size of `UpcallStub` as you said. But I think it should be fixed in all of upcall implementation. `UpcallStub` is "Stub", so it compliant native calling convention. Thus I believe native frame unwinder like `X86Frame` should always work if frame size is set in `UpcallStub`.
> 
> The problem is not the upcall stub frame itself. We know which ABI that is using. The problems is any intermediate frames between the upcall stub frame and last Java frame before that. These intermediate native frames can have any ABI. There is no single 'native calling convention'. We know that we are interfacing with something that follows the C ABI, but that code may switch to a different ABI (e.g. Rust, C++, or some other language) which may have a different stack frame layout. Stack walking those frames might break. The frame anchor used by entry/upcall frames helps to avoid this by letting the stack walker jump over all the native frames, and continue walking at the last java frame before the upcall stub instead. That means it doesn't have to deal with the foreign stack layout of frames in between.
> 
>> We need to fix all of upcall implementation in both case, and zero frame size is not nature. In addition adding frame size is simpler than add special handling for `UpcallStub` and SA. Thus I give +1 to add frame size to `UpcallStub`.
> 
> I'm not necessarily opposed to adding a frame size to upcall stubs, but as a fix for SA stack...

@JornVernee @plummercj
Thanks for your comment!
I will try to fix SA to refer `JavaFrameAnchor`, and also to fix test case in weekend.

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

PR Comment: https://git.openjdk.org/jdk/pull/20789#issuecomment-2327691917