Weaver returns null instead of original bytes for unwoven classes

This change makes sense independently of #277, but also enables using

  - cp "my.jar;aspectjweaver.jar"
  -XX:+AllowArchivingWithJavaAgent
  -javaagent:aspectjweaver.jar

while creating a CDS archive. Afterward, the application can be run in
its woven state from the CDS archive even without '-javaagent', because
the byte code was archived in its woven state ("poor man's AJC"). See
#277 (comment)
for details.

Fixes #277.

Signed-off-by: Alexander Kriegisch <[email protected]>

loadtime/src/main/java/org/aspectj/weaver/loadtime/Aj.java

@@ -75,41 +75,41 @@ public void initialize() {
 	private final static String deleLoader2 = "jdk.internal.reflect.DelegatingClassLoader"; // On JDK11+
 
 	@Override
-	public byte[] preProcess(String className, byte[] bytes, ClassLoader loader, ProtectionDomain protectionDomain) {
-		if (loader == null || className == null ||
-			loader.getClass().getName().equals(deleLoader) || loader.getClass().getName().equals(deleLoader2)) {
+	public byte[] preProcess(String className, final byte[] bytes, ClassLoader classLoader, ProtectionDomain protectionDomain) {
+		if (classLoader == null || className == null ||
+			classLoader.getClass().getName().equals(deleLoader) || classLoader.getClass().getName().equals(deleLoader2)) {
 			// skip boot loader, null classes (hibernate), or those from a reflection loader
-			return bytes;
+			return null;
 		}
 
 		if (loadersToSkip != null) {
 			// Check whether to reject it
-			if (loadersToSkip.contains(loader.getClass().getName())) {
+			if (loadersToSkip.contains(classLoader.getClass().getName())) {
 //				System.out.println("debug: no weaver created for loader '"+loader.getClass().getName()+"'");
-				return bytes;
+				return null;
 			}
 		}
 
 		if (trace.isTraceEnabled())
-			trace.enter("preProcess", this, new Object[] { className, bytes, loader });
+			trace.enter("preProcess", this, new Object[] { className, bytes, classLoader });
 		if (trace.isTraceEnabled())
-			trace.event("preProcess", this, new Object[] { loader.getParent(), Thread.currentThread().getContextClassLoader() });
+			trace.event("preProcess", this, new Object[] { classLoader.getParent(), Thread.currentThread().getContextClassLoader() });
 
 		try {
-			synchronized (loader) {
+			synchronized (classLoader) {
 
 				if (SimpleCacheFactory.isEnabled()) {
-					byte[] cacheBytes= laCache.getAndInitialize(className, bytes,loader,protectionDomain);
+					byte[] cacheBytes= laCache.getAndInitialize(className, bytes, classLoader, protectionDomain);
 					if (cacheBytes!=null){
 							return cacheBytes;
 					}
 				}
 
-				WeavingAdaptor weavingAdaptor = WeaverContainer.getWeaver(loader, weavingContext);
+				WeavingAdaptor weavingAdaptor = WeaverContainer.getWeaver(classLoader, weavingContext);
 				if (weavingAdaptor == null) {
 					if (trace.isTraceEnabled())
 						trace.exit("preProcess");
-					return bytes;
+					return null;
 				}
 				try {
 					weavingAdaptor.setActiveProtectionDomain(protectionDomain);
@@ -134,7 +134,7 @@ public byte[] preProcess(String className, byte[] bytes, ClassLoader loader, Pro
 			// would make sense at least in test f.e. see TestHelper.handleMessage()
 			if (trace.isTraceEnabled())
 				trace.exit("preProcess", th);
-			return bytes;
+			return null;
 		} finally {
 			CompilationAndWeavingContext.resetForThread();
 		}

loadtime/src/main/java/org/aspectj/weaver/loadtime/ClassLoaderWeavingAdaptor.java

@@ -569,7 +569,7 @@ private boolean weaveAndDefineConceteAspects() {
 
 			try {
 				byte[] newBytes = weaveClass(name, bytes, true);
-				this.generatedClassHandler.acceptClass(name, bytes, newBytes);
+				this.generatedClassHandler.acceptClass(name, bytes, newBytes == null ? bytes : newBytes);
 			} catch (IOException ex) {
 				trace.error("weaveAndDefineConceteAspects", ex);
 				error("exception weaving aspect '" + name + "'", ex);

loadtime/src/main/java/org/aspectj/weaver/loadtime/ClassPreProcessor.java

@@ -24,7 +24,18 @@ public interface ClassPreProcessor {
 	 */
 	void initialize();
 
-	byte[] preProcess(String className, byte[] bytes, ClassLoader classLoader, ProtectionDomain protectionDomain);
+	/**
+	 * @param className        the name of the class in the internal form of fully qualified class and interface names as
+	 *                         defined in <i>The Java Virtual Machine Specification</i>. For example,
+	 *                         <code>"java/util/List"</code>.
+	 * @param bytes            the input byte buffer in class file format - must not be modified
+	 * @param classLoader      the defining loader of the class to be transformed, may be {@code null} if the bootstrap
+	 *                         loader
+	 * @param protectionDomain the protection domain of the class being defined or redefined
+	 *
+	 * @return a well-formed class file buffer (weaving result), or {@code null} if no weaving was performed
+	 */
+	byte[] preProcess(String className, final byte[] bytes, ClassLoader classLoader, ProtectionDomain protectionDomain);
 
 	void prepareForRedefinition(ClassLoader loader, String className);
 }

loadtime/src/main/java/org/aspectj/weaver/loadtime/ClassPreProcessorAgentAdapter.java

@@ -43,7 +43,7 @@ public class ClassPreProcessorAgentAdapter implements ClassFileTransformer {
 	 */
 	@Override
 	public byte[] transform(ClassLoader loader, String className, Class<?> classBeingRedefined, ProtectionDomain protectionDomain,
-			byte[] bytes) throws IllegalClassFormatException {
+			final byte[] bytes) throws IllegalClassFormatException {
 		if (classBeingRedefined != null) {
 			System.err.println("INFO: (Enh120375):  AspectJ attempting reweave of '" + className + "'");
 			classPreProcessor.prepareForRedefinition(loader, className);

loadtime/src/main/java/org/aspectj/weaver/loadtime/WeavingURLClassLoader.java

@@ -136,6 +136,8 @@ protected Class defineClass(String name, byte[] b, CodeSource cs) throws IOExcep
 
 			try {
 				b = adaptor.weaveClass(name, b, false);
+				if (b == null)
+					b = orig;
 			} catch (AbortException ex) {
 				trace.error("defineClass", ex);
 				throw ex;
@@ -149,7 +151,7 @@ protected Class defineClass(String name, byte[] b, CodeSource cs) throws IOExcep
 		try {
 			clazz= super.defineClass(name, b, cs);
 		} catch (Throwable th) {
-			trace.error("Weaving class problem. Original class has been returned. The error was caused because of: " + th, th);
+			trace.error("Weaving class problem. Original class has been returned. Error cause: " + th, th);
 			clazz= super.defineClass(name, orig, cs);
 		}
 		if (trace.isTraceEnabled()) {

weaver/src/main/java/org/aspectj/weaver/tools/WeavingAdaptor.java

@@ -318,26 +318,30 @@ protected Boolean initialValue() {
 	/**
 	 * Weave a class using aspects previously supplied to the adaptor.
 	 *
-	 * @param name the name of the class
-	 * @param bytes the class bytes
-	 * @param mustWeave if true then this class *must* get woven (used for concrete aspects generated from XML)
-	 * @return the woven bytes
-	 * @exception IOException weave failed
+	 * @param name      the name of the class in the internal form of fully qualified class and interface names as defined
+	 *                  in <i>The Java Virtual Machine Specification</i>. For example, <code>"java/util/List"</code>.
+	 * @param bytes     the input byte buffer in class file format - must not be modified
+	 * @param mustWeave if true then this class <i>must</i> get woven (used for concrete aspects generated from XML)
+	 *
+	 * @return a well-formed class file buffer (the weaving result), or {@code null} if no weaving was performed
+	 *
+	 * @throws IOException weave failed
 	 */
-	public byte[] weaveClass(String name, byte[] bytes, boolean mustWeave) throws IOException {
+	public byte[] weaveClass(String name, final byte[] bytes, boolean mustWeave) throws IOException {
 		if (trace == null) {
 			// Pr231945: we are likely to be under tomcat and ENABLE_CLEAR_REFERENCES hasn't been set
 			System.err
 					.println("AspectJ Weaver cannot continue to weave, static state has been cleared.  Are you under Tomcat? In order to weave '"
 							+ name
 							+ "' during shutdown, 'org.apache.catalina.loader.WebappClassLoader.ENABLE_CLEAR_REFERENCES=false' must be set (see https://bugs.eclipse.org/bugs/show_bug.cgi?id=231945).");
-			return bytes;
+			return null;
 		}
 		if (weaverRunning.get()) {
 			// System.out.println("AJC: avoiding re-entrant call to transform " + name);
-			return bytes;
+			return null;
 		}
 		try {
+			byte[] newBytes = null;
 			weaverRunning.set(true);
 			if (trace.isTraceEnabled()) {
 				trace.enter("weaveClass", this, new Object[] { name, bytes });
@@ -347,7 +351,7 @@ public byte[] weaveClass(String name, byte[] bytes, boolean mustWeave) throws IO
 				if (trace.isTraceEnabled()) {
 					trace.exit("weaveClass", false);
 				}
-				return bytes;
+				return null;
 			}
 
 			boolean debugOn = !messageHandler.isIgnoring(Message.DEBUG);
@@ -360,15 +364,14 @@ public byte[] weaveClass(String name, byte[] bytes, boolean mustWeave) throws IO
 
 						// Determine if we have the weaved class cached
 						CachedClassReference cacheKey = null;
-						final byte[] original_bytes = bytes;
 						if (cache != null && !mustWeave) {
-							cacheKey = cache.createCacheKey(name, original_bytes);
-							CachedClassEntry entry = cache.get(cacheKey, original_bytes);
+							cacheKey = cache.createCacheKey(name, bytes);
+							CachedClassEntry entry = cache.get(cacheKey, bytes);
 							if (entry != null) {
 								// If the entry has been explicitly ignored
 								// return the original bytes
 								if (entry.isIgnored()) {
-									return bytes;
+									return null;
 								}
 								return entry.getBytes();
 							}
@@ -382,7 +385,12 @@ public byte[] weaveClass(String name, byte[] bytes, boolean mustWeave) throws IO
 						if (debugOn) {
 							debug("weaving '" + name + "'");
 						}
-						bytes = getWovenBytes(name, bytes);
+						newBytes = getWovenBytes(name, bytes);
+						// TODO: Is this OK performance-wise?
+						if (Arrays.equals(bytes, newBytes)) {
+							// null means unchanged in java.lang.instrument.ClassFileTransformer::transform
+							newBytes = null;
+						}
 						// temporarily out - searching for @Aspect annotated types is a slow thing to do - we should
 						// expect the user to name them if they want them woven - just like code style
 						// } else if (shouldWeaveAnnotationStyleAspect(name, bytes)) {
@@ -405,10 +413,10 @@ public byte[] weaveClass(String name, byte[] bytes, boolean mustWeave) throws IO
 						if (cacheKey != null) {
 							// If no transform has been applied, mark the class
 							// as ignored.
-							if (Arrays.equals(original_bytes, bytes)) {
-								cache.ignore(cacheKey, original_bytes);
+							if (newBytes == null) {
+								cache.ignore(cacheKey, bytes);
 							} else {
-								cache.put(cacheKey, original_bytes, bytes);
+								cache.put(cacheKey, bytes, newBytes);
 							}
 						}
 					} else if (debugOn) {
@@ -422,9 +430,9 @@ public byte[] weaveClass(String name, byte[] bytes, boolean mustWeave) throws IO
 			}
 
 			if (trace.isTraceEnabled()) {
-				trace.exit("weaveClass", bytes);
+				trace.exit("weaveClass", newBytes);
 			}
-			return bytes;
+			return newBytes;
 		} finally {
 			weaverRunning.remove();
 		}

weaver/src/main/java/org/aspectj/weaver/tools/cache/SimpleCache.java

@@ -72,6 +72,7 @@ public byte[] getAndInitialize(String classname, byte[] bytes,
 		byte[] res = get(classname, bytes);
 
 		if (Arrays.equals(SAME_BYTES, res)) {
+			// TODO: Should we return null (means "not transformed") in this case?
 			return bytes;
 		} else {
 			if (res != null) {