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 11:28:59 UTC 2014
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