RFR: 8011397: JTREG needs to copy additional WhiteBox class file to JTwork/scratch/sun/hotspot
Igor Ignatyev
igor.ignatyev at oracle.com
Mon Aug 11 11:18:41 UTC 2014
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