7008: Agent should properly support Class and Thread

Reviewed-by: jmatsuoka, ghb

agent/src/main/java/org/openjdk/jmc/agent/Agent.java

@@ -51,14 +51,13 @@
 import org.openjdk.jmc.agent.util.ModuleUtils;
 
 /**
- * Small ASM based byte code instrumentation agent for declaratively adding logging and JFR events.
- * Note: This agent is currently work in progress, and it is not supported for production use yet.
+ * Small ASM based byte code instrumentation agent for declaratively adding JFR events.
  */
 public class Agent {
 	/**
 	 * This should be generated as part of the build later.
 	 */
-	public static final String VERSION = "0.9.0"; //$NON-NLS-1$
+	public static final String VERSION = "1.0.0"; //$NON-NLS-1$
 	private static boolean loadedDynamically = false;
 
 	/**

agent/src/main/java/org/openjdk/jmc/agent/jfr/JFRTransformDescriptor.java

@@ -239,7 +239,7 @@ public boolean isAllowedEventFieldType(Convertable convertable, Type type) {
 		if (isAllowConverter() && convertable.hasConverter()) {
 			return true;
 		}
-		return type.getSort() != Type.OBJECT && type.getSort() != Type.ARRAY;
+		return TypeUtils.isSupportedType(type);
 	}
 
 	public void matchFound(boolean matched) {

agent/src/main/java/org/openjdk/jmc/agent/jfr/impl/JFREventClassGenerator.java

@@ -46,7 +46,6 @@
 import org.openjdk.jmc.agent.Attribute;
 import org.openjdk.jmc.agent.Field;
 import org.openjdk.jmc.agent.Parameter;
-import org.openjdk.jmc.agent.ReturnValue;
 import org.openjdk.jmc.agent.impl.MalformedConverterException;
 import org.openjdk.jmc.agent.impl.ResolvedConvertable;
 import org.openjdk.jmc.agent.jfr.JFRTransformDescriptor;
@@ -82,7 +81,7 @@ private static void generateAttributeFields(ClassWriter cw, JFRTransformDescript
 			createField(cw, td, param, args[param.getIndex()]);
 		}
 		if (td.getReturnValue() != null) {
-			createField(cw, td, Type.getReturnType(td.getMethod().getSignature()));
+			createField(cw, td, td.getReturnValue(), Type.getReturnType(td.getMethod().getSignature()));
 		}
 
 		for (Field field : td.getFields()) {
@@ -104,7 +103,7 @@ private static void createField(ClassWriter cw, JFRTransformDescriptor td, Attri
 			ResolvedConvertable resolved;
 			try {
 				resolved = new ResolvedConvertable(attribute.getConverterDefinition(), type);
-				fieldType = getFieldType(Type.getType(resolved.getConverterMethod().getReturnType()));
+				fieldType = getFieldTypeDescriptor(Type.getType(resolved.getConverterMethod().getReturnType()));
 			} catch (MalformedConverterException e) {
 				Agent.getLogger()
 						.log(Level.SEVERE,
@@ -114,7 +113,7 @@ private static void createField(ClassWriter cw, JFRTransformDescriptor td, Attri
 				return;
 			}
 		} else {
-			fieldType = getFieldType(type);
+			fieldType = getFieldTypeDescriptor(type);
 		}
 
 		FieldVisitor fv = cw.visitField(Opcodes.ACC_PROTECTED, attribute.getFieldName(), fieldType, null, null);
@@ -146,46 +145,6 @@ private static void createField(ClassWriter cw, JFRTransformDescriptor td, Attri
 		fv.visitEnd();
 	}
 
-	private static void createField(ClassWriter cw, JFRTransformDescriptor td, Type type) {
-		ReturnValue returnValue = td.getReturnValue();
-		if (!td.isAllowedEventFieldType(returnValue, type)) {
-			Logger.getLogger(JFREventClassGenerator.class.getName())
-					.warning("Skipped generating field in event class for return value " + returnValue + " and type " //$NON-NLS-1$//$NON-NLS-2$
-							+ type + " because of configuration settings!"); //$NON-NLS-1$
-			return;
-		}
-
-		String fieldType = getFieldType(type);
-
-		FieldVisitor fv = cw.visitField(Opcodes.ACC_PROTECTED, returnValue.getFieldName(), fieldType, null, null);
-
-		// Name
-		AnnotationVisitor av = fv.visitAnnotation("Ljdk/jfr/Label;", true);
-		av.visit("value", returnValue.getName());
-		av.visitEnd();
-
-		// Description
-		av = fv.visitAnnotation("Ljdk/jfr/Description;", true);
-		av.visit("value", returnValue.getDescription());
-		av.visitEnd();
-
-		// "ContentType"
-		// We support the old JDK 7 style content types transparently.
-		// We also support user defined content types and a single string value annotation parameter to the annotation.
-		String contentTypeAnnotation = getContentTypeAnnotation(returnValue.getContentType());
-		if (contentTypeAnnotation != null) {
-			String[] contentTypeAnnotationInfo = contentTypeAnnotation.split(";");
-			av = fv.visitAnnotation(contentTypeAnnotationInfo[0] + ";", true);
-			if (contentTypeAnnotationInfo.length > 1) {
-				av.visit("value", contentTypeAnnotationInfo[1]);
-			}
-			av.visitEnd();
-		}
-
-		// FIXME: RelKey
-		fv.visitEnd();
-	}
-
 	private static String getContentTypeAnnotation(String contentType) {
 		if (contentType == null) {
 			return null;
@@ -220,8 +179,8 @@ private static String getContentTypeAnnotation(String contentType) {
 		}
 	}
 
-	private static String getFieldType(Type type) {
-		if (type.getSort() == Type.OBJECT || type.getSort() == Type.ARRAY) {
+	private static String getFieldTypeDescriptor(Type type) {
+		if (!TypeUtils.isSupportedType(type)) {
 			return "Ljava/lang/String;"; //$NON-NLS-1$
 		}
 		return type.getDescriptor();

agent/src/main/java/org/openjdk/jmc/agent/jfr/impl/JFRMethodAdvisor.java

@@ -150,9 +150,9 @@ private void createEvent() throws IllegalSyntaxException, MalformedConverterExce
 				if (param.hasConverter()) {
 					argumentType = convertify(mv, param, argumentType);
 				} else {
-					if (TypeUtils.shouldStringify(param, argumentType)) {
+					if (!TypeUtils.isSupportedType(argumentType) && transformDescriptor.isAllowToString()) {
 						TypeUtils.stringify(mv);
-						argumentType = TypeUtils.STRING_TYPE;
+						argumentType = TypeUtils.TYPE_STRING;
 					}
 				}
 				writeAttribute(param, argumentType);
@@ -176,9 +176,9 @@ private void createEvent() throws IllegalSyntaxException, MalformedConverterExce
 				if (field.hasConverter()) {
 					fieldType = convertify(mv, field, fieldType);
 				} else {
-					if (TypeUtils.shouldStringify(field, fieldType)) {
+					if (!TypeUtils.isSupportedType(fieldType) && transformDescriptor.isAllowToString()) {
 						TypeUtils.stringify(mv);
-						fieldType = TypeUtils.STRING_TYPE;
+						fieldType = TypeUtils.TYPE_STRING;
 					}
 				}
 
@@ -267,6 +267,11 @@ private void writeAttribute(Attribute param, Type type) {
 
 	private Type convertify(MethodVisitor mv, Attribute convertable, Type type) throws MalformedConverterException {
 		ResolvedConvertable resolvedConvertable = new ResolvedConvertable(convertable.getConverterDefinition(), type);
+		return convertify(mv, resolvedConvertable, type);
+	}
+
+	private Type convertify(MethodVisitor mv, ResolvedConvertable resolvedConvertable, Type type)
+			throws MalformedConverterException {
 		mv.visitMethodInsn(Opcodes.INVOKESTATIC, Type.getInternalName(resolvedConvertable.getConverterClass()),
 				resolvedConvertable.getConverterMethod().getName(),
 				Type.getMethodDescriptor(resolvedConvertable.getConverterMethod()), false);
@@ -310,13 +315,13 @@ private void emitSettingReturnParam(int opcode, ReturnValue returnValue) throws
 		if (returnValue.hasConverter()) {
 			returnType = convertify(mv, returnValue, returnType);
 		} else {
-			if (TypeUtils.shouldStringify(returnValue, returnType)) {
+			if (!TypeUtils.isSupportedType(returnType)) {
 				TypeUtils.stringify(mv);
-				returnType = TypeUtils.STRING_TYPE;
+				returnType = TypeUtils.TYPE_STRING;
 			}
 		}
 
-		writeAttribute(returnValue, returnTypeRef);
+		writeAttribute(returnValue, returnType);
 	}
 
 	private void commitEvent() {

agent/src/main/java/org/openjdk/jmc/agent/jfrlegacy/impl/JFRLegacyMethodAdvisor.java

@@ -126,7 +126,7 @@ private void createEvent() throws IllegalSyntaxException {
 			if (transformDescriptor.isAllowedEventFieldType(param, argumentType)) {
 				mv.visitInsn(DUP);
 				loadArg(param.getIndex());
-				writeAttribute(param, argumentType);
+				writeAttribute(param, argumentType, transformDescriptor.isAllowToString());
 			}
 		}
 
@@ -141,7 +141,7 @@ private void createEvent() throws IllegalSyntaxException {
 			if (transformDescriptor.isAllowedEventFieldType(field, refChain.getType())) {
 				mv.visitInsn(DUP);
 				loadField(refChain);
-				writeAttribute(field, refChain.getType());
+				writeAttribute(field, refChain.getType(), transformDescriptor.isAllowToString());
 			}
 		}
 
@@ -220,10 +220,10 @@ private void loadField(ReferenceChain refChain) {
 						TypeUtils.getFrameVerificationType(type)});
 	}
 
-	private void writeAttribute(Attribute param, Type type) {
-		if (TypeUtils.shouldStringify(param, type)) {
+	private void writeAttribute(Attribute param, Type type, boolean allowToString) {
+		if (!TypeUtils.isSupportedType(type) && allowToString) {
 			TypeUtils.stringify(mv);
-			type = TypeUtils.STRING_TYPE;
+			type = TypeUtils.TYPE_STRING;
 		}
 		putField(Type.getObjectType(transformDescriptor.getEventClassName()), param.getFieldName(), type);
 	}
@@ -254,7 +254,7 @@ private void emitSettingReturnParam(int opcode, ReturnValue returnValue) {
 			dupX2();
 			pop();
 		}
-		writeAttribute(returnValue, returnTypeRef);
+		writeAttribute(returnValue, returnTypeRef, transformDescriptor.isAllowToString());
 	}
 
 	private void commitEvent() {

agent/src/main/java/org/openjdk/jmc/agent/util/TypeUtils.java

@@ -44,7 +44,6 @@
 import org.objectweb.asm.Opcodes;
 import org.objectweb.asm.Type;
 import org.openjdk.jmc.agent.Agent;
-import org.openjdk.jmc.agent.Convertable;
 import org.openjdk.jmc.agent.jfrlegacy.impl.JFRUtils;
 
 /**
@@ -56,11 +55,14 @@ public final class TypeUtils {
 	 * The internal name of this class.
 	 */
 	public static final String INAME = Type.getInternalName(TypeUtils.class);
-	public static final Type OBJECT_TYPE = Type.getObjectType("java/lang/Object"); //$NON-NLS-1$
-	public static final Type OBJECT_ARRAY_TYPE = Type.getObjectType("[Ljava/lang/Object;"); //$NON-NLS-1$
-	public static final Type STRING_TYPE = Type.getType("Ljava/lang/String;"); //$NON-NLS-1$
 
-	public static final Object STRING_INTERNAL_NAME = "java/lang/String"; //$NON-NLS-1$
+	public static final Type TYPE_OBJECT = Type.getObjectType("java/lang/Object"); //$NON-NLS-1$
+	public static final Type TYPE_OBJECT_ARRAY = Type.getObjectType("[Ljava/lang/Object;"); //$NON-NLS-1$
+	public static final Type TYPE_STRING = Type.getType("Ljava/lang/String;"); //$NON-NLS-1$
+
+	public static final String INTERNAL_NAME_STRING = "java/lang/String"; //$NON-NLS-1$
+	public static final String INTERNAL_NAME_THREAD = Type.getInternalName(Thread.class);
+	public static final String INTERNAL_NAME_CLASS = Type.getInternalName(Class.class);
 
 	private static final String UNSAFE_JDK_7_CLASS = "sun.misc.Unsafe"; //$NON-NLS-1$
 	private static final String UNSAFE_JDK_11_CLASS = "jdk.internal.misc.Unsafe"; //$NON-NLS-1$
@@ -218,13 +220,6 @@ public static void stringify(MethodVisitor mv) {
 				"(Ljava/lang/Object;)Ljava/lang/String;", false); //$NON-NLS-1$
 	}
 
-	public static boolean shouldStringify(Convertable convertable, Type argumentType) {
-		if (argumentType.getSort() == Type.ARRAY || argumentType.getSort() == Type.OBJECT) {
-			return !argumentType.getInternalName().equals(STRING_INTERNAL_NAME);
-		}
-		return false;
-	}
-
 	/**
 	 * Transforms a FQN in internal form, so that it can be used in e.g. formal descriptors.
 	 *
@@ -298,8 +293,8 @@ public static int getConstZeroOpcode(Type type) {
 	 * frame verification of a given type.
 	 * 
 	 * @param type
-	 *            the type of the element on the operand stack or in the local variable table
-	 * @return a array element for <code>MethodVisitor.visitFrame()</code>'s parameter
+	 *            the type of the element on the operand stack or in the local variable table.
+	 * @return a array element for <code>MethodVisitor.visitFrame()</code>'s parameter.
 	 */
 	public static Object getFrameVerificationType(Type type) {
 		switch (type.getSort()) {
@@ -326,6 +321,21 @@ public static Object getFrameVerificationType(Type type) {
 		}
 	}
 
+	/**
+	 * Returns true if the type provided is supported for a JFR event field.
+	 * 
+	 * @param type
+	 *            the type to check.
+	 * @return true if the type provided is supported for a JFR event field.
+	 */
+	public static boolean isSupportedType(Type type) {
+		if (INTERNAL_NAME_THREAD.equals(type.getInternalName()) || INTERNAL_NAME_CLASS.equals(type.getInternalName())
+				|| INTERNAL_NAME_STRING.equals(type.getInternalName())) {
+			return true;
+		}
+		return type.getSort() != Type.OBJECT && type.getSort() != Type.ARRAY;
+	}
+
 	/**
 	 * Type agnostic array toString() which also handles primitive arrays.
 	 */

agent/src/test/java/org/openjdk/jmc/agent/converters/test/DoLittleContainerThreadConverter.java

@@ -0,0 +1,45 @@
+/*
+ * Copyright (c) 2021, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2021, Datadog, Inc. All rights reserved.
+ * 
+ * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
+ *
+ * The contents of this file are subject to the terms of either the Universal Permissive License
+ * v 1.0 as shown at http://oss.oracle.com/licenses/upl
+ *
+ * or the following license:
+ *
+ * Redistribution and use in source and binary forms, with or without modification, are permitted
+ * provided that the following conditions are met:
+ * 
+ * 1. Redistributions of source code must retain the above copyright notice, this list of conditions
+ * and the following disclaimer.
+ * 
+ * 2. Redistributions in binary form must reproduce the above copyright notice, this list of
+ * conditions and the following disclaimer in the documentation and/or other materials provided with
+ * the distribution.
+ * 
+ * 3. Neither the name of the copyright holder nor the names of its contributors may be used to
+ * endorse or promote products derived from this software without specific prior written permission.
+ * 
+ * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS" AND ANY EXPRESS OR
+ * IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND
+ * FITNESS FOR A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT HOLDER OR
+ * CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL
+ * DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
+ * DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY,
+ * WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY
+ * WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+ */
+package org.openjdk.jmc.agent.converters.test;
+
+import org.openjdk.jmc.agent.test.DoLittleContainer;
+
+/**
+ * Converts a {@link DoLittleContainer} to {@link Thread}.
+ */
+public class DoLittleContainerThreadConverter {
+	public static Thread convert(DoLittleContainer container) {
+		return container.getThread();
+	}
+}