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

[Ioi Lam]

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.

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