Skip to content

Restore MemberName on stack for linkTo* methods after PopFrame #19744

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jul 10, 2024

Conversation

babsingh
Copy link
Contributor

@babsingh babsingh commented Jun 23, 2024

OJDK MH INLs: linkToStaticSpecial, linkToVirtual and linkToInterface
pop the top element from the stack before executing the target method.
The top element is an instance of MemberName. These native INLs don't
show up on the stack, and when we pop a frame, we drop to the frame
which exists before these INLs. A crash happens if the element popped
by the OJDK MH INLs is not restored after a frame is popped. To
prevent the crash, the removed element is restored after a frame is
popped.

RTC Issue 151332

@babsingh babsingh requested a review from gacholio June 23, 2024 18:09
@gacholio
Copy link
Contributor

This needs to be tested with the target frame compiled - I suspect the decompilation case will already have the stack in the correct format, leading the this code pushing extraneous data.

@gacholio
Copy link
Contributor

You indicate in the RTC PR that this is tested with JIT , but I don't see why the decomp case would not have fully filled in the stack, so the extra work here would be unnecessary. Please elaborate.

@babsingh
Copy link
Contributor Author

babsingh commented Jun 24, 2024

I found that the decompilation case also needs the fix based on testing and introspection ...

I tried the below testcase with -Xjit:count=0 to evaluate the decompilation case. Initially, I had the fix disabled for the decompilation case with if (NULL == walkState.jitInfo) { FIX }. Without the fix, the crash was also seen for the decompilation case. I noticed a J2I frame at the top of the stack in case of the decompilation case. After enabling the fix for the decompilation case, the crash no longer happened.

Test Case

import java.lang.invoke.*;

public class Test1 {
        public static void main(String[] args) throws Throwable {
                MethodHandles.Lookup publicLookup = MethodHandles.publicLookup();
                MethodType mt = MethodType.methodType(String.class, String.class);

                MethodHandle concatMH = publicLookup.findVirtual(String.class, "concat", mt);
                String out = (String)concatMH.invoke("Hello ", "World!");
                System.out.println(out);
        }
}

JDB Instructions

./build/linux-x86_64-server-release/images/jdk/bin/jdb -Xjit:count=0 Test1.java

jdb > stop at Test1:9
jdb > run
jdb > stop in java.lang.String.concat  # Method that's invoked by the MethodHandle
jdb > pop                              # Dropping back to the invoke frame
jdb > cont

@babsingh babsingh force-pushed the main7 branch 3 times, most recently from 0d0719e to f1acc1c Compare June 26, 2024 23:26
@babsingh babsingh changed the title Append MemberName to stack for linkTo* methods after frame pop Restore MemberName on stack for linkTo* methods after PopFrame Jun 26, 2024
@babsingh babsingh marked this pull request as ready for review June 26, 2024 23:27
@gacholio
Copy link
Contributor

This seems like the right approach for the interpreter, but I still maintain the JIT case should take care of itself (there may be errors in the OSR framework for this case, or the decompiler itself). Please run with -verbose:stackwalk=0 which will dump stack traces around the decompile points and we can see what's happening in the jitted case.

@gacholio
Copy link
Contributor

I don't recall ever updating the decompiler to handle the polymorphic signature calls (the decompiler decides what the pending stack should look like, and assumes the OSR code will agree).

@babsingh
Copy link
Contributor Author

run with -verbose:stackwalk=0 which will dump stack traces

Verbose stack walk output: verbose_stackwalk.log. There was a lot of output. I filtered it for the following thread: 1AA00. I wasn't sure if I was reading the output correctly.

I don't recall ever updating the decompiler to handle the polymorphic signature calls (the decompiler decides what the pending stack should look like, and assumes the OSR code will agree).

@gacholio Does the attached verbose output confirm that the JIT decompiled won't take care of itself?

I also noticed a crash with -verbose:stackwalk=0 in swalk.c:225 where walkState->userData4 was NULL. I added a NULL check to resolve it; I will open another PR to have it reviewed.

openj9/runtime/vm/swalk.c

Lines 225 to 236 in 293f2b7

J9UTF8 * className = J9ROMCLASS_CLASSNAME(((J9Class *) walkState->userData4)->romClass);
char detailStackBuffer[J9VM_PACKAGE_NAME_BUFFER_LENGTH];
char * detail = NULL;
j9object_t detailMessage = J9VMJAVALANGTHROWABLE_DETAILMESSAGE(currentThread, walkState->restartException);
if (NULL != detailMessage) {
detail = walkState->walkThread->javaVM->internalVMFunctions->copyStringToUTF8WithMemAlloc(currentThread, detailMessage, J9_STR_NULL_TERMINATE_RESULT, ": ", 2, detailStackBuffer, J9VM_PACKAGE_NAME_BUFFER_LENGTH, NULL);
}
swPrintf(walkState, 2, "\tThrowing exception: %.*s%s\n", J9UTF8_LENGTH(className), J9UTF8_DATA(className), detail ? detail : "");
if (detailStackBuffer != detail) {
j9mem_free_memory(detail);
}

Native stack during the crash:

...
#11 0x00007f72f620c695 in mainSynchSignalHandler (signal=11, sigInfo=0x7f72c2edc5b0, contextInfo=0x7f72c2edc480) at /root/openj9-openjdk-jdk23/omr/port/unix/omrsignal.c:1066
#12 <signal handler called>
#13 0x00007f72f49e653b in walkStackFramesVerbose (currentThread=0x206800, walkState=0x7f72c2edcbd0) at /root/openj9-openjdk-jdk23/openj9/runtime/vm/swalk.c:225
#14 0x00007f72f48fb0d7 in jvmtiPopFrame (env=<optimized out>, thread=0x7f726002deb0) at /root/openj9-openjdk-jdk23/openj9/runtime/jvmti/jvmtiStackFrame.cpp:498
#15 0x00007f72f481f16f in popOneFrame (thread=0x7f726002deb0) at src/jdk.jdwp.agent/share/native/libjdwp/threadControl.c:1884
#16 threadControl_popFrames (thread=thread@entry=0x7f726002deb0, fnum=0) at src/jdk.jdwp.agent/share/native/libjdwp/threadControl.c:1963
#17 0x00007f72f480126e in popFrames (in=0x7f72c2edd9c0, out=0x7f72c2edd9f0) at src/jdk.jdwp.agent/share/native/libjdwp/StackFrameImpl.c:451
...

babsingh added a commit to babsingh/openj9 that referenced this pull request Jun 28, 2024
A segfault was observed with -verbose:stackwalk=0 in eclipse-openj9#19744 where
walkState->userData4 was NULL.

Signed-off-by: Babneet Singh <[email protected]>
babsingh added a commit to babsingh/openj9 that referenced this pull request Jun 28, 2024
A segfault was observed with -verbose:stackwalk=0 in eclipse-openj9#19744 where
walkState->userData4 was NULL.

Signed-off-by: Babneet Singh <[email protected]>
babsingh added a commit to babsingh/openj9 that referenced this pull request Jun 28, 2024
A segfault was observed with -verbose:stackwalk=0 in eclipse-openj9#19744 where
walkState->userData4 was NULL.

Signed-off-by: Babneet Singh <[email protected]>
babsingh added a commit to babsingh/openj9 that referenced this pull request Jun 28, 2024
A segfault was observed with -verbose:stackwalk=0 in eclipse-openj9#19744 where
walkState->userData4 was NULL.

Signed-off-by: Babneet Singh <[email protected]>
OJDK MH INLs: linkToStaticSpecial, linkToVirtual and linkToInterface
pop the top element from the stack before executing the target method.
The top element is an instance of MemberName. These native INLs don't
show up on the stack, and when we pop a frame, we drop to the frame
which exists before these INLs. A crash happens if the element popped
by the OJDK MH INLs is not restored after a frame is popped. To
prevent the crash, the removed element is restored after a frame is
popped.

RTC Issue 151332

Signed-off-by: Babneet Singh <[email protected]>
@gacholio
Copy link
Contributor

jenkins test sanity,extended alinux jdk21

@gacholio gacholio merged commit abc6fa4 into eclipse-openj9:master Jul 10, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants