On Mon, 2 Sep 2024 15:13:44 GMT, Jorn Vernee <jver...@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
