[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-serviceability-dev
Subject:    Re: RFR (xs) 8251209 [TESTBUG] CDS jvmti tests should use "@modules java.instrument"
From:       David Holmes <david.holmes () oracle ! com>
Date:       2020-08-07 6:41:58
Message-ID: 76bb64fd-bb2d-40ed-7624-b5fe8adce3c6 () oracle ! com
[Download RAW message or body]

On 7/08/2020 4:13 pm, Ioi Lam wrote:
> On 8/6/20 11:01 PM, David Holmes wrote:
> > On 7/08/2020 2:41 pm, Ioi Lam wrote:
> > > On 8/6/20 5:58 PM, David Holmes wrote:
> > > > Correction ...
> > > > 
> > > > On 7/08/2020 7:52 am, David Holmes wrote:
> > > > > Hi Ioi,
> > > > > 
> > > > > On 7/08/2020 4:25 am, Ioi Lam wrote:
> > > > > > https://bugs.openjdk.java.net/browse/JDK-8251209
> > > > > > http://cr.openjdk.java.net/~iklam/jdk16/8251209-cds-jvmti-tests-modules-tag.v01/ \
> > > > > >  
> > > > > > 
> > > > > > Summary -- changed the tests from (mis)using
> > > > > > 
> > > > > > * @requires vm.flavor != "minimal"
> > > > > > 
> > > > > > to
> > > > > > 
> > > > > > * @modules java.instrument
> > > > > > 
> > > > > > ... to be consistent with other jvmti tests.
> > > > > 
> > > > > That seems like an invalid precondition to me. It would have been 
> > > > > somewhat valid in the Compact Profiles world when we did not 
> > > > > provide "java.instrument" in the profiles which supported 
> > > > > MinimalVM, but you can define a minimal VM in a build that still 
> > > > > has all modules available. I don't think building the minimal VM 
> > > > > makes any changes to the supported modules.
> > > > > 
> > > > > Also AIUI the @modules statement simply adds the necessary 
> > > > > command-line args to use the java.instrument module (if present), 
> > > > > it doesn't ensure that the listed module has to be present.
> > > > 
> > > > It does in fact ensure that:
> > > > 
> > > > "Otherwise, a test will not be run if the system being tested does 
> > > > not contain all of the specified modules."
> > > > 
> > > > http://openjdk.java.net/jtreg/tag-spec.html
> > > > 
> > > > But as I said the module could be present in a JRE but you are still 
> > > > using the MinimalVM.
> > > > 
> > > 
> > > Hi David,
> > > 
> > > As I mentioned above, I am following the same rule as other jvmti 
> > > tests, which only use "@modules java.instrument" and do not check 
> > > whether the VM is minimal. E.g.,
> > > 
> > > http://hg.openjdk.java.net/jdk/jdk/file/4d36e29a5410/test/hotspot/jtreg/serviceability/jvmti/GetObjectSizeClass.java \
> > > 
> > 
> > 
> > 
> > Sure but I contend those tests are wrong and the tests you are 
> > changing are right (or more right given common test configurations).
> > 
> > > 
> > > -------
> > > 
> > > If I understand correctly, you're saying someone can build a minimal 
> > > JDK (configure --with-jvm-variants=minimal), and then try to add the 
> > > java.instrument module to it. I.e., adding the following module to 
> > > your JDK (with jlink, or by hand).
> > 
> > Just build a JDK with multiple VMs present.
> > 
> > > 
> > > $ unzip -l ./jmods/java.instrument.jmod
> > > Length      Date    Time    Name
> > > ---------  ---------- -----   ----
> > > 294  2020-08-04 17:03   classes/module-info.class
> > > 1102  2020-08-04 17:03 
> > > classes/sun/instrument/TransformerManager$TransformerInfo.class
> > > 4294  2020-08-04 17:03 
> > > classes/sun/instrument/TransformerManager.class
> > > 911  2020-08-04 17:03 
> > > classes/sun/instrument/InstrumentationImpl$1.class
> > > 16663  2020-08-04 17:03 
> > > classes/sun/instrument/InstrumentationImpl.class
> > > 1356  2020-08-04 17:03 
> > > classes/java/lang/instrument/ClassFileTransformer.class
> > > 554  2020-08-04 17:03 
> > > classes/java/lang/instrument/IllegalClassFormatException.class
> > > 1734  2020-08-04 17:03 
> > > classes/java/lang/instrument/Instrumentation.class
> > > 563  2020-08-04 17:03 
> > > classes/java/lang/instrument/UnmodifiableModuleException.class
> > > 970  2020-08-04 17:03 
> > > classes/java/lang/instrument/ClassDefinition.class
> > > 551  2020-08-04 17:03 
> > > classes/java/lang/instrument/UnmodifiableClassException.class
> > > 3244  2020-08-04 17:03   legal/COPYRIGHT
> > > 44  2020-08-04 17:03   legal/LICENSE
> > > 50920  2020-08-04 17:03 lib/libinstrument.so<<<<<<<<<
> > > 
> > > But this module has a native library, libinstrument.so, which 
> > > requires JVMTI to be present in libjvm.so. E.g.:
> > > 
> > > jvmtiEnv *
> > > retransformableEnvironment(JPLISAgent * agent) {
> > > ....
> > > jnierror = (*agent->mJVM)->GetEnv( agent->mJVM,
> > > (void **) &retransformerEnv,
> > > JVMTI_VERSION_1_1);
> > > 
> > > So if you try to run the CDS JVMTI test cases, it will be executed 
> > > (because your JDK says "I have java.instrument") and the test finds 
> > > out that your JDK's java.instrument module isn't working properly. So 
> > > the test is doing exactly what it's supposed to do.
> > 
> > The whole point of the @requires is to not waste time and resources 
> > running a test on a platform that cannot run the test successfully.
> > 
> > So the fully correct solution could be to have both settings:
> > 
> > @requires vm.flavor != "minimal"
> > @modules java.instrument
> > 
> > if you require both a VM that supports JVM TI and you need a JRE that 
> > includes the java.instrument module. But that assumes your test does 
> > need java.instrument. Not all JVM TI tests need java.instrument, but 
> > all instrumentation tests depend on JVM TI. Just looking at the first 
> > three of tests in your webrev I don't see any dependency on 
> > java.instrument - they are CDS only tests as far as I can see and so 
> > require a VM with CDS which means not a Minimal VM - though perhaps it 
> > is sufficient to have the
> > 
> > @requires vm.cds
> > 
> > in those cases?
> > 
> > For the other JVM TI related tests using -javaagent they probably need 
> > both @requires and @module.
> 
> You can disable JVMTI with "configure --disable-jvm-feature-jvmti". So 
> checking for vm.flavor != "minimal" is not sufficient.

Not it isn't but we don't try to accommodate every possible VM feature 
and module that someone could create a JRE with. Otherwise we will need 
an @require capability for every selectable feature and we would need to 
update every test to explicitly list every dependency.

But we do build a minimal VM and people can test the minimal VM, and if 
you build the minimal VM as part a multi-VM JRE then you do have 
java.instrument present. So only testing for the module, instead of 
testing for a non-minimal VM is not sufficient and changing tests to do 
that is a step in the wrong direction IMO.

Cheers,
David

> Similarly, "@requires vm.cds" doesn't guarantee that JVMTI is supported.
> 
> Maybe we should have a new VM prop so we can do
> 
> @requires vm.jvmti
> @modules java.instrument
> 
> Thanks
> - Ioi
> 
> 
> 
> > 
> > David
> > -----
> > 
> > > I would argue that this is better than before (which would exclude 
> > > the test when the libjvm.so is a minimal build, and would will not 
> > > detect such a mis-configured java.instrument module.)
> > > 
> > > 
> > > Thanks
> > > - Ioi
> > > 
> > > 
> 


[prev in list] [next in list] [prev in thread] [next in thread]