Ok, my poor eyes. One short comment below.
On 2/4/2013 4:22 PM, Daniel D. Daugherty wrote:
Thanks for the additional test comments. Replies embedded below.
On 2/4/13 1:06 PM, Coleen Phillimore wrote:
On 2/4/2013 12:19 PM, Daniel D. Daugherty wrote:
Adding back dropped aliases and people...
I think I have all the aliases on this, this time.
And the individual people also. Thanks!
Thanks for reviewing the first test! Replies embedded below.
Ok, now I see about the scaffolding framework for the first test. I
can say it looks consistent with the other tests in that directory.
I reviewed the second test too. I have some questions.
in
http://cr.openjdk.java.net/~dcubed/8007420-webrev/0-jdk8-tl/test/java/lang/instrument/RedefineSubclassWithTwoInterfaces.sh.html
In lines 70-77 you move the same two files twice.
I don't think so. Getting rid of the
"RedefineSubclassWithTwoInterfaces" prefix will make the
real script code more apparent:
70 mv X_Target.java \
71 X_Target_1.java
72 mv X_Target.class \
73 X_Target_1.class
74 mv X_Impl.java \
75 X_Impl_1.java
76 mv X_Impl.class \
77 X_Impl_1.class
Also the file,
http://cr.openjdk.java.net/~dcubed/8007420-webrev/0-jdk8-tl/test/java/lang/instrument/RedefineSubclassWithTwoInterfacesImpl_1.java.html
Is exactly the same as the file
RedefineSubclassWithTwoInterfacesImpl.java
Isn't one supposed to be derived from Target_1 rather than Target or
different in some way?
The "_1" stuffis just for version naming purposes. The actual
class name has to be the same between Foo.java and Foo_1.java.
In this particular bug's case, we only needed to redefine
RedefineSubclassWithTwoInterfacesImpl with an EMCP version
to tickle the bug. That's why there are no differences
between RedefineSubclassWithTwoInterfacesImpl.java and
RedefineSubclassWithTwoInterfacesImpl_1.java.
Good eyes though.
Can you put a comment about why there are no differences but it's a
different file?
This second test makes more sense to me than the first anyway. Are
you going to wait for the fix to get propagated before checking it in?
Yes, I have to wait until the HSX fix has made it to the T&L repo
before I can push the tests. Integrating failing tests intentionally
is not permitted in T&L. Of course, you can integrate them disabled,
but then you have to go back and enable the test sometime.
Yes, this is somewhat of a drag.
Everything else looks fine.
Thanks!
Coleen
Dan
Thanks,
Coleen
On 2/4/13 8:09 AM, Coleen Phillimore wrote:
On 2/1/2013 6:55 PM, Daniel D. Daugherty wrote:
And here is the webrev for the new tests (relative to JDK8-T&L):
http://cr.openjdk.java.net/~dcubed/8007420-webrev/0-jdk8-tl/
I am so impressed that you were able to write a test for this.
Thanks. The shame of it is that I wrote this test back when I fixed
the 1-liner bug back in 2009. The test never got pushed to the JDK
repo so the regression was able to sneak in later. Sigh.
I can't follow this shell script at all. Who calls createJavaFile?
This test is written using the JDI ShellScaffold.sh infrastructure
in test/com/sun/jdi/ShellScaffold.sh (1106 lines of pure joy!).
createJavaFile() is a hook called by ShellScaffold.sh to create
the .java file that will be exercised by the test.
http://cr.openjdk.java.net/~dcubed/8007420-webrev/0-jdk8-tl/test/com/sun/jdi/RedefineAbstractClass.sh.html
I think a limited amount of duplication of Java code in the test
directory is worth having these scripts be more simple. It looks
like the only thing that substituted in the java test is the main
class, but lines 66 to 104 can be a stand-alone java source file.
No, the main class just has the breakpoints.
This is the line for the redefinition:
76 // @1 uncomment System.out.println("This is call " +
counter + " to checkFunc");
so the original version doesn't have this line and new version
has this line. The glue that makes all that work is in TestScaffold.sh.
I don't remember for sure, but I think JDI ShellScaffold.sh does not
work well with multiple .java files. It has been a long time since
I've written a test in this format. These JDI .sh tests are meant to
be standalone if need be. That's why this comment is always here:
143 # You could replace this next line with the contents
144 # of ShellScaffold.sh and this script will run just the same.
145 mysetup
Again, I can't really follow the logic here and I dread ever having
to debug this. Can you simplify this?
Which part? By definition, RedefineClasses tests are complicated
because
you have to have at least two versions of .class files, sometimes more.
This test is reproducing a bug using "jdb" so there is all the infra
that supports controlling "jdb".
I think this test is as simple as it can be given the context, but I'm
open to specific simplification suggestions.
Did you plan to review the java.lang.instrument (JLI) test? It's a bit
more convoluted because it is trying to mimic the WLS test case.
Dan
Thanks,
Coleen
As always, comments and suggestions are welcome.
Dan
On 2/1/13 4:39 PM, Daniel D. Daugherty wrote:
> There are two new tests that will be pushed to the JDK repos using
> a different bug ID (not yet filed):
New bug is now filed:
8007420 add test for 6805864 to com/sun/jdi, add test for
7182152
to java/lang/instrument
http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=8007420
https://jbs.oracle.com/bugs/browse/JDK-8007420
Of course, the tests cannot be pushed until the HSX changes have
made
it into a promoted build and thus available to JDK8-T&L.
Dan
On 2/1/13 12:55 PM, Daniel D. Daugherty wrote:
Greetings,
I have a fix for the following JVM/TI bug:
7182152 Instrumentation hot swap test incorrect monitor count
http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7182152
https://jbs.oracle.com/bugs/browse/JDK-7182152
The fix for the bug in the product code is one line:
src/share/vm/oops/klassVtable.cpp:
@@ -992,18 +1020,50 @@
// RC_TRACE macro has an embedded ResourceMark
RC_TRACE(0x00200000, ("itable method update: %s(%s)",
new_method->name()->as_C_string(),
new_method->signature()->as_C_string()));
}
- break;
+ // cannot 'break' here; see for-loop comment above.
}
ime++;
}
}
}
and is applicable to JDK7u10/HSX-23.6 and JDK7u14/HSX-24. Coleen
already fixed the bug as part of the Perm Gen Removal (PGR) project
in HSX-25. Yes, we found a 1-line bug fix buried in the monster PGR
changeset. Many thanks to Coleen for her help in this bug hunt!
The rest of the code in the webrevs are:
- additional JVM/TI tracing code backported from Coleen's PGR
changeset
- additional JVM/TI tracing code added by me and forward ported
to HSX-25
- a new -XX:TraceRedefineClasses=16384 flag value for finding these
elusive old or obsolete methods
- exposure of some printing code to the PRODUCT build so that
the new
tracing is available in a PRODUCT build
You might be wondering why the new tracing code is exposed in a
PRODUCT
build. Well, it appears that more and more PRODUCT bits
deployments are
using JVM/TI RedefineClasses() and/or RetransformClasses() at
run-time
to instrument their systems. This bug (7182152) was only
intermittently
reproducible in the WLS environment in which it occurred so I
made the
tracing available in a PRODUCT build to assist in the hunt.
Raj from the WLS team has also verified that the HSX-23.6
version of
fix resolves the issue in his environment. Thanks Raj!
Here are the URLs for the three webrevs:
http://cr.openjdk.java.net/~dcubed/7182152-webrev/0-hsx23.6/
http://cr.openjdk.java.net/~dcubed/7182152-webrev/0-hsx24/
http://cr.openjdk.java.net/~dcubed/7182152-webrev/0-hsx25/
I have run the following test suites from the JPDA stack on the
JDK7u10/HSX-23.6 version of the fix with
-XX:TraceRedefineClasses=16384
specified:
sdk-jdi
sdk-jdi_closed
sdk-jli
vm-heapdump
vm-hprof
vm-jdb
vm-jdi
vm-jdwp
vm-jvmti
vm-sajdi
The tested configs are:
{Solaris-X86, WinXP}
X {Client VM, Server VM}
X {-Xmixed, -Xcomp}
X {product, fastdebug}
With the 1-liner fix in place, the new tracing code does not
find any
instances of this failure mode in any of the above test suites.
Without
the the 1-liner fix in place, the new tracing code finds one
instance
of this failure mode in the above test suites:
test/java/lang/instrument/IsModifiableClassAgent.java
There are two new tests that will be pushed to the JDK repos using
a different bug ID (not yet filed):
test/com/sun/jdi/RedefineAbstractClass.sh
test/java/lang/instrument/RedefineSubclassWithTwoInterfaces.sh
There will be a separate review request for the new tests.
I'm currently running the JPDA stack of tests on the JDK7u14/HSX-24
and JDK8-B75/HSX-25 versions of the fix. That testing will likely
take all weekend to complete.
Thanks, in advance, for any comments and/or suggestions.
Dan
