On Thu, 13 Aug 2026 04:51:36 GMT, Shiv Shah <[email protected]> wrote:
>> The sleeping thread tests had a hardcoded list of jdk methods for checking >> the stack trace so changes to Thread.sleep often required updating the >> tests. Now they only require the java.lang.Thread.sleep entry frame and >> ignore whatever sits above it while the callers below still go through the >> existing expected methods. Calls to getThreadInfo without an explicit >> maximum depth return no stack, so those paths skip the frame content check. >> >> strace001 samples running threads rather than sleeping threads, so it has no >> Thread.sleep frame to use as a boundary. Instead, it finds the innermost >> frame belonging to the test and ignores frames above it. Its stack length >> check now uses the requested depth plus the expected method count instead of >> the hardcoded depth + 7 estimate, and an unexpectedly empty snapshot now >> fails instead of passing accidentally. >> >> >> >> >> --------- >> - [x] I confirm that I make this contribution in accordance with the >> [OpenJDK Interim AI Policy](https://openjdk.org/legal/ai). > > Shiv Shah has updated the pull request with a new target base due to a merge > or a rebase. The incremental webrev excludes the unrelated changes brought in > by the merge/rebase. The pull request contains three additional commits since > the last revision: > > - Require the Thread.sleep entry frame instead of accepting any JDK frame > - Merge branch 'master' into JDK-8340088-v2 > - 8340088: Stack tracing tests of sleeping thread should be more resilient > to code changes This looks good to me. I've posted a couple of nits thought. test/hotspot/jtreg/vmTestbase/nsk/monitoring/share/ThreadController.java line 702: > 700: && elements[i].getMethodName().equals("sleep")) { > 701: return super.checkStackTrace( > 702: Arrays.copyOfRange(elements, i, > elements.length)); Nit: A comment would be nice to have before the line 701 to explain why a call to the super.checkStackTrace() is needed. We already found a frame with the sleep() method. What else do we need here? Q: Also, Is it okay to have two methods with the same signature and implementation? Can we avoid this duplication? test/hotspot/jtreg/vmTestbase/nsk/monitoring/share/thread/SleepingThread.java line 105: > 103: if > (elements[i].getClassName().equals("java.lang.Thread") > 104: && > elements[i].getMethodName().equals("sleep")) { > 105: return > super.checkStackTrace(Arrays.copyOfRange(elements, i, elements.length)); Nit: A comment would be nice to have before the line 105 to explain why a call to the `super.checkStackTrace()` is needed. We already found a frame with the `sleep()` method. What else do we need here? test/hotspot/jtreg/vmTestbase/nsk/monitoring/stress/thread/strace001.java line 237: > 235: } > 236: > 237: // The method checks whether the element belongs to the test's own > code Nit: Need **dot** at the end of comment at line 237. ------------- Marked as reviewed by sspitsyn (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/31357#pullrequestreview-5025279411 PR Review Comment: https://git.openjdk.org/jdk/pull/31357#discussion_r3858389834 PR Review Comment: https://git.openjdk.org/jdk/pull/31357#discussion_r3858363882 PR Review Comment: https://git.openjdk.org/jdk/pull/31357#discussion_r3858396813
