8371194: serviceability/sa/TestJhsdbJstackMixedWithXComp.java failing #28284
Conversation
Co-authored-by: Patricio Chilano Mateo <pchilanomate@openjdk.org>
|
👋 Welcome back ysuenaga! A progress list of the required criteria for merging this PR into |
|
@YaSuenag This change now passes all automated pre-integration checks. ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details. After integration, the commit message for the final commit will be: You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed. At the time when this comment was updated there had been 272 new commits pushed to the
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details. ➡️ To integrate this PR with the above commit message to the |
|
/contributor add @pchilano |
|
@YaSuenag |
Webrevs
|
|
This PR is for Linux, other platforms (Windows, Mac) would not work. However I think it is not a problem because Windows and Mac has not been supported mixed jstack originally. |
pchilano
left a comment
pchilano
left a comment
There was a problem hiding this comment.
Fix looks good to me (can't comment on the PPC and RISCV parts though).
| * @bug 8370176 | ||
| * @requires vm.hasSA | ||
| * @requires os.family == "linux" | ||
| * @requires os.arch == "amd64" |
There was a problem hiding this comment.
Can't we run the test on AArch64 now?
There was a problem hiding this comment.
I think this PR works on Linux AArch64, but I'm not sure because previous change (#27885) worked on some Linux AArch64, but there are some platforms whch did not work. Thus I think it is better to keep this test for Linux AMD64 only.
I couldn't reproduce the problem on Fedora Rawhide on Raspberry Pi 4, but there might be some issue because SA does not have DWARF parser for AArch 64 - it means the native we cannot find sender frame which depends on DWARF, does not use FP as frame pointer.
There was a problem hiding this comment.
I enabled this test for linux-aarch64 and it seems to pass our testing.
There was a problem hiding this comment.
I saw the problem on VMError::report_and_die because its frame could not be unwinded without DWARF.
I have not seen the problem in other place, but I'm concerned the test fails on some Linux AArch64 platforms. Thus I think it is better to wait enabling the test until SA supports DWARF.
|
On macosx I'm seeing the following when running ClhsdbPStack.java: The "Not available for Mac OS X processes" message is expected when running on a macosx process, but in this case the test is running the core version of the test, so SA is acting on a core file. The test goes on to fail because there are no threads in the output. The message is generated by PStack.java whenever the thread list is null and it is running on OSX. In that case it assumes the reason the thread list is null is because it is running on a macosx process. It's not clear to me why the thread list is null. I don't see any changes that look like they could cause that. This happens with both x64 and aarch64, although you need to remove the test from the problem list for macosx-aarch64 to get it to reproduce. macosx-x64 is not problem listed. |
Maybe it is a side effect of deleting
I think it is better to separate BsdDebuggerLocal::getJavaThreadsInfo() - filling threadList can move to getThreadList() because it is not needed in getJavaThreadsInfo(). |
|
@plummercj Can you try this patch on your Mac? You can apply it to this PR branch. diff --git a/src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/debugger/bsd/BsdDebuggerLocal.java b/src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/debugger/bsd/BsdDebuggerLocal.java
index 10f6881d010..f091b22f9d7 100644
--- a/src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/debugger/bsd/BsdDebuggerLocal.java
+++ b/src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/debugger/bsd/BsdDebuggerLocal.java
@@ -1,5 +1,5 @@
/*
- * Copyright (c) 2002, 2024, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2002, 2025, Oracle and/or its affiliates. All rights reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
@@ -25,7 +25,9 @@
package sun.jvm.hotspot.debugger.bsd;
import java.util.ArrayList;
+import java.util.Collections;
import java.util.List;
+import java.util.stream.IntStream;
import sun.jvm.hotspot.debugger.Address;
import sun.jvm.hotspot.debugger.DebuggerBase;
@@ -75,10 +77,11 @@ public class BsdDebuggerLocal extends DebuggerBase implements BsdDebugger {
// CDebugger support
private BsdCDebugger cdbg;
- // threadList and loadObjectList are filled by attach0 method
- private List<ThreadProxy> threadList;
+ // loadObjectList are filled by attach0 method
private List<LoadObject> loadObjectList;
+ private List<JavaThread> javaThreadList;
+
// called by native method lookupByAddress0
private ClosestSymbol createClosestSymbol(String name, long offset) {
return new ClosestSymbol(name, offset);
@@ -244,7 +247,7 @@ private void findABIVersion() throws DebuggerException {
/** From the Debugger interface via JVMDebugger */
public synchronized void attach(int processID) throws DebuggerException {
checkAttached();
- threadList = new ArrayList<>();
+ javaThreadList = new ArrayList<>();
loadObjectList = new ArrayList<>();
class AttachTask implements WorkerThreadTask {
int pid;
@@ -264,7 +267,7 @@ public void doit(BsdDebuggerLocal debugger) {
/** From the Debugger interface via JVMDebugger */
public synchronized void attach(String execName, String coreName) {
checkAttached();
- threadList = new ArrayList<>();
+ javaThreadList = new ArrayList<>();
loadObjectList = new ArrayList<>();
attach0(execName, coreName);
attached = true;
@@ -278,7 +281,7 @@ public synchronized boolean detach() {
return false;
}
- threadList = null;
+ javaThreadList = null;
loadObjectList = null;
if (isCore) {
@@ -492,7 +495,25 @@ public Address newAddress(long value) {
/** From the BsdCDebugger interface */
public List<ThreadProxy> getThreadList() {
requireAttach();
- return threadList;
+
+ if (javaThreadList == null) {
+ return null;
+ }
+ if (!isCore) {
+ // TODO: thread list is now supported for corefile only.
+ return Collections.emptyList();
+ }
+
+ if (javaThreadList.isEmpty()) {
+ Threads threads = VM.getVM().getThreads();
+ IntStream.range(0, threads.getNumberOfThreads())
+ .mapToObj(threads::getJavaThreadAt)
+ .forEach(javaThreadList::add);
+ }
+
+ return javaThreadList.stream()
+ .map(t -> t.getThreadProxy())
+ .toList();
}
/** From the BsdCDebugger interface */
@@ -561,21 +582,16 @@ public void doit(BsdDebuggerLocal debugger) {
/** this functions used for core file reading and called from native attach0,
it returns an array of long integers as
[thread_id, stack_start, stack_end, thread_id, stack_start, stack_end, ....] for
- all java threads recorded in Threads. Also adds the ThreadProxy to threadList */
+ all java threads recorded in Threads. */
public long[] getJavaThreadsInfo() {
requireAttach();
- Threads threads = VM.getVM().getThreads();
- int len = threads.getNumberOfThreads();
- long[] result = new long[len * 3]; // triple
+ long[] result = new long[javaThreadList.size() * 3]; // triple
long beg, end;
int i = 0;
- for (int k = 0; k < threads.getNumberOfThreads(); k++) {
- JavaThread t = threads.getJavaThreadAt(k);
+ for (var t : javaThreadList) {
end = t.getStackBaseValue();
beg = end - t.getStackSize();
- BsdThread bsdt = (BsdThread)t.getThreadProxy();
- long uid = bsdt.getUniqueThreadId();
- if (threadList != null) threadList.add(bsdt);
+ long uid = ((BsdThread)t.getThreadProxy()).getUniqueThreadId();
result[i] = uid;
result[i + 1] = beg;
result[i + 2] = end; |
|
With your diffs ClhsdbPStack.java is passing but now I'm seeing issues with the following 3 core file tests on both aarch64 and x64: In the log it is having trouble with the stack trace: This repeats for every thread. |
|
@plummercj Thanks a lot for your report! The error what you saw is caused that thread list wasn't made in diff --git a/src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/debugger/bsd/BsdDebuggerLocal.java b/src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/debugger/bsd/BsdDebuggerLocal.java
index 10f6881d010..7c71dcd0ca5 100644
--- a/src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/debugger/bsd/BsdDebuggerLocal.java
+++ b/src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/debugger/bsd/BsdDebuggerLocal.java
@@ -1,5 +1,5 @@
/*
- * Copyright (c) 2002, 2024, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2002, 2025, Oracle and/or its affiliates. All rights reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
@@ -25,7 +25,9 @@
package sun.jvm.hotspot.debugger.bsd;
import java.util.ArrayList;
+import java.util.Collections;
import java.util.List;
+import java.util.stream.IntStream;
import sun.jvm.hotspot.debugger.Address;
import sun.jvm.hotspot.debugger.DebuggerBase;
@@ -75,10 +77,11 @@ public class BsdDebuggerLocal extends DebuggerBase implements BsdDebugger {
// CDebugger support
private BsdCDebugger cdbg;
- // threadList and loadObjectList are filled by attach0 method
- private List<ThreadProxy> threadList;
+ // loadObjectList are filled by attach0 method
private List<LoadObject> loadObjectList;
+ private List<JavaThread> javaThreadList;
+
// called by native method lookupByAddress0
private ClosestSymbol createClosestSymbol(String name, long offset) {
return new ClosestSymbol(name, offset);
@@ -241,10 +244,16 @@ private void findABIVersion() throws DebuggerException {
}
}
+ private List<JavaThread> generateJavaThreadList() {
+ Threads threads = VM.getVM().getThreads();
+ return IntStream.range(0, threads.getNumberOfThreads())
+ .mapToObj(threads::getJavaThreadAt)
+ .toList();
+ }
+
/** From the Debugger interface via JVMDebugger */
public synchronized void attach(int processID) throws DebuggerException {
checkAttached();
- threadList = new ArrayList<>();
loadObjectList = new ArrayList<>();
class AttachTask implements WorkerThreadTask {
int pid;
@@ -253,6 +262,10 @@ public void doit(BsdDebuggerLocal debugger) {
debugger.attached = true;
debugger.isCore = false;
findABIVersion();
+
+ // TODO: thread list on macOS is now supported for corefile only.
+ debugger.javaThreadList = debugger.isDarwin ? Collections.emptyList()
+ : generateJavaThreadList();
}
}
@@ -264,12 +277,12 @@ public void doit(BsdDebuggerLocal debugger) {
/** From the Debugger interface via JVMDebugger */
public synchronized void attach(String execName, String coreName) {
checkAttached();
- threadList = new ArrayList<>();
loadObjectList = new ArrayList<>();
attach0(execName, coreName);
attached = true;
isCore = true;
findABIVersion();
+ javaThreadList = generateJavaThreadList();
}
/** From the Debugger interface via JVMDebugger */
@@ -278,7 +291,7 @@ public synchronized boolean detach() {
return false;
}
- threadList = null;
+ javaThreadList = null;
loadObjectList = null;
if (isCore) {
@@ -492,7 +505,9 @@ public Address newAddress(long value) {
/** From the BsdCDebugger interface */
public List<ThreadProxy> getThreadList() {
requireAttach();
- return threadList;
+ return javaThreadList.stream()
+ .map(JavaThread::getThreadProxy)
+ .toList();
}
/** From the BsdCDebugger interface */
@@ -561,21 +576,16 @@ public void doit(BsdDebuggerLocal debugger) {
/** this functions used for core file reading and called from native attach0,
it returns an array of long integers as
[thread_id, stack_start, stack_end, thread_id, stack_start, stack_end, ....] for
- all java threads recorded in Threads. Also adds the ThreadProxy to threadList */
+ all java threads recorded in Threads. */
public long[] getJavaThreadsInfo() {
requireAttach();
- Threads threads = VM.getVM().getThreads();
- int len = threads.getNumberOfThreads();
- long[] result = new long[len * 3]; // triple
+ long[] result = new long[javaThreadList.size() * 3]; // triple
long beg, end;
int i = 0;
- for (int k = 0; k < threads.getNumberOfThreads(); k++) {
- JavaThread t = threads.getJavaThreadAt(k);
+ for (var t : javaThreadList) {
end = t.getStackBaseValue();
beg = end - t.getStackSize();
- BsdThread bsdt = (BsdThread)t.getThreadProxy();
- long uid = bsdt.getUniqueThreadId();
- if (threadList != null) threadList.add(bsdt);
+ long uid = ((BsdThread)t.getThreadProxy()).getUniqueThreadId();
result[i] = uid;
result[i + 1] = beg;
result[i + 2] = end; |
VM.initialize() is not triggered until after HotSpotAgent.setupDebugger() returns: |
|
Ok, I confirmed diff --git a/src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/debugger/bsd/BsdDebuggerLocal.java b/src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/debugger/bsd/BsdDebuggerLocal.java
index 10f6881d010..891ebc91f49 100644
--- a/src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/debugger/bsd/BsdDebuggerLocal.java
+++ b/src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/debugger/bsd/BsdDebuggerLocal.java
@@ -1,5 +1,5 @@
/*
- * Copyright (c) 2002, 2024, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2002, 2025, Oracle and/or its affiliates. All rights reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
@@ -25,7 +25,9 @@
package sun.jvm.hotspot.debugger.bsd;
import java.util.ArrayList;
+import java.util.Collections;
import java.util.List;
+import java.util.stream.IntStream;
import sun.jvm.hotspot.debugger.Address;
import sun.jvm.hotspot.debugger.DebuggerBase;
@@ -75,10 +77,11 @@ public class BsdDebuggerLocal extends DebuggerBase implements BsdDebugger {
// CDebugger support
private BsdCDebugger cdbg;
- // threadList and loadObjectList are filled by attach0 method
- private List<ThreadProxy> threadList;
+ // loadObjectList are filled by attach0 method
private List<LoadObject> loadObjectList;
+ private List<JavaThread> javaThreadList;
+
// called by native method lookupByAddress0
private ClosestSymbol createClosestSymbol(String name, long offset) {
return new ClosestSymbol(name, offset);
@@ -241,10 +244,21 @@ private void findABIVersion() throws DebuggerException {
}
}
+ private void fillJavaThreadList() {
+ // TODO: thread list on macOS is now supported for corefile only.
+ if (!isCore && isDarwin) {
+ javaThreadList = Collections.emptyList();
+ } else {
+ Threads threads = VM.getVM().getThreads();
+ javaThreadList = IntStream.range(0, threads.getNumberOfThreads())
+ .mapToObj(threads::getJavaThreadAt)
+ .toList();
+ }
+ }
+
/** From the Debugger interface via JVMDebugger */
public synchronized void attach(int processID) throws DebuggerException {
checkAttached();
- threadList = new ArrayList<>();
loadObjectList = new ArrayList<>();
class AttachTask implements WorkerThreadTask {
int pid;
@@ -264,7 +278,6 @@ public void doit(BsdDebuggerLocal debugger) {
/** From the Debugger interface via JVMDebugger */
public synchronized void attach(String execName, String coreName) {
checkAttached();
- threadList = new ArrayList<>();
loadObjectList = new ArrayList<>();
attach0(execName, coreName);
attached = true;
@@ -278,7 +291,7 @@ public synchronized boolean detach() {
return false;
}
- threadList = null;
+ javaThreadList = null;
loadObjectList = null;
if (isCore) {
@@ -492,7 +505,12 @@ public Address newAddress(long value) {
/** From the BsdCDebugger interface */
public List<ThreadProxy> getThreadList() {
requireAttach();
- return threadList;
+ if (javaThreadList == null) {
+ fillJavaThreadList();
+ }
+ return javaThreadList.stream()
+ .map(JavaThread::getThreadProxy)
+ .toList();
}
/** From the BsdCDebugger interface */
@@ -561,21 +579,19 @@ public void doit(BsdDebuggerLocal debugger) {
/** this functions used for core file reading and called from native attach0,
it returns an array of long integers as
[thread_id, stack_start, stack_end, thread_id, stack_start, stack_end, ....] for
- all java threads recorded in Threads. Also adds the ThreadProxy to threadList */
+ all java threads recorded in Threads. */
public long[] getJavaThreadsInfo() {
requireAttach();
- Threads threads = VM.getVM().getThreads();
- int len = threads.getNumberOfThreads();
- long[] result = new long[len * 3]; // triple
+ if (javaThreadList == null) {
+ fillJavaThreadList();
+ }
+ long[] result = new long[javaThreadList.size() * 3]; // triple
long beg, end;
int i = 0;
- for (int k = 0; k < threads.getNumberOfThreads(); k++) {
- JavaThread t = threads.getJavaThreadAt(k);
+ for (var t : javaThreadList) {
end = t.getStackBaseValue();
beg = end - t.getStackSize();
- BsdThread bsdt = (BsdThread)t.getThreadProxy();
- long uid = bsdt.getUniqueThreadId();
- if (threadList != null) threadList.add(bsdt);
+ long uid = ((BsdThread)t.getThreadProxy()).getUniqueThreadId();
result[i] = uid;
result[i + 1] = beg;
result[i + 2] = end; |
|
It's passing now. |
|
@plummercj Thanks a lot! I pushed the change to this PR. |
|
PING: could you review this? |
Sorry I didn't have a chance to get to this yet. I'm on holiday the rest of the week. I should be able to look at it on Monday. I did run your changes through most of our relevant CI testing, and everything passed. |
plummercj
left a comment
plummercj
left a comment
There was a problem hiding this comment.
Overall this looks good. I made one very minor request for a comment fix. Thanks for all your efforts here1
src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/debugger/bsd/BsdDebuggerLocal.java
Outdated
Show resolved
Hide resolved
…sd/BsdDebuggerLocal.java Co-authored-by: Chris Plummer <chris.plummer@oracle.com>
|
Thanks a lot for your review! |
|
Thanks a lot for your review & help! /integrate |
|
Going to push as commit d3083ac.
Your commit was automatically rebased without conflicts. |
openjdk
bot
removed
ready
@YaSuenag it doesn't seem to work on macOS however. We get an exception moving from the native frames to Java frames: Bug is being filed |
4 participants
We've fixed mixed mode jstack for the debuggee running with
-Xcompin JDK-8370176, but it was not enough. We need to handle like unifyingCFrameandFramein stack unwinding as possible, and need to change how to get caller SP/FP from stack.This PR works fine on both Linux AMD64 and Linux AArch64. All of
hotspot/jtreg/serviceability/satests have been passed on both platforms.Big thanks to @pchilano for your help!
Progress
Issue
Reviewers
Contributors
<pchilanomate@openjdk.org>Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/28284/head:pull/28284$ git checkout pull/28284Update a local copy of the PR:
$ git checkout pull/28284$ git pull https://git.openjdk.org/jdk.git pull/28284/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 28284View PR using the GUI difftool:
$ git pr show -t 28284Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/28284.diff
Using Webrev
Link to Webrev Comment