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:09:03 UTC 2014
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