RFR: 8011397: JTREG needs to copy additional WhiteBox class file to JTwork/scratch/sun/hotspot
Erik Helin
erik.helin at oracle.com
Wed Jul 16 10:39:03 UTC 2014
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.
>
> 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