RFR: 6656: Allow capturing field values with path syntax

[Marcus Hirt]

On Tue, 17 Dec 2019 22:26:53 GMT, Kangcheng Xu <kxu at openjdk.org> wrote:

> This patch implements [JMC-6656: Allow capturing field values with path syntax](https://bugs.openjdk.java.net/browse/JMC-6656).
> 
> In the xml configuration, a field capture looks like:
> <field>
>   <name>a field value</name>
>   <description>a field value capture with a path syntax</description>
>   <expression>this.field.prop</expression>
> </field>
> See `org/openjdk/jmc/agent/test/jfrprobes_template.xml` for more examples.
> 
> There are currently two limitations to pay attention to:
> 
> 1. Instrumentation point cannot be in synthesized classes:
> Instrumented classes are first loaded by obtaining the bytecode and inspected reflectively to resolve typing information. This fails if the class load is unable to provide the bytecode when `ClassLoader.getResouce()` is called. The cases where it might fail are unlikely to be for classes like proxies or auxiliaries for Java frameworks that have no visible equivalent.
> 
> 2. Instrumentation is unable to access nestmates' private fields:
> Before [nest-base access control](https://openjdk.java.net/jeps/181) was introduced in Java 11, accessing nestmates' private fields is, while lexically correct, forbidden by the JVM. The Java compiler works around by inserting synthetic getters/setters. However, it's not for a BCI agent since changing the class structure is not allowed during class transformation.
> 
> Please let me know your thoughts. Thank you very much!

First quick review. Will take a closer look as soon as I can find some more time.

core/org.openjdk.jmc.agent/src/main/java/org/openjdk/jmc/agent/Attribute.java line 2:

> 1: /*
> 2:  * Copyright (c) 2018, Oracle and/or its affiliates. All rights reserved.
> 3:  *

2019

core/org.openjdk.jmc.agent/src/main/java/org/openjdk/jmc/agent/Field.java line 2:

> 1: /*
> 2:  * Copyright (c) 2018, Oracle and/or its affiliates. All rights reserved.
> 3:  *

2019

core/org.openjdk.jmc.agent/src/main/java/org/openjdk/jmc/agent/Transformer.java line 69:

> 68: 		registry.storeClassPreInstrumentation(className, classfileBuffer);
> 69: 		byte[] instrumentedClassfileBuffer = registry.isRevertIntrumentation() ?
> 70: 				registry.getClassPreInstrumentation(className) : classfileBuffer;

This is not related to this change, but this looks funny to me - why are we storing the class pre-instrumentation? To return to the pre-instrumented version should just be to redefine the class (i.e. run all transformers) without applying any transform.

core/org.openjdk.jmc.agent/src/main/java/org/openjdk/jmc/agent/Transformer.java line 73:

> 72: 			// Don't reuse this class loader instance. We want the loader and its class to unload after we're done.
> 73: 			classBeingRedefined = new InspectionClassLoader(loader).loadClass(TypeUtils.getCanonicalName(className));
> 74: 		} catch (ClassNotFoundException e) {

This means we'll load the class even if we don't need it. Any chance we could do this lazy when and if we need it?

core/org.openjdk.jmc.agent/src/main/java/org/openjdk/jmc/agent/util/AccessUtils.java line 2:

> 1: /*
> 2:  * Copyright (c) 2018, Oracle and/or its affiliates. All rights reserved.
> 3:  *

2019

core/org.openjdk.jmc.agent/src/main/java/org/openjdk/jmc/agent/util/AccessUtils.java line 45:

> 44: 
> 45: public class AccessUtils {
> 46: 	public static Field getFieldOnHierarchy(Class<?> clazz, String name) throws NoSuchFieldException {

Should probably be a final class. Class javadocs would be nice.

core/org.openjdk.jmc.agent/src/main/java/org/openjdk/jmc/agent/util/InspectionClassLoader.java line 2:

> 1: /*
> 2:  * Copyright (c) 2018, Oracle and/or its affiliates. All rights reserved.
> 3:  *

2019

core/org.openjdk.jmc.agent/src/main/java/org/openjdk/jmc/agent/util/InspectionClassLoader.java line 39:

> 38: 
> 39: // One-time use loader for reflective class inspection. Don't keep static reference to one of these.
> 40: public class InspectionClassLoader extends ClassLoader {

Use javadoc instead, so that tooling will render it in IDEs.

core/org.openjdk.jmc.agent/src/main/java/org/openjdk/jmc/agent/util/TypeUtils.java line 273:

> 272: 
> 273: 	public static Object getFrameVerificationType(Type type) {
> 274: 		switch (type.getSort()) {

In general, javadocs for public methods.

core/org.openjdk.jmc.agent/src/main/java/org/openjdk/jmc/agent/util/expression/IllegalSyntaxException.java line 2:

> 1: /*
> 2:  * Copyright (c) 2018, Oracle and/or its affiliates. All rights reserved.
> 3:  *

2019

core/org.openjdk.jmc.agent/src/main/java/org/openjdk/jmc/agent/util/expression/ReferenceChain.java line 2:

> 1: /*
> 2:  * Copyright (c) 2018, Oracle and/or its affiliates. All rights reserved.
> 3:  *

2019

core/org.openjdk.jmc.agent/src/main/java/org/openjdk/jmc/agent/util/expression/ReferenceChain.java line 40:

> 39: 
> 40: public class ReferenceChain {
> 41: 	private final Class<?> callerClass;

Javadocs for public classes, in general. Should probably be final.

core/org.openjdk.jmc.agent/src/main/java/org/openjdk/jmc/agent/util/expression/ReferenceChainElement.java line 2:

> 1: /*
> 2:  * Copyright (c) 2018, Oracle and/or its affiliates. All rights reserved.
> 3:  *

2019

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

Changes requested by hirt (Lead).

PR: https://git.openjdk.java.net/jmc/pull/20