RFR: 8011397: JTREG needs to copy additional WhiteBox class file to JTwork/scratch/sun/hotspot
Vladimir Kozlov
vladimir.kozlov at oracle.com
Mon Aug 11 16:40:24 UTC 2014
Looks fine.
Thanks,
Vladimir
On 8/11/14 6:55 AM, Andrey Zakharov wrote:
> Hi, all
> Here is updated patch with recent added tests:
>
> * ./test/compiler/classUnloading/methodUnloading/TestMethodUnloading.java
> * ./test/gc/class_unloading/TestCMSClassUnloadingEnabledHWM.java
> * ./test/gc/class_unloading/TestG1ClassUnloadingHWM.java
>
> webrev:
> http://cr.openjdk.java.net/~fzhinkin/azakharov/8011397/webrev.04/
> Bug:
> https://bugs.openjdk.java.net/browse/JDK-8011397
>
> Thanks.
>
> On 11.08.2014 15:28, Andrey Zakharov wrote:
>> Hi, Igor.
>> I have another counts with applied patch, but it looks like couple of tests has beed added since last check.
>>
>> $ grep -Rl --exclude-dir=testlibrary "sun.hotspot.WhiteBox\$WhiteBoxPermission" ./test/| wc -l
>> 133
>>
>> $ grep -Rl --exclude-dir=testlibrary "sun.hotspot.WhiteBox" ./test/| wc -l
>> 142
>>
>> Will update patch soon.
>> Thanks.
>>
>>
>> On 11.08.2014 15:18, Igor Ignatyev wrote:
>>> Andrey,
>>>
>>> could you please look at and fix other tests which use whitebox?
>>>
>>> $ grep -Rl --exclude-dir=testlibrary "sun.hotspot.WhiteBox" ./test | wc -l
>>> 139
>>> $ grep -Rl --exclude-dir=testlibrary "sun.hotspot.WhiteBox\$WhiteBoxPermission" ./test/ | wc -l
>>> 10
>>>
>>> Thanks
>>> Igor
>>>
>>> On 08/11/2014 03:09 PM, Andrey Zakharov wrote:
>>>> Hi, all
>>>>
>>>> Is it possible to review this below
>>>> webrev:
>>>> http://cr.openjdk.java.net/~fzhinkin/azakharov/8011397/webrev.03/
>>>> Bug:
>>>> https://bugs.openjdk.java.net/browse/JDK-8011397
>>>>
>>>> Thanks.
>>>>
>>>> On 28.07.2014 18:51, Andrey Zakharov wrote:
>>>>> Hi, all.
>>>>> I've prepared rechecked and verified fix for subject.
>>>>> webrev:
>>>>> http://cr.openjdk.java.net/~fzhinkin/azakharov/8011397/webrev.03/
>>>>> Bug:
>>>>> https://bugs.openjdk.java.net/browse/JDK-8011397
>>>>>
>>>>> I've grep'ed workspace by sun.hotspot.WhiteBox and fixed every file.
>>>>> Also I've updated copyright.
>>>>> Testing was done by aurora batch 538304.ute.hs_jtreg.accept.full
>>>>> It looks good.
>>>>>
>>>>> Thanks.
>>>>>
>>>>>
>>>>> On 16.07.2014 15:13, Andrey Zakharov wrote:
>>>>>>
>>>>>> On 16.07.2014 14:39, Erik Helin wrote:
>>>>>>> On Tuesday 15 July 2014 19:26:34 PM Andrey Zakharov wrote:
>>>>>>>> Hi, Erik, Bengt. Could you, please, review this too.
>>>>>>> Andrey, why did you only update a couple of tests to also copy
>>>>>>> sun.hotspot.WhiteBox$WhiteBoxPermission? You updated 14 tests, there are
>>>>>>> still 116 tests using sun.hotspot.WhiteBox.
>>>>>>>
>>>>>>> Why doesn't these 116 tests have to be updated?
>>>>>>>
>>>>>>> Thanks,
>>>>>>> Erik
>>>>>> Thanks Erik. Actually this first one patch 8011397.WhiteBoxPermission
>>>>>> <https://bugs.openjdk.java.net/secure/attachment/20274/8011397.WhiteBoxPermission>
>>>>>> is correct. I will rework it and upload to webrev space.
>>>>>>
>>>>>>
>>>>>>>> Thanks.
>>>>>>>>
>>>>>>>> On 15.07.2014 17:58, Mikael Gerdin wrote:
>>>>>>>>> Andrey,
>>>>>>>>>
>>>>>>>>> On Monday 07 July 2014 20.48.21 Andrey Zakharov wrote:
>>>>>>>>>> Hi ,all
>>>>>>>>>> Mikael, can you please review it.
>>>>>>>>> Sorry, I was on vacation last week.
>>>>>>>>>
>>>>>>>>>> webrev:
>>>>>>>>>> http://cr.openjdk.java.net/~fzhinkin/azakharov/8011397/webrev.01/
>>>>>>>>> Looks ok for now. We should consider revisiting this by either switching
>>>>>>>>> to
>>>>>>>>> @run main/bootclasspath
>>>>>>>>> or
>>>>>>>>> deleting the WhiteboxPermission nested class and using some other way for
>>>>>>>>> permission checks (if they are at all needed).
>>>>>>>>>
>>>>>>>>> /Mikael
>>>>>>>>>
>>>>>>>>>> Thanks.
>>>>>>>>>>
>>>>>>>>>> On 25.06.2014 19:08, Andrey Zakharov wrote:
>>>>>>>>>>> Hi, all
>>>>>>>>>>> So in progress of previous email -
>>>>>>>>>>> webrev:
>>>>>>>>>>> http://cr.openjdk.java.net/~fzhinkin/azakharov/8011397/webrev.01/
>>>>>>>>>>>
>>>>>>>>>>> Thanks.
>>>>>>>>>>>
>>>>>>>>>>> On 16.06.2014 19:57, Andrey Zakharov wrote:
>>>>>>>>>>>> Hi, all
>>>>>>>>>>>> So issue is that when tests with WhiteBox API has been invoked with
>>>>>>>>>>>> -Xverify:all it fails with Exception java.lang.NoClassDefFoundError:
>>>>>>>>>>>> sun/hotspot/WhiteBox$WhiteBoxPermission
>>>>>>>>>>>> Solutions that are observed:
>>>>>>>>>>>> 1. Copy WhiteBoxPermission with WhiteBox. But
>>>>>>>>>>>>
>>>>>>>>>>>>>> Perhaps this is a good time to get rid of ClassFileInstaller
>>>>>>>>>>>> altogether?
>>>>>>>>>>>>
>>>>>>>>>>>> 2. Using bootclasspath to hook pre-built whitebox (due @library
>>>>>>>>>>>> /testlibrary/whitebox) . Some tests has @run main/othervm, some uses
>>>>>>>>>>>> ProcessBuilder.
>>>>>>>>>>>>
>>>>>>>>>>>> - main/othervm/bootclasspath adds ${test.src} and
>>>>>>>>>>>>
>>>>>>>>>>>> ${test.classes}to options.
>>>>>>>>>>>>
>>>>>>>>>>>> - With ProcessBuilder we can just add ${test.classes}
>>>>>>>>>>>>
>>>>>>>>>>>> Question here is, can it broke some tests ? While testing this, I
>>>>>>>>>>>> found onlyhttps://bugs.openjdk.java.net/browse/JDK-8046231, others
>>>>>>>>>>>> looks fine.
>>>>>>>>>>>>
>>>>>>>>>>>> 3. Make ClassFileInstaller deal with inner classes like that:
>>>>>>>>>>>> diff -r 6ed24aedeef0 -r c01651363ba8
>>>>>>>>>>>> test/testlibrary/ClassFileInstaller.java
>>>>>>>>>>>> --- a/test/testlibrary/ClassFileInstaller.java Thu Jun 05 19:02:56
>>>>>>>>>>>> 2014 +0400
>>>>>>>>>>>> +++ b/test/testlibrary/ClassFileInstaller.java Fri Jun 06 18:18:11
>>>>>>>>>>>> 2014 +0400
>>>>>>>>>>>> @@ -50,6 +50,16 @@
>>>>>>>>>>>>
>>>>>>>>>>>> }
>>>>>>>>>>>> // Create the class file
>>>>>>>>>>>> Files.copy(is, p, StandardCopyOption.REPLACE_EXISTING);
>>>>>>>>>>>>
>>>>>>>>>>>> +
>>>>>>>>>>>> + for (Class<?> cls :
>>>>>>>>>>>> Class.forName(arg).getDeclaredClasses()) {
>>>>>>>>>>>> + //if (!Modifier.isStatic(cls.getModifiers())) {
>>>>>>>>>>>> + String pathNameSub =
>>>>>>>>>>>> cls.getCanonicalName().replace('.', '/').concat(".class");
>>>>>>>>>>>> + Path pathSub = Paths.get(pathNameSub);
>>>>>>>>>>>> + InputStream streamSub =
>>>>>>>>>>>> cl.getResourceAsStream(pathNameSub);
>>>>>>>>>>>> + Files.copy(streamSub, pathSub,
>>>>>>>>>>>> StandardCopyOption.REPLACE_EXISTING);
>>>>>>>>>>>> + //}
>>>>>>>>>>>> + }
>>>>>>>>>>>> +
>>>>>>>>>>>>
>>>>>>>>>>>> }
>>>>>>>>>>>>
>>>>>>>>>>>> }
>>>>>>>>>>>>
>>>>>>>>>>>> }
>>>>>>>>>>>>
>>>>>>>>>>>> Works fine for ordinary classes, but fails for WhiteBox due
>>>>>>>>>>>> Class.forName initiate Class. WhiteBox has "static" section, and
>>>>>>>>>>>> initialization fails as it cannot bind to native methods
>>>>>>>>>>>> "registerNatives" and so on.
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> So, lets return to first one option? Just add everywhere
>>>>>>>>>>>>
>>>>>>>>>>>> * @run main ClassFileInstaller sun.hotspot.WhiteBox
>>>>>>>>>>>>
>>>>>>>>>>>> + * @run main ClassFileInstaller
>>>>>>>>>>>> sun.hotspot.WhiteBox$WhiteBoxPermission
>>>>>>>>>>>>
>>>>>>>>>>>> Thanks.
>>>>>>>>>>>>
>>>>>>>>>>>> On 10.06.2014 19:43, Igor Ignatyev wrote:
>>>>>>>>>>>>> Andrey,
>>>>>>>>>>>>>
>>>>>>>>>>>>> I don't like this idea, since it completely changes the tests.
>>>>>>>>>>>>> 'run/othervm/bootclasspath' adds all paths from CP to BCP, so the
>>>>>>>>>>>>> tests whose main idea was testing WB methods themselves (sanity,
>>>>>>>>>>>>> compiler/whitebox, ...) don't check that it's possible to use WB
>>>>>>>>>>>>> when the application isn't in BCP.
>>>>>>>>>>>>>
>>>>>>>>>>>>> Igor
>>>>>>>>>>>>>
>>>>>>>>>>>>> On 06/09/2014 06:59 PM, Andrey Zakharov wrote:
>>>>>>>>>>>>>> Hi, everybody
>>>>>>>>>>>>>> I have tested my changes on major platforms and found one bug, filed:
>>>>>>>>>>>>>> https://bugs.openjdk.java.net/browse/JDK-8046231
>>>>>>>>>>>>>> Also, i did another try to make ClassFileInstaller to copy all inner
>>>>>>>>>>>>>> classes within parent, but this fails for WhiteBox due its static
>>>>>>>>>>>>>> "registerNatives" dependency.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Please, review suggested changes:
>>>>>>>>>>>>>> - replace ClassFileInstaller and run/othervm with
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> "run/othervm/bootclasspath".
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> bootclasspath parameter for othervm adds-Xbootclasspath/a:
>>>>>>>>>>>>>> option with ${test.src} and ${test.classes}according to
>>>>>>>>>>>>>> http://hg.openjdk.java.net/code-tools/jtreg/file/31003a1c46d9/src/sha
>>>>>>>>>>>>>> re
>>>>>>>>>>>>>> /classes/com/sun/javatest/regtest/MainAction.java.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Is this suitable for our needs - give to test compiled WhiteBox?
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> - replace explicit -Xbootclasspath option values (".") in
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> ProcessBuilder invocations to ${test.classes} where WhiteBox has been
>>>>>>>>>>>>>> compiled.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Webrev:
>>>>>>>>>>>>>> http://cr.openjdk.java.net/~fzhinkin/azakharov/8011397/webrev.00/
>>>>>>>>>>>>>> Bug:https://bugs.openjdk.java.net/browse/JDK-8011397
>>>>>>>>>>>>>> Thanks.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> On 23.05.2014 15:40, Andrey Zakharov wrote:
>>>>>>>>>>>>>>> On 22.05.2014 12:47, Igor Ignatyev wrote:
>>>>>>>>>>>>>>>> Andrey,
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> 1. You changed dozen of tests, have you tested your changes?
>>>>>>>>>>>>>>> Locally, aurora on the way.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> 2. Your changes of year in copyright is wrong. it has to be
>>>>>>>>>>>>>>>> $first_year, [$last_year, ], see Mark's email[1] for details.
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> [1]
>>>>>>>>>>>>>>>> http://mail.openjdk.java.net/pipermail/jdk7-dev/2010-May/001321.htm
>>>>>>>>>>>>>>>> l
>>>>>>>>>>>>>>> Thanks, fixed. will be uploaded soon.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> Igor
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> On 05/21/2014 07:37 PM, Andrey Zakharov wrote:
>>>>>>>>>>>>>>>>> On 13.05.2014 14:43, Andrey Zakharov wrote:
>>>>>>>>>>>>>>>>>> Hi
>>>>>>>>>>>>>>>>>> So here is trivial patch -
>>>>>>>>>>>>>>>>>> removing ClassFileInstaller sun.hotspot.WhiteBox and adding
>>>>>>>>>>>>>>>>>> main/othervm/bootclasspath
>>>>>>>>>>>>>>>>>> where this needed
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> Also, some tests are modified as
>>>>>>>>>>>>>>>>>> - "-Xbootclasspath/a:.",
>>>>>>>>>>>>>>>>>> + "-Xbootclasspath/a:" +
>>>>>>>>>>>>>>>>>> System.getProperty("test.classes"),
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> Thanks.
>>>>>>>>>>>>>>>>> webrev:http://cr.openjdk.java.net/~jwilhelm/8011397/webrev.02/
>>>>>>>>>>>>>>>>> bug:https://bugs.openjdk.java.net/browse/JDK-8011397
>>>>>>>>>>>>>>>>> Thanks.
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> On 09.05.2014 12:13, Mikael Gerdin wrote:
>>>>>>>>>>>>>>>>>>> On Thursday 08 May 2014 19.28.13 Igor Ignatyev wrote:
>>>>>>>>>>>>>>>>>>>> // cc'ing hotspot-dev instaed of compiler, runtime and gc
>>>>>>>>>>>>>>>>>>>> lists.
>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>> On 05/08/2014 07:09 PM, Filipp Zhinkin wrote:
>>>>>>>>>>>>>>>>>>>>> Andrey,
>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>> I've CC'ed compiler and runtime mailing list, because you're
>>>>>>>>>>>>>>>>>>>>> changes
>>>>>>>>>>>>>>>>>>>>> affect test for other components as too.
>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>> I don't like your solution (but I'm not a reviewer, so treat
>>>>>>>>>>>>>>>>>>>>> my
>>>>>>>>>>>>>>>>>>>>> words
>>>>>>>>>>>>>>>>>>>>> just as suggestion),
>>>>>>>>>>>>>>>>>>>>> because we'll have to write more meta information for each
>>>>>>>>>>>>>>>>>>>>> test
>>>>>>>>>>>>>>>>>>>>> and it
>>>>>>>>>>>>>>>>>>>>> is very easy to
>>>>>>>>>>>>>>>>>>>>> forget to install WhiteBoxPermission if you don't test your
>>>>>>>>>>>>>>>>>>>>> test
>>>>>>>>>>>>>>>>>>>>> with
>>>>>>>>>>>>>>>>>>>>> some security manager.
>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>> From my point of view, it will be better to extend
>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>> ClassFileInstaller
>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>> so it will copy not only
>>>>>>>>>>>>>>>>>>>>> a class whose name was passed as an arguments, but also all
>>>>>>>>>>>>>>>>>>>>> inner
>>>>>>>>>>>>>>>>>>>>> classes of that class.
>>>>>>>>>>>>>>>>>>>>> And if someone want copy only specified class without inner
>>>>>>>>>>>>>>>>>>>>> classes,
>>>>>>>>>>>>>>>>>>>>> then some option
>>>>>>>>>>>>>>>>>>>>> could be added to ClassFileInstaller to force such behaviour.
>>>>>>>>>>>>>>>>>>> Perhaps this is a good time to get rid of ClassFileInstaller
>>>>>>>>>>>>>>>>>>> altogether?
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>> https://bugs.openjdk.java.net/browse/JDK-8009117
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>> The reason for its existence is that the WhiteBox class needs
>>>>>>>>>>>>>>>>>>> to be
>>>>>>>>>>>>>>>>>>> on the
>>>>>>>>>>>>>>>>>>> boot class path.
>>>>>>>>>>>>>>>>>>> If we can live with having all the test's classes on the boot
>>>>>>>>>>>>>>>>>>> class
>>>>>>>>>>>>>>>>>>> path then
>>>>>>>>>>>>>>>>>>> we could use the /bootclasspath option in jtreg as stated in
>>>>>>>>>>>>>>>>>>> the RFE.
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>> /Mikael
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>> Thanks,
>>>>>>>>>>>>>>>>>>>>> Filipp.
>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>> On 05/08/2014 04:47 PM, Andrey Zakharov wrote:
>>>>>>>>>>>>>>>>>>>>>> Hi!
>>>>>>>>>>>>>>>>>>>>>> Suggesting patch with fixes for
>>>>>>>>>>>>>>>>>>>>>> https://bugs.openjdk.java.net/browse/JDK-8011397
>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>> webrev:
>>>>>>>>>>>>>>>>>>>>>> https://bugs.openjdk.java.net/secure/attachment/20275/8011397
>>>>>>>>>>>>>>>>>>>>>> .t
>>>>>>>>>>>>>>>>>>>>>> gz
>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>> patch:
>>>>>>>>>>>>>>>>>>>>>> https://bugs.openjdk.java.net/secure/attachment/20274/8011397
>>>>>>>>>>>>>>>>>>>>>> .W
>>>>>>>>>>>>>>>>>>>>>> hiteBoxPer
>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>> mission
>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>> Thanks.
>>>>>>
>>>>>
>>>>
>>
>
More information about the hotspot-dev
mailing list