RFR: 8011397: JTREG needs to copy additional WhiteBox class file to JTwork/scratch/sun/hotspot
Andrey Zakharov
andrey.x.zakharov at oracle.com
Mon Jul 28 14:51:58 UTC 2014
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