RFR: 8011397: JTREG needs to copy additional WhiteBox class file to JTwork/scratch/sun/hotspot
Andrey Zakharov
andrey.x.zakharov at oracle.com
Wed Jul 16 11:13:14 UTC 2014
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 only https://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