RFR(XL): 8157957 - ClassNotFoundException: jdk.test.lib.JDKToolFinder
Christian Tornqvist
christian.tornqvist at oracle.com
Mon Aug 15 15:42:02 UTC 2016
Hi David,
>Do the jtreg authors agree with that rule? I'm happy to have such a simple
rule as long as it is agreed upon by all.
We've had some discussions about this, Jonathan says that explicit @build is
needed to support running with cached classes in jtwork and modifying the
classes in the @library path. This isn't something that is a very common
case in Hotspot and is avoidable by using a clean jtwork dir when modifying
these shared classes.
Think of it this way, when you run javac on your program Main.java that use
the class A, you don't have to explicitly run javac on A.java first, this is
the way the javac works and there's nothing different with jtreg here.
>Are these only: Platform.jdkLibPath(), Platform.sharedObjectName ?
>They seem general-purpose to me even if mosts tests don't need to care.
>So they seem suitable inclusions into the Platform class to me.
No, I was mostly talking about:
test/testlibrary/jdk/test/lib/dtrace/*
test/testlibrary/jdk/test/lib/AllocationHelper.java
test/testlibrary/jdk/test/lib/HeapRegionUsageTool.java
test/testlibrary/jdk/test/lib/InputArguments.java
I can put back the jdkLibPath and sharedObjectName code if we believe that
this is something than more than one test will need. I think we should be
careful about putting unnecessary code in the shared classes.
>why is / listed as a library ??
This is the compiler team's decision to use package name in their tests and
use / as the @library path, can't say I agree with it.
>Just to be crystal clear, did you also run all the @ignore'd tests?
No, that was on my list of things to do, which I forgot. Just ran it and
found issues with 2 of them that I corrected:
* Missing imports of Platform and JDKToolFinder in
runtime/NMT/MallocStressTest.java
* Extra "}" in serviceability/dcmd/jvmti/LoadAgentDcmdTest.java
Thanks,
Christian
-----Original Message-----
From: David Holmes [mailto:david.holmes at oracle.com]
Sent: Monday, August 15, 2016 12:58 AM
To: Christian Tornqvist <christian.tornqvist at oracle.com>;
hotspot-dev at openjdk.java.net
Subject: Re: RFR(XL): 8157957 - ClassNotFoundException:
jdk.test.lib.JDKToolFinder
Hi Christian,
First, thanks for attempting this, it is a huge effort!
Second, as this is changing the <root>/test/lib files then it needs to be
reviewed by more than just the hotspot group.
I won't claim to have examined every file in detail - there is a lot that
has to be taken on face-value here. I have skimmed the webrevs and patch
files.
A few comments in-lined below ...
On 13/08/2016 1:32 AM, Christian Tornqvist wrote:
> Hi everyone,
>
> Please review this fix for the CNFE issue we've had in the Hotspot
> jtreg tests. The two big culprits for the intermittent CNFE are:
> Duplicate classes on classpath with different content and @build tags
> causing class files to be written in bad places.
>
> There has been a lot of confusion around when there's a need to add
> @build to a test. In general it turns out it has been overused, which
> can lead to side effects. Here's an easy rule:
>
> If you run only that test in a clean jtwork folder and it passes, then
> there's no need for @build.
Do the jtreg authors agree with that rule? I'm happy to have such a simple
rule as long as it is agreed upon by all.
> If it doesn't pass, then it might need an explicit @build, here are
> some examples when it might be needed:
>
> * If the class is used by ClassFileInstaller and this is invoked by
> @run main ClassFileInstaller (sun.hotspot.Whitebox is an example of
> this)
>
> * If it's a class that that doesn't have reference from the Test
> itself (if "javac Test" wouldn't compile it, you might need to
> explicitly @build it)
>
> The change includes:
>
> * Removing all unnecessary @build tags, some of the CNFE was due to
> use of it to compile classes in /test/lib or /testlibrary when having
> different classpath (@library) set.
>
> * Changed @build <testname> to @build sun.hotspot.Whitebox
>
> * Moved /test/lib/share/classes/jdk/test/lib to /test/lib/jdk/test/lib
>
> * Some of the classes in /hotspot/test/testlibrary was only used by
> one or two tests, these classes shouldn't be part of the shared
> testlibrary code and they were moved to the location of the test
Are these only: Platform.jdkLibPath(), Platform.sharedObjectName ?
They seem general-purpose to me even if mosts tests don't need to care.
So they seem suitable inclusions into the Platform class to me.
>
> * Merged/moved the classes in /hotspot/test/testlibrary to /test/lib
>
> * Moved some of the /test/lib classes into packages
>
> * Changed @library /testlibrary to @library /test/lib
Related to that I see a number of entries like:
- * @library /testlibrary /test/lib /
+ * @library /test/lib /
this is not part of your change by why is / listed as a library ??
> * Changed imports from jdk.test.lib.* to explicit imports to help in
> future refactoring
>
> Almost all of the changes in here are in the jtreg @build and @library
> tags, very little code has been touched. Copyright headers will be
> updated before push.
>
> Testing done: Ran the entire hotspot/test/ folder multiple times
> locally and in RBT
Just to be crystal clear, did you also run all the @ignore'd tests?
> Webrev:
>
> http://cr.openjdk.java.net/~ctornqvi/webrev/8157957/webrev.00/
The jdk/test/* changes only seem to be for the @library changes in the path,
I don't see any changes to @build tags. Is that because all the @builds are
necessary or are you simply not taking on the task of also updating the jdk
tests?
> A recently added test required me to fix that as well, generating and
> uploading a new webrev of the Hotspot repo changes takes about 3h
> though, so here's the diff for that file:
>
> diff -r f37577c20a6b test/serviceability/sa/sadebugd/SADebugDTest.java
>
> --- a/test/serviceability/sa/sadebugd/SADebugDTest.java Wed Aug 10
> 21:02:14
> 2016 -0400
>
> +++ b/test/serviceability/sa/sadebugd/SADebugDTest.java Fri Aug 12
> +++ 11:25:54
> 2016 -0400
>
> @@ -26,7 +26,7 @@
>
> * @summary Checks that the jshdb debugd utility sucessfully starts
> * and tries to attach to a running process
> * @modules java.base/jdk.internal.misc
>
> - * @library /test/lib/share/classes
> + * @library /test/lib
> *
> * @run main/othervm SADebugDTest
> */
Looks fine.
Thanks,
David
-----
> Bug:
>
> https://bugs.openjdk.java.net/browse/JDK-8157957
>
> Thanks,
>
> Christian
>
>
>
>
>
More information about the jdk9-dev
mailing list