RFR: 8011397: JTREG needs to copy additional WhiteBox class file to JTwork/scratch/sun/hotspot
Andrey Zakharov
andrey.x.zakharov at oracle.com
Tue Jul 15 15:26:34 UTC 2014
Hi, Erik, Bengt. Could you, please, review this too.
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/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.t
>>>>>>>>>>>>>> gz
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> patch:
>>>>>>>>>>>>>> https://bugs.openjdk.java.net/secure/attachment/20274/8011397.W
>>>>>>>>>>>>>> hiteBoxPer
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> mission
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Thanks.
More information about the hotspot-dev
mailing list