On Tue, 30 Aug 2022 23:29:18 GMT, Chris Plummer <cjplum...@openjdk.org> wrote:
> The root cause of this CR is that we are trying to access the java heap
> during the middle of a GC. This particular test is prone to that happening
> since it runs jstack 4 times on the debuggee as it starts up (the debuggee is
> jshell, so it does a lot of initialization on startup).
>
> SA needs to deal with potentially bad references to java objects. These might
> come from VM structs (such as the JavaThread reference to its java Thread
> instance), or a java object reference from a field of another java objects.
> There's always the possibility that these can be invalid. We work around this
> in most of our testing by getting the debuggee into a steady state so no GC
> can be in progress.
>
> For the most part SA is pretty good about dealing with these bad object
> references, and fails gracefully with an exception, but otherwise continues
> to run. However, this test exposed some holes that resulted in (1) way to
> many exception stack traces and (2) exiting the SA tool rather than trying to
> continue.
>
> The main fix is in JavaThread.printThreadInfo() where we now do a better job
> of dealing with exceptions if any of the java objects references are not
> valid. Even without access to the Thread instance, we can still produce a
> stack trace, but it will be missing things like the thread name and priority.
> This is a lot better than just giving up with the first exception.
>
> The other changes are for the most part just improvements in the error
> output, and getting rid of the printStackTrace() that was cluttering up the
> output.
I only found minor nits. Your hardened code appears to keep things in
the same output format which is good. I think your "noise" reductions
when there are errors are also good to have.
I look forward to seeing the reduction in CI noise.
src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/runtime/JavaThread.java
line 357:
> 355: obj = vmOopHandle.resolve();
> 356: } catch (Exception e) {
> 357: System.out.println("WARNING: could not get Thread object: " + e);
nit: This code seems to use an indent of 2 spaces.
src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/runtime/JavaThread.java
line 516:
> 514: Oop threadOop = this.getThreadObj();
> 515: if (threadOop == null) {
> 516: System.out.println("Could not get the java Thread object.
> Thread info will be limitted.");
typo: s/limitted/limited/
src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/runtime/JavaThread.java
line 546:
> 544: out.print(this.getAddress());
> 545: out.print(" nid=");
> 546: out.print(String.format("%d ",this.getOSThread().threadId()));
nit space after ','
src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/runtime/JavaThread.java
line 550:
> 548: out.print(" [");
> 549: if (this.getLastJavaSP() == null) {
> 550: out.print(String.format(ADDRESS_FORMAT,0L));
nit: space after ','
src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/runtime/JavaVFrame.java
line 69:
> 67: } else {
> 68: klassName = "<unknown class>";
> 69: }
nit: existing code uses 2 spaces, but your addition uses 4.
-------------
Marked as reviewed by dcubed (Reviewer).
PR: https://git.openjdk.org/jdk/pull/10088
