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

[David Holmes]

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