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 7 16:48:21 UTC 2014
Hi ,all
Mikael, can you please review it.
webrev:
http://cr.openjdk.java.net/~fzhinkin/azakharov/8011397/webrev.01/
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/share/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.html
>>>>> 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.tgz
>>>>>>>>>>>>
>>>>>>>>>>>> patch:
>>>>>>>>>>>> https://bugs.openjdk.java.net/secure/attachment/20274/8011397.WhiteBoxPer
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> mission
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> Thanks.
>>>>>>>>
>>>>>>>
>>>>>
>>>>
>>
>
More information about the hotspot-dev
mailing list