[aarch64-port-dev ] RFR: 8209413: AArch64: NPE in clhsdb jstack command

[Andrew Haley]

On 2/6/19 6:42 PM, Andrew Haley wrote:
> On 2/6/19 10:54 AM, Nick Gasson (Arm Technology China) wrote:
>> Hi Andrew
>>
>>> Here's the test that reveals the problem: it seems that you need an entry frame which calls compiled Java code.
>>
>> This seems slightly different to the original problem, although
>> maybe related. Because here the top-most frame is a compiled Java
>> frame we'll take the vm.isJavaPCDbg and !vm.isClientCompiler
>> branches of AARCH64CurrentFrameGuess::run which as far as I can tell
>> always gets the PC from the thread context. The original crash I was
>> looking at happened when the top frame was native (else branch on
>> line 183) where the PC is set to null which causes the two-argument
>> AARCH64Frame constructor to be used.
> 
> OK.
> 
>> Unfortunately I'm on holiday until next Thursday so can't test
>> anything. Did you try Thread.sleep? That's what LingeredApp that the
>> jtreg tests is trying to get a stack trace of calls.
> 
> Well, that was fun. When generally assume that we can find a saved PC
> in a fixed place in the frame, and this is always the word above the
> FP. However, when we're in JNI native code, as you've noticed the
> C[++] compiler isn't helpful enough to lave the caller PC in a
> well-known place. When the frame is created by our AArch64 assembly-
> langage code we'll be fine because we always do the right thing.
> 
> The problem is that when we save the Java frame state in the Thread,
> we're doing it for the sake of the runtime, not for debuggers, so we
> don't always save all the information that a debugger might need.
> 
> This patch should work for compiled native methods, but I'm not at all
> sure about all of the other places where we call out from runtime
> stubs to the VM. We perhaps check that the PC returned by
> raw_sp.getAddressAt(-1 * VM.getVM().getAddressSize()) is in the code
> cache before we use it.
> 
> Anyway, try this: it should fix your immediate bug.

I had a much better idea. We usually save the PC in the frame anchor,
so we should use it if we can. Add a little sanity checking, and we
should be good to go, and we won't need to grovel about in the stack.

I looked in all the places where we save information in the frame
anchor, and we almost always set the PC appropriately. I tried this
patch with a few basic smoke tests, and it seems to be considerably
better than what we have right now. The only place it fails is when
we're executing some C++-compiled code which is called directly from
the interpreter or from compiled code. We could apply some magic in
those cases, but it would incur some overhead and I'm not sure it's
worthwhile.

Please try this instead and let me know how you get on. Thanks.

diff -r 3954d70e1c50 src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/runtime/aarch64/AARCH64CurrentFrameGuess.java
--- a/src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/runtime/aarch64/AARCH64CurrentFrameGuess.java	Wed Feb 06 08:31:27 2019 +0100
+++ b/src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/runtime/aarch64/AARCH64CurrentFrameGuess.java	Thu Feb 07 12:45:29 2019 +0000
@@ -223,8 +223,13 @@
         }
       }

-      setValues(sp, fp, null);
-
+      // We found a PC in the frame anchor. Check that it's plausible, and
+      // if it is, use it.
+      if (vm.isJavaPCDbg(pc)) {
+        setValues(sp, fp, pc);
+      } else {
+        setValues(sp, fp, null);
+      }
       return true;
     }
   }


-- 
Andrew Haley
Java Platform Lead Engineer
Red Hat UK Ltd. <https://www.redhat.com>
EAC8 43EB D3EF DB98 CC77 2FAD A5CD 6035 332F A671