RFR: 8011397: JTREG needs to copy additional WhiteBox class file to JTwork/scratch/sun/hotspot
Andrey Zakharov
andrey.x.zakharov at oracle.com
Mon Aug 11 13:55:53 UTC 2014
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