RFR: 8011397: JTREG needs to copy additional WhiteBox class file to JTwork/scratch/sun/hotspot
Mikael Gerdin
mikael.gerdin at oracle.com
Tue Aug 12 11:42:04 UTC 2014
On Monday 11 August 2014 17.55.53 Andrey Zakharov wrote:
> Hi, all
> Here is updated patch with recent added tests:
>
> * ./test/compiler/classUnloading/methodUnloading/TestMethodUnloading.java
> * ./test/gc/class_unloading/TestCMSClassUnloadingEnabledHWM.java
> * ./test/gc/class_unloading/TestG1ClassUnloadingHWM.java
>
> webrev:
> http://cr.openjdk.java.net/~fzhinkin/azakharov/8011397/webrev.04/
Ok,
/Mikael
> Bug:
> https://bugs.openjdk.java.net/browse/JDK-8011397
>
> Thanks.
>
> On 11.08.2014 15:28, Andrey Zakharov wrote:
> > Hi, Igor.
> > I have another counts with applied patch, but it looks like couple of
> > tests has beed added since last check.
> >
> > $ grep -Rl --exclude-dir=testlibrary
> > "sun.hotspot.WhiteBox\$WhiteBoxPermission" ./test/| wc -l
> > 133
> >
> > $ grep -Rl --exclude-dir=testlibrary "sun.hotspot.WhiteBox" ./test/|
> > wc -l
> > 142
> >
> > Will update patch soon.
> > Thanks.
> >
> > On 11.08.2014 15:18, Igor Ignatyev wrote:
> >> Andrey,
> >>
> >> could you please look at and fix other tests which use whitebox?
> >>
> >> $ grep -Rl --exclude-dir=testlibrary "sun.hotspot.WhiteBox" ./test |
> >> wc -l
> >> 139
> >> $ grep -Rl --exclude-dir=testlibrary
> >> "sun.hotspot.WhiteBox\$WhiteBoxPermission" ./test/ | wc -l
> >> 10
> >>
> >> Thanks
> >> Igor
> >>
> >> On 08/11/2014 03:09 PM, Andrey Zakharov wrote:
> >>> Hi, all
> >>>
> >>> Is it possible to review this below
> >>> webrev:
> >>> http://cr.openjdk.java.net/~fzhinkin/azakharov/8011397/webrev.03/
> >>> Bug:
> >>> https://bugs.openjdk.java.net/browse/JDK-8011397
> >>>
> >>> Thanks.
> >>>
> >>> On 28.07.2014 18:51, Andrey Zakharov wrote:
> >>>> Hi, all.
> >>>> I've prepared rechecked and verified fix for subject.
> >>>> webrev:
> >>>> http://cr.openjdk.java.net/~fzhinkin/azakharov/8011397/webrev.03/
> >>>> Bug:
> >>>> https://bugs.openjdk.java.net/browse/JDK-8011397
> >>>>
> >>>> I've grep'ed workspace by sun.hotspot.WhiteBox and fixed every file.
> >>>> Also I've updated copyright.
> >>>> Testing was done by aurora batch 538304.ute.hs_jtreg.accept.full
> >>>> It looks good.
> >>>>
> >>>> Thanks.
> >>>>
> >>>> On 16.07.2014 15:13, Andrey Zakharov wrote:
> >>>>> 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.WhiteBo
> >>>>> xPermission>
> >>>>>
> >>>>> 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 onlyhttps://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/001
> >>>>>>>>>>>>>>> 321.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