RFR: 8011397: JTREG needs to copy additional WhiteBox class file to JTwork/scratch/sun/hotspot
Mikael Gerdin
mikael.gerdin at oracle.com
Tue Jul 15 13:58:49 UTC 2014
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