Looks good to me.
tom
Severin Gehwolf wrote on 10/7/19 2:43 AM:
Hi Tom,
On Thu, 2019-10-03 at 11:14 -0700, Tom Rodriguez wrote:
I think your fix is a reasonable way to resolve the NPE. We certainly
shouldn't try to create a ScopeDesc from an invalid location. Maybe
getPCDescNearDbg should try to do a better job finding a PcDesc with a
valid scope_decode_offset instead? Something like this maybe?
diff -r c414c554b38b
src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/code/NMethod.java
--- a/src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/code/NMethod.java
+++ b/src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/code/NMethod.java
@@ -306,12 +306,16 @@
/** This is only for use by the debugging system, and is only
intended for use in the topmost frame, where we are not
guaranteed to be at a PC for which we have a PCDesc. It finds
- the PCDesc with realPC closest to the current PC. */
+ the PCDesc with realPC closest to the current PC that has a valid
scopeDecodeOffset. */
public PCDesc getPCDescNearDbg(Address pc) {
PCDesc bestGuessPCDesc = null;
long bestDistance = 0;
for (Address p = scopesPCsBegin(); p.lessThan(scopesPCsEnd()); p =
p.addOffsetTo(pcDescSize)) {
PCDesc pcDesc = new PCDesc(p);
+ if (pd.getScopeDecodeOffset() ==
DebugInformationRecorder.SERIALIZED_NULL) {
+ // We've observed a serialized null decode offset so ignore
this PcDesc
+ continue;
+ }
// In case pc is null
long distance = -pcDesc.getRealPC(this).minus(pc);
if ((bestGuessPCDesc == null) ||
As far as I can see getPCDescNearDbg is only used by getScopeDescNearDbg
so the semantic change seems reasonable to me.
Updated webrev:
http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8196969/04/webrev/
Preliminary testing looks good.
How does it look?
Andrew: Are you still OK with this change?
Thanks,
Severin
tom
Chris Plummer wrote on 10/3/19 9:19 AM:
Hi Severin,
I've cc'd Tom Rodriquez who said he would have a look.
thanks,
Chris
On 10/1/19 12:51 PM, Chris Plummer wrote:
Hi Severin,
Sorry, this is not an area that I have any expertise in. However, I
did confirm that it fixes the NPE I was seeing with
JShellHeapDumpTest.java, which brings up a question. You said this
happens with -Xcomp, but I was never using -Xcomp. Might it also be
triggered without -Xcomp?
thanks,
Chris
On 10/1/19 1:58 AM, Severin Gehwolf wrote:
Anyone? Chris maybe?
On Fri, 2019-09-27 at 14:12 +0200, Severin Gehwolf wrote:
Hi,
Could I please get reviews for this SA fix? The issue only happens
intermittently and with -Xcomp. The new regression test reproduces the
issue somewhat reliably. I got 10/10 fails for unpatched, but I've seen
it pass as well.
When the issue happens, PCDesc's getScopeDecodeOffset() returs 0
(DebugInformationRecorder.SERIALIZED_NULL). The current SA code doesn't
handle this case and goes on and tries to read ScopeDesc from the
DebugInfoReadStream at the bogus offset. From then on, bad things
happen. A NPE in StackTrace could be one symptom.
The same code in hotspot deals with serialized null differently. It
doesn't read from the debug info stream, and manually sets up a
reasonable frame. Note decode_body is called from ScopeDesc's
constructor where decode_offset might have been set to 0:
void ScopeDesc::decode_body() {
if (decode_offset() == DebugInformationRecorder::serialized_null) {
// This is a sentinel record, which is only relevant to
// approximate queries. Decode a reasonable frame.
_sender_decode_offset = DebugInformationRecorder::serialized_null;
_method = _code->method();
_bci = InvocationEntryBci;
_locals_decode_offset = DebugInformationRecorder::serialized_null;
_expressions_decode_offset =
DebugInformationRecorder::serialized_null;
_monitors_decode_offset =
DebugInformationRecorder::serialized_null;
} else {
// decode header
DebugInfoReadStream* stream = stream_at(decode_offset());
_sender_decode_offset = stream->read_int();
_method = stream->read_method();
_bci = stream->read_bci();
// decode offsets for body and sender
_locals_decode_offset = stream->read_int();
_expressions_decode_offset = stream->read_int();
_monitors_decode_offset = stream->read_int();
}
}
The proposed patch handles serialized null scopes similar to the
hotspot side of things, by returning a null scope. CompiledVFrame
already deals with null scopes when in debugging mode.
Bug: https://bugs.openjdk.java.net/browse/JDK-8196969
webrev:
http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8196969/03/webrev/
Testing: tier 1 tests on Linux x86_64 (release/fastdebug). jdk-submit
and ran various reproducer tests including 1000 interations of the
added regression test. All pass.
Thoughts?
Thanks,
Severin
