RFR: Data plugin

[Alexandre Iline]

On Wed, 27 Apr 2022 01:54:06 GMT, Leonid Kuskov <lkuskov at openjdk.org> wrote:

>> * JREInstr
>>  * instrumentation plugin enhancement
>>  * build script
>>  * Implantable interface
>>  * support for all data types
>>  * other minor enhancements
>
> plugins/data_coverage/src/openjdk/jcov/data/Env.java line 63:
> 
>> 61:         Set<String> keys = System.getProperties().stringPropertyNames();
>> 62:         keys.stream().filter(k -> k.startsWith(prefix))
>> 63:                 .forEach(k -> System.clearProperty(k));
> 
> It's not good idea to clear/update system properties. Actually much better to use an "application" copy of them and interact with the copy as needed. Or after program shut down updated system properties could be used by an other application?

The way the plugin is designed, its behavior is controlled through the system properties. This is done this way to allow usage of jcov command line, such as Instr, JREInstr, etc, to specify how the code is instrumented, why code, where results are saved, etc. As such, it is _unavoidable_ to have the plugin code to _read_ the system properties. Here are a few examples of system properties which specifies the plugin behavior: Instrument.PLUGIN_CLASS, Instrument.JCOV_TEMPLATE, Collect.COVERAGE_OUT, Collect.COVERAGE_IN, Plugin.METHOD_FILTER, Collect.SERIALIZER

An additional level is added to the instrumentation to allow, during instrumentation time, to specify properties which will be needed during the  execution time. This is just a convenience which shortens command line needed to run the tests later. Instead of listing the properties on the command line when running the tests, the properties are stored within the instrumented code. This is, coincidently, a convenience which I would be handy for normal, non-plugin, Jcov functionality: to be able, during the instrumentation, specify a custom grabber port, to use an example. I thought, therefore, that the solution which I was suggesting was an elegant one, since the system properties which are set during the execution are exactly the same properties which the plugin is expected to be set as system properties to work.

I suppose it is possible to change the code of the plugin to try to load properties from a local storage rather than from the system properties. I will have to take a close and a hard look to see if it can be done nicely.

> plugins/data_coverage/src/openjdk/jcov/data/Env.java line 66:
> 
>> 64:     }
>> 65: 
>> 66:     public static String getStringEnv(String property, String defaultValue) {
> 
> defaultValue isn't used and could be omited. 
> line 70 of Class Collect  
> 
> if (!Env.getStringEnv(COVERAGE_IN, "").isEmpty()) {
> ``` 
> could be chaged to 
> 
> if( Env.getStringEnv(COVERAGE_IN) != null
> 
> Also isn't clear why you wraps IOException with UncheckedIOException here.

on line 115:
} else return defaultValue;

> plugins/data_coverage/src/openjdk/jcov/data/Env.java line 112:
> 
>> 110:                 String[] params = propValue.substring(ob + 1, cb).split(",");
>> 111:                 Class[] paramTypes = new Class[params.length];
>> 112:                 Arrays.fill(paramTypes, String.class);
> 
> Here the method MethodTypeDesc.ofDescriptor(String descriptor)could be used to parse method's descriptor if you use JVM notation for method descriptors like: (IDLjava/lang/Thread;)Ljava/lang/Object; for Object m(int i, double d, Thread t). Just in case if universal parser is needed here and params are not just Strings.

Indeed that would make things a lot easier! I would implement it as a separate enhancement, if you do not mind.

> plugins/data_coverage/src/openjdk/jcov/data/JREInstr.java line 73:
> 
>> 71:                 "-template", jcovTemplate,
>> 72:                 "-im", "java.base",
>> 73:                 jre};
> 
> Is Objects.requireNonNull() needed?  It could be that jcovRuntime, jcovTemplate or pluginClass is null.

Perhaps.  And also perhaps the JCov code needs to be enhanced to handle the nulls.

If an option value is null, from the code it looks like an NPE can be thrown out of EnvHandler.processOption(...) (line 299):
if (value.startsWith(PROP_FILE_SPECIFIER)) {

in another case nulls are handled: EnvHandler.addValuedOption(...) (line 383):
            throw new CLParsingException("Option '" + option + "' is invalid: value expected.");

> plugins/data_coverage/src/openjdk/jcov/data/arguments/instrument/MethodFilter.java line 50:
> 
>> 48:                 res.add(new Plugin.TypeDescriptor("[", "java/lang/Object", ALOAD));
>> 49:                 if(desc.charAt(pos + 1) == 'L') pos = desc.indexOf(";", pos) + 1;
>> 50:                 else pos = pos + 2;
> 
> Is the case [[[Ljava/lang/Object; covered?

not currently, there is more work to do. Hence the //TODO :)

> plugins/data_coverage/src/openjdk/jcov/data/arguments/runtime/Collect.java line 105:
> 
>> 103: 
>> 104:     static int countParams(String desc) {
>> 105:         if(!desc.startsWith("(")) throw new IllegalArgumentException("Not a method descriptor: " + desc);
> 
> I'm not sure. But this parser should be updated with MethodTypeDesc.ofDescriptor && parameterCount() That will support all changes at method descriptors like Q references.

Yes, as a separate enhancement.

-------------

PR: https://git.openjdk.java.net/jcov/pull/19