RFR(XL): 8157957 - ClassNotFoundException: jdk.test.lib.JDKToolFinder

Tue Aug 16 20:50:40 UTC 2016

Hi David,

> 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?
The JDK part will have to be done as a separate change, as Dmitry pointed
out, there's another copy of the test library code in
/jdk/test/lib/testlibrary that should be merged down to /test/lib and the
@build's should be cleaned up as well.

Thanks,
Christian

-----Original Message-----
From: David Holmes [mailto:david.holmes at oracle.com] 
Sent: Monday, August 15, 2016 7:11 PM
To: Christian Tornqvist <christian.tornqvist at oracle.com>;
hotspot-dev at openjdk.java.net; jdk9-dev at openjdk.java.net
Subject: Re: RFR(XL): 8157957 - ClassNotFoundException:
jdk.test.lib.JDKToolFinder

Hi Christian,

Thanks for bring a broader audience into this. For reference to others the
thread started with:

http://mail.openjdk.java.net/pipermail/hotspot-dev/2016-August/024198.html

On 16/08/2016 1:42 AM, Christian Tornqvist wrote:
> 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.

It is a bit more complicated than that with libraries on different
sourcepaths, but I don't know the intricacies of javac's implicit
compilation checks. There are some obvious cases where reflection means
there is no compile-time dependency between classes, but I don't know if we
utilize anything of that nature in the test library (arguably we should in
places to ensure the library can function on compact profiles and with the
minimal VM - but that is a different problem).

I remain very wary of this approach because I know we had to add @build to
fix some missing library class problems.

>> 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:

Ah! I missed the significance of the renames ...

> test/testlibrary/jdk/test/lib/dtrace/*

Could be generally useful if all our dtrace tests were located together, but
as they are not ...

> test/testlibrary/jdk/test/lib/AllocationHelper.java

Potentially useful. We have a lot of tests that try to consume/exhaust
memory, but whether they are amenable to conversion to use this utility is
another matter.

> test/testlibrary/jdk/test/lib/HeapRegionUsageTool.java

Potentially useful.

> test/testlibrary/jdk/test/lib/InputArguments.java

This seems particularly useful, but also seems to have some overlap with
existing argument processing code.

> 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.

I don't have an issue with putting any kind of utility code (like the above
examples) into the shared test library if there is potential for sharing.
While retrofitting existing tests to use these things and remove duplication
would be a lot of effort, at least if they exist in the library someone
writing new tests and browsing the library won't be tempted to re-invent the
wheel.

So I'd vote for not changing these.

>> 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.

Not sure I even understand what it means!

>> 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

Ok.

You overlooked answering my query about the JDK test changes.

Thanks,
David

> 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
>>
>>
>>
>>
>>
>