openj9 icon indicating copy to clipboard operation
openj9 copied to clipboard

Restore MemberName on stack for linkTo* methods after PopFrame

Open babsingh opened this issue 1 year ago • 6 comments

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 avatar Jun 23 '24 17:06 babsingh

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 avatar Jun 24 '24 13:06 gacholio

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.

gacholio avatar Jun 24 '24 13:06 gacholio

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 avatar Jun 24 '24 14:06 babsingh

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 avatar Jun 27 '24 13:06 gacholio

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 avatar Jun 27 '24 18:06 gacholio

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. https://github.com/eclipse-openj9/openj9/blob/293f2b7812795bdf5ca611bc832bf5a1dc3d052a/runtime/vm/swalk.c#L225-L236

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 avatar Jun 27 '24 21:06 babsingh

jenkins test sanity,extended alinux jdk21

gacholio avatar Jul 10 '24 20:07 gacholio