RFR (xs) 8251209 [TESTBUG] CDS jvmti tests should use "@modules java.instrument"

[David Holmes]

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.

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
> 
>