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

Igor Ignatyev igor.ignatyev at oracle.com
Tue Aug 16 10:45:06 UTC 2016 

Hi Christian,

> On Aug 16, 2016, at 12:51 AM, Christian Tornqvist <christian.tornqvist at oracle.com> wrote:
> 
> Hi Igor,
> 
>> 1. why have you put InfiniteLoop and TimeLimitedRunner into apps package? it looks like this package contained classes which are application: LingeredApp and LingeredAppWithDeadlock, neither TimeLimitedRunner >nor InfiniteLoop is an application. ClassFileInstaller, on contrary, is an application, but it’s in default package.
> But they're wrappers for other things, we could place them in jdk.test.lib.wrappers if that makes you happier.
jdk.test.lib.wrappers or just jdk.test.lib is fine w/ me.

> ClassFileInstaller is a utility used by @run, so putting this in a package would just make it noisier to write tests using this (all the Whitebox tests right now). In the future, we may be able to change how we use Whitebox in jtreg to get rid of the whole @build/@run/@run chain.
> 
>> 2. AYK, we have tests for our testlibraries, I think those tests should be moved from hotspot/test/testlibrary_tests/ to root/test/testlibrary_tests/ together w/ libraries. btw, RedefineClassHelper has a reference to >test/testlibrary_tests/RedefineClassTest.java
> This requires creating a new Test root in /test
right and we will also need to modify our test definitions to run these tests if we want. AYCS we already have a test for LingeredApp root/test/lib-test, so now we have 2 places w/ tests for testlibraries, but we can clean up that later, could you please file an RFE?

>> 1. in some tests you haven’t replaced @build <test> w/ @buid s.h.WhiteBox, e.g. test/compiler/calls/fromInterpreted/InterpretedInvokeSpecial2NativeTest.java still has '@build compiler.calls.common.InvokeSpecial' >however the test uses the same code as test/compiler/calls/fromInterpreted/InterpretedInvokeSpecial2InterpretedTest and you made replacement in latter.
> I most likely just missed replacing that @build, I'll look at it tomorrow.
> 
>> 2. in some tests you haven’t removed ‘@build s.h.WhiteBox’ when it should be possible, e.g. test/compiler/jsr292/NonInlinedCall/InvokeTest.java. the test doesn’t use ClassFileInstaller or reflection to get s.h.WhiteBox, >so its dependency on WhiteBox class is statically computable and hence there is no need to have s.h.WhiteBox in @build action.
> The problem is that as soon as someone starts @build:ing a class, it gets really tricky, especially when there are inheritance and different classpaths (@library) involved. So the safest way to resolve this is to make sure everyone @build sun.hotspot.Whitebox 
> 
>> what I’m saying is it’s hard to correct all @build in all our tests and I don’t think it’s safe to do it now given the size, intermittent nature of possible bugs and how close we are to RDP. so I’d prefer to postpone @build >part of change till jdk 10.
> The @build are in most cases the cause of the issue, so not fixing this would mean not fixing this issue.
I see.
> 
> Thanks,
> Christian
> -----Original Message-----
> From: Igor Ignatyev [mailto:igor.ignatyev at oracle.com] 
> Sent: Monday, August 15, 2016 5:13 PM
> To: Christian Tornqvist <christian.tornqvist at oracle.com>
> Cc: hotspot-dev at openjdk.java.net
> Subject: Re: RFR(XL): 8157957 - ClassNotFoundException: jdk.test.lib.JDKToolFinder
> 
> Hi Christian,
> 
> thank you for your tremendous work and sorry that you had to redo it ‘cuz of me )
> 
> I’ve not finished review yet, but in order to speed up things a little bit, here is my questions/comments so far:
> 
> - http://cr.openjdk.java.net/~ctornqvi/webrev/8157957/webrev.00/root
> 1. why have you put InfiniteLoop and TimeLimitedRunner into apps package? it looks like this package contained classes which are application: LingeredApp and LingeredAppWithDeadlock, neither TimeLimitedRunner nor InfiniteLoop is an application. ClassFileInstaller, on contrary, is an application, but it’s in default package.
> 2. AYK, we have tests for our testlibraries, I think those tests should be moved from hotspot/test/testlibrary_tests/ to root/test/testlibrary_tests/ together w/ libraries. btw, RedefineClassHelper has a reference to test/testlibrary_tests/RedefineClassTest.java
> 
> - http://cr.openjdk.java.net/~ctornqvi/webrev/8157957/webrev.00/hotspot
> while I was looking thru the changeset I've noticed that 1. in some tests you haven’t replaced @build <test> w/ @buid s.h.WhiteBox, e.g. test/compiler/calls/fromInterpreted/InterpretedInvokeSpecial2NativeTest.java still has '@build compiler.calls.common.InvokeSpecial' however the test uses the same code as test/compiler/calls/fromInterpreted/InterpretedInvokeSpecial2InterpretedTest and you made replacement in latter.
> 2. in some tests you haven’t removed ‘@build s.h.WhiteBox’ when it should be possible, e.g. test/compiler/jsr292/NonInlinedCall/InvokeTest.java. the test doesn’t use ClassFileInstaller or reflection to get s.h.WhiteBox, so its dependency on WhiteBox class is statically computable and hence there is no need to have s.h.WhiteBox in @build action.
> 
> what I’m saying is it’s hard to correct all @build in all our tests and I don’t think it’s safe to do it now given the size, intermittent nature of possible bugs and how close we are to RDP. so I’d prefer to postpone @build part of change till jdk 10.
> 
> Thanks,
> — Igor
> 
>> On Aug 12, 2016, at 6:32 PM, Christian Tornqvist <christian.tornqvist at oracle.com> 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.
>> 
>> 
>> 
>> 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
>> 
>> * 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
>> 
>> * 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
>> 
>> 
>> 
>> Webrev:
>> 
>> http://cr.openjdk.java.net/~ctornqvi/webrev/8157957/webrev.00/
>> 
>> 
>> 
>> 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
>> 
>> */
>> 
>> 
>> 
>> Bug:
>> 
>> https://bugs.openjdk.java.net/browse/JDK-8157957
>> 
>> 
>> 
>> Thanks,
>> 
>> Christian
>> 
>> 
>> 
>> 
>> 
> 
>

More information about the hotspot-dev mailing list