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

Wed Aug 17 00:36:42 UTC 2016

 > I think these changes should absolutely not wait.  If we need 
modifications
 > to a few tests, we can do them in a small changeset.

I'm very much in agreement that this needs to be done now.

Dan

On 8/16/16 1:51 PM, George Triantafillou wrote:
> +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
>>>>
>>>>
>>>>
>>>>
>>>>
>>>
>>
>