On 3/12/15 11:27 PM, serguei.spit...@oracle.com wrote:
Please, review the jdk 9 fix for:
https://bugs.openjdk.java.net/browse/JDK-8067662
Open hotspot webrev:
http://cr.openjdk.java.net/~sspitsyn/webrevs/2015/hotspot/8067662-JVMTI-trace.1/
Thumbs up. None of these comments are critical.
src/share/vm/classfile/javaClasses.hpp
No comments.
src/share/vm/classfile/javaClasses.cpp
line 1316: return method != NULL &&
(method->constants()->version() == version
line 1317: && version < MAX_VERSION);
Don't really need the parens and the indenting implies
a logical grouping that isn't really there. Perhaps:
return method != NULL && method->constants()->version() ==
version
&& version < MAX_VERSION;
Although I also wonder why the caller would pass a 'version'
value that is >= MAX_VERSION. Perhaps this instead:
assert(version < MAX_VERSION, "version is too big");
return method != NULL && method->constants()->version() ==
version
line 1347: typeArrayOop _cprefs;
Perhaps add a comment: // needed to insulate method name
against redefinition
line 1486: holder = holder->get_klass_version(version); // Use
specific ik version as a holder
Perhaps the following comment instead:
// Use specific ik version as a holder since the mirror might
// refer to version that is now obsolete.
I'm not sure "obsolete" is the right term here, but say
something about why the mirror version is not good enough.
Update: 'obsolete' is the right term now that I've re-read the
code review invite! Perhaps this is better:
// Use specific ik version as a holder since the mirror might
// refer to version that is now obsolete and no longer accessible
// via the previous versions list.
line 1854: // Get method id, cpref, bci, version and mirror from
chunk
Perhaps:
// Get method id, bci, version, mirror and cpref from chunk
since that's the order you do the work...
line 1909: holder = holder->get_klass_version(version); // Use
specific ik version as a holder
Similar comment as on line 1486.
src/share/vm/oops/instanceKlass.hpp
No comments.
src/share/vm/oops/instanceKlass.cpp
No comments.
Open webrev for unit test update:
http://cr.openjdk.java.net/~sspitsyn/webrevs/2015/jdk/8067662-JVMTI-trace-test.1/
test/java/lang/instrument/RedefineMethodInBacktrace.sh
No comments.
test/java/lang/instrument/RedefineMethodInBacktraceApp.java
line 175: System.exit(1);
A comment to explain why this test needs to exit on
failure would be good. When the work to get rid of
shell script driven tests gets here, it would be
useful for understanding why this test must be
standalone.
test/java/lang/instrument/RedefineMethodInBacktraceTarget.java
No comments.
test/java/lang/instrument/RedefineMethodInBacktraceTargetB.java
No comments.
test/java/lang/instrument/RedefineMethodInBacktraceTargetB_2.java
No comments.
test/java/lang/instrument/RedefineMethodInBacktraceTarget_2.java
No commetns.
Dan
Summary:
An NPE is trown in a thread dumping via JMX when doing CPU profiling
in NetBeans Profiler and VisualVM.
The issue is that the methods and related klass versions are not
kept alive if a class redefinition
takes place between catching a Throwable and taking a thread dump.
It can result in loosing an obsolete method, and so, the
reconstruction of method name becomes impossible.
In such a case, the null reference is returned instead of a valid
method name.
The solution is to use current klass version and method orig_idnum
instead of ordinary method idnum
to find required method pointer. In the worst case when the related
klass version is gone
(was not included to or was removed from the previous_versions
linked list), a saved method name
CP index of the latest klass version can be used to restore the
method name.
The footprint extra overhead for this approach is u2 per stack frame.
The plan is also to backport the fix to 8u60.
Testing:
In progress: nsk redefine classes tests, JTREG java/lang/instrument
Thanks,
Serguei
