From f01c8349cc403e12bb2a0f2f355b8cd7784c8afc Mon Sep 17 00:00:00 2001 From: Ark Date: Mon, 13 Aug 2018 18:45:03 -0700 Subject: [PATCH] Deep-merge Reference fields and methods Plus misc cleanup. --- .../trace/agent/tooling/Instrumenter.java | 2 +- .../trace/agent/tooling/muzzle/Reference.java | 65 ++++++++++++++----- .../tooling/muzzle/ReferenceCreator.java | 40 ++++++++---- 3 files changed, 78 insertions(+), 29 deletions(-) diff --git a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/Instrumenter.java b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/Instrumenter.java index 542d76444b..684efe5781 100644 --- a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/Instrumenter.java +++ b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/Instrumenter.java @@ -132,7 +132,7 @@ public interface Instrumenter { log.debug( "Instrumentation muzzled: {} -- {} on {}", instrumentationPrimaryName, - this.getClass().getName(), + getClass().getName(), classLoader); for (Reference.Mismatch mismatch : mismatches) { log.debug("-- {}", mismatch); diff --git a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/muzzle/Reference.java b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/muzzle/Reference.java index 3f253ac6d7..2907ae85ec 100644 --- a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/muzzle/Reference.java +++ b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/muzzle/Reference.java @@ -84,8 +84,8 @@ public class Reference { className, superName, merge(interfaces, anotherReference.interfaces), - merge(fields, anotherReference.fields), - merge(methods, anotherReference.methods)); + mergeFields(fields, anotherReference.fields), + mergeMethods(methods, anotherReference.methods)); } private static Set merge(Set set1, Set set2) { @@ -95,6 +95,32 @@ public class Reference { return set; } + private static Set mergeMethods(Set methods1, Set methods2) { + List merged = new ArrayList<>(methods1); + for (Method method : methods2) { + int i = merged.indexOf(method); + if (i == -1) { + merged.add(method); + } else { + merged.set(i, merged.get(i).merge(method)); + } + } + return new HashSet<>(merged); + } + + private static Set mergeFields(Set fields1, Set fields2) { + List merged = new ArrayList<>(fields1); + for (Field field : fields2) { + int i = merged.indexOf(field); + if (i == -1) { + merged.add(field); + } else { + merged.set(i, merged.get(i).merge(field)); + } + } + return new HashSet<>(merged); + } + private static Set mergeFlags(Set flags1, Set flags2) { Set merged = merge(flags1, flags2); // TODO: Assert flags are non-contradictory and resolve @@ -144,8 +170,15 @@ public class Reference { } } + /** + * A mismatch between a Reference and a runtime class. + * + *

This class' toString returns a human-readable description of the mismatch along with + * source-code locations of the instrumentation which caused the mismatch. + */ public abstract static class Mismatch { - final Source[] mismatchSources; + /** Instrumentation sources which caused the mismatch. */ + private final Source[] mismatchSources; Mismatch(Source[] mismatchSources) { this.mismatchSources = mismatchSources; @@ -160,10 +193,11 @@ public class Reference { } } + /** Human-readable string describing the mismatch. */ abstract String getMismatchDetails(); public static class MissingClass extends Mismatch { - final String className; + private final String className; public MissingClass(Source[] sources, String className) { super(sources); @@ -177,9 +211,9 @@ public class Reference { } public static class MissingFlag extends Mismatch { - final Flag expectedFlag; - final String classMethodOrFieldDesc; - final int foundAccess; + private final Flag expectedFlag; + private final String classMethodOrFieldDesc; + private final int foundAccess; public MissingFlag( Source[] sources, String classMethodOrFieldDesc, Flag expectedFlag, int foundAccess) { @@ -197,14 +231,14 @@ public class Reference { /** Fallback mismatch in case an unexpected exception occurs during reference checking. */ public static class ReferenceCheckError extends Mismatch { - private final Exception referenceCheckExcetpion; + private final Exception referenceCheckException; private final Reference referenceBeingChecked; private final ClassLoader classLoaderBeingChecked; public ReferenceCheckError( Exception e, Reference referenceBeingChecked, ClassLoader classLoaderBeingChecked) { super(new Source[0]); - this.referenceCheckExcetpion = e; + this.referenceCheckException = e; this.referenceBeingChecked = referenceBeingChecked; this.classLoaderBeingChecked = classLoaderBeingChecked; } @@ -219,7 +253,7 @@ public class Reference { sw.write("\n"); // add exception message and stack trace final PrintWriter pw = new PrintWriter(sw); - referenceCheckExcetpion.printStackTrace(pw); + referenceCheckException.printStackTrace(pw); return sw.toString(); } } @@ -243,8 +277,8 @@ public class Reference { } public static class MissingMethod extends Mismatch { - final String className; - final String method; + private final String className; + private final String method; public MissingMethod(Source[] sources, String className, String method) { super(sources); @@ -299,7 +333,7 @@ public class Reference { @Override public boolean matches(int asmFlags) { - return (Opcodes.ACC_PUBLIC & asmFlags) != 0 || (Opcodes.ACC_PROTECTED & asmFlags) != 0; + return PUBLIC.matches(asmFlags) || (Opcodes.ACC_PROTECTED & asmFlags) != 0; } }, PRIVATE_OR_HIGHER { @@ -464,8 +498,6 @@ public class Reference { @Override public String toString() { - // ()V - // toString()Ljava/lang/String; return name + getDescriptor(); } @@ -476,7 +508,8 @@ public class Reference { @Override public boolean equals(Object o) { if (o instanceof Method) { - return toString().equals(o.toString()); + final Method m = (Method) o; + return name.equals(m.name) && getDescriptor().equals(m.getDescriptor()); } return false; } diff --git a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/muzzle/ReferenceCreator.java b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/muzzle/ReferenceCreator.java index f0d0172b23..ba7f42013d 100644 --- a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/muzzle/ReferenceCreator.java +++ b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/muzzle/ReferenceCreator.java @@ -26,6 +26,14 @@ import net.bytebuddy.jar.asm.Type; // - inner class // - cast opcodes in method bodies public class ReferenceCreator extends ClassVisitor { + /** + * Classes in this namespace will be scanned and used to create references. + * + *

For now we're hardcoding this to the instrumentation package so we only create references + * from the method advice and helper classes. + */ + private static String REFERENCE_CREATION_PACKAGE = "datadog.trace.instrumentation."; + public static Map createReferencesFrom( String entryPointClassName, ClassLoader loader) { return ReferenceCreator.createReferencesFrom(entryPointClassName, loader, true); @@ -63,7 +71,7 @@ public class ReferenceCreator extends ClassVisitor { for (Map.Entry entry : instrumentationReferences.entrySet()) { // Don't generate references created outside of the datadog instrumentation package. if (!visitedSources.contains(entry.getKey()) - && entry.getKey().startsWith("datadog.trace.instrumentation.")) { + && entry.getKey().startsWith(REFERENCE_CREATION_PACKAGE)) { instrumentationQueue.add(entry.getKey()); } if (references.containsKey(entry.getKey())) { @@ -86,6 +94,11 @@ public class ReferenceCreator extends ClassVisitor { return references; } + /** + * Get the package of an internal class name. + * + *

foo/bar/Baz -> foo/bar/ + */ private static String internalPackageName(String internalName) { return internalName.replaceAll("/[^/]+$", ""); } @@ -139,6 +152,17 @@ public class ReferenceCreator extends ClassVisitor { } } + /** + * @return If TYPE is an array, return the underlying type. If TYPE is not an array simply return + * the type. + */ + private static Type underlyingType(Type type) { + while (type.getSort() == Type.ARRAY) { + type = type.getElementType(); + } + return type; + } + private Map references = new HashMap<>(); private String refSourceClassName; private Type refSourceType; @@ -249,10 +273,7 @@ public class ReferenceCreator extends ClassVisitor { fieldType) .build()); - Type underlyingFieldType = fieldType; - while (underlyingFieldType.getSort() == Type.ARRAY) { - underlyingFieldType = underlyingFieldType.getElementType(); - } + final Type underlyingFieldType = underlyingType(fieldType); if (underlyingFieldType.getSort() == Type.OBJECT) { addReference( new Reference.Builder(underlyingFieldType.getInternalName()) @@ -282,10 +303,7 @@ public class ReferenceCreator extends ClassVisitor { final Type methodType = Type.getMethodType(descriptor); { // ref for method return type - Type returnType = methodType.getReturnType(); - while (returnType.getSort() == Type.ARRAY) { - returnType = returnType.getElementType(); - } + final Type returnType = underlyingType(methodType.getReturnType()); if (returnType.getSort() == Type.OBJECT) { addReference( new Reference.Builder(returnType.getInternalName()) @@ -296,9 +314,7 @@ public class ReferenceCreator extends ClassVisitor { } // refs for method param types for (Type paramType : methodType.getArgumentTypes()) { - while (paramType.getSort() == Type.ARRAY) { - paramType = paramType.getElementType(); - } + paramType = underlyingType(paramType); if (paramType.getSort() == Type.OBJECT) { addReference( new Reference.Builder(paramType.getInternalName())