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

Tue Aug 16 19:51:26 UTC 2016

+1!

-George

On 8/15/2016 8:06 PM, Coleen Phillimore wrote:
>
> I Reviewed these changes (via patchfiles mostly).   They're an 
> enormous amount of work but they'll make our jobs writing and running 
> tests easier and less time consuming.  There shouldn't be compilcated 
> @build rules which are barriers to writing correct tests.
>
> The new rules are really simple.  In addition, we have tests with 
> .jcod or .jasm files which would require a @build directive but that 
> makes sense.
>
> I think these changes should absolutely not wait.  If we need 
> modifications to a few tests, we can do them in a small changeset. 
> This change will make testing more reliable, ie. less random 
> ClassNotFoundException bugs that are difficult and time consuming to 
> figure out, and we're going to need this as we get closer to the JDK9 
> deadline!
>
> Thanks,
> Coleen
>
>
> On 8/15/16 5:51 PM, Christian Tornqvist 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. 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, I was looking at 
>> doing this but then decided not to.
>>
>>> 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.
>>
>> 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
>>>
>>>
>>>
>>>
>>>
>>
>