Matthews changes - enabling us to see bytecode before and after LTW (bug 117854)

15 files changed, 203 insertions, 59 deletions

diff --git a/loadtime/src/org/aspectj/weaver/loadtime/Aj.java b/loadtime/src/org/aspectj/weaver/loadtime/Aj.java
index 079b5439a..c70545301 100644
--- a/loadtime/src/org/aspectj/weaver/loadtime/Aj.java
+++ b/loadtime/src/org/aspectj/weaver/loadtime/Aj.java
@@ -11,8 +11,6 @@
  *******************************************************************************/
 package org.aspectj.weaver.loadtime;
 
-import java.io.File;
-import java.io.FileOutputStream;
 import java.lang.reflect.Method;
 import java.lang.reflect.InvocationTargetException;
 import java.util.Map;
@@ -63,11 +61,7 @@ public class Aj implements ClassPreProcessor {
 
         try {
             WeavingAdaptor weavingAdaptor = WeaverContainer.getWeaver(loader, weavingContext);
-            byte[] weaved = weavingAdaptor.weaveClass(className, bytes);
-            if (weavingAdaptor.shouldDump(className.replace('/', '.'))) {
-                dump(className, weaved);
-            }
-            return weaved;
+            return weavingAdaptor.weaveClass(className, bytes);
         } catch (Throwable t) {
             //FIXME AV wondering if we should have the option to fail (throw runtime exception) here
             // would make sense at least in test f.e. see TestHelper.handleMessage()
@@ -165,32 +159,6 @@ public class Aj implements ClassPreProcessor {
     }
 
     /**
-     * Dump the given bytcode in _dump/... (dev mode)
-     *
-     * @param name
-     * @param b
-     * @throws Throwable
-     */
-    static void dump(String name, byte[] b) throws Throwable {
-        String className = name.replace('.', '/');
-        final File dir;
-        if (className.indexOf('/') > 0) {
-            dir = new File("_ajdump" + File.separator + className.substring(0, className.lastIndexOf('/')));
-        } else {
-            dir = new File("_ajdump");
-        }
-        dir.mkdirs();
-        String fileName = "_ajdump" + File.separator + className + ".class";
-        FileOutputStream os = new FileOutputStream(fileName);
-        os.write(b);
-        os.close();
-    }
-    
-    /*
-     * Shared classes methods
-     */
-
-    /**
      * Returns a namespace based on the contest of the aspects available
      */
     public String getNamespace (ClassLoader loader) {
diff --git a/loadtime/src/org/aspectj/weaver/loadtime/ClassLoaderWeavingAdaptor.java b/loadtime/src/org/aspectj/weaver/loadtime/ClassLoaderWeavingAdaptor.java
index 09d11f162..b17392d05 100644
--- a/loadtime/src/org/aspectj/weaver/loadtime/ClassLoaderWeavingAdaptor.java
+++ b/loadtime/src/org/aspectj/weaver/loadtime/ClassLoaderWeavingAdaptor.java
@@ -54,6 +54,7 @@ public class ClassLoaderWeavingAdaptor extends WeavingAdaptor {
     private final static String AOP_XML = "META-INF/aop.xml";
 
     private List m_dumpTypePattern = new ArrayList();
+    private boolean m_dumpBefore = false;
     private List m_includeTypePattern = new ArrayList();
     private List m_excludeTypePattern = new ArrayList();
     private List m_includeStartsWith = new ArrayList();
@@ -81,8 +82,8 @@ public class ClassLoaderWeavingAdaptor extends WeavingAdaptor {
              */
             public void acceptClass(String name, byte[] bytes) {
                 try {
-                    if (shouldDump(name.replace('/', '.'))) {
-                        Aj.dump(name, bytes);
+                    if (shouldDump(name.replace('/', '.'), false)) {
+                        dump(name, bytes, false);
                     }
                 } catch (Throwable throwable) {
                     throwable.printStackTrace();
@@ -438,6 +439,9 @@ public class ClassLoaderWeavingAdaptor extends WeavingAdaptor {
                 TypePattern pattern = new PatternParser(dump).parseTypePattern();
                 m_dumpTypePattern.add(pattern);
             }
+            if (definition.shouldDumpBefore()) {
+            	m_dumpBefore = true;
+            }
         }
     }
 
@@ -543,11 +547,17 @@ public class ClassLoaderWeavingAdaptor extends WeavingAdaptor {
         return accept;
     }
 
-    public boolean shouldDump(String className) {
+    protected boolean shouldDump(String className, boolean before) {
+    	// Don't dump before weaving unless asked to
+    	if (before && !m_dumpBefore) {
+            return false;
+    	}
+    	
         // avoid ResolvedType if not needed
         if (m_dumpTypePattern.isEmpty()) {
             return false;
         }
+
         //TODO AV - optimize for className.startWith only
         ResolvedType classInfo = weaver.getWorld().resolve(UnresolvedType.forName(className), true);
         //dump
diff --git a/loadtime/src/org/aspectj/weaver/loadtime/definition/Definition.java b/loadtime/src/org/aspectj/weaver/loadtime/definition/Definition.java
index efb7d3259..2730428d2 100644
--- a/loadtime/src/org/aspectj/weaver/loadtime/definition/Definition.java
+++ b/loadtime/src/org/aspectj/weaver/loadtime/definition/Definition.java
@@ -24,6 +24,7 @@ public class Definition {
     private StringBuffer m_weaverOptions;
 
     private List m_dumpPatterns;
+    private boolean m_dumpBefore;
 
     private List m_includePatterns;
 
@@ -39,6 +40,7 @@ public class Definition {
 
     public Definition() {
         m_weaverOptions = new StringBuffer();
+        m_dumpBefore = false;
         m_dumpPatterns = new ArrayList(0);
         m_includePatterns = new ArrayList(0);
         m_excludePatterns = new ArrayList(0);
@@ -56,6 +58,14 @@ public class Definition {
         return m_dumpPatterns;
     }
 
+	public void setDumpBefore(boolean b) {
+		m_dumpBefore = b;
+	}
+
+	public boolean shouldDumpBefore() {
+		return m_dumpBefore;
+	}
+
     public List getIncludePatterns() {
         return m_includePatterns;
     }
diff --git a/loadtime/src/org/aspectj/weaver/loadtime/definition/DocumentParser.java b/loadtime/src/org/aspectj/weaver/loadtime/definition/DocumentParser.java
index 931e26d17..2800ebece 100644
--- a/loadtime/src/org/aspectj/weaver/loadtime/definition/DocumentParser.java
+++ b/loadtime/src/org/aspectj/weaver/loadtime/definition/DocumentParser.java
@@ -51,6 +51,7 @@ public class DocumentParser extends DefaultHandler {
     private final static String ASPECTJ_ELEMENT = "aspectj";
     private final static String WEAVER_ELEMENT = "weaver";
     private final static String DUMP_ELEMENT = "dump";
+    private final static String DUMP_BEFOREANDAFTER_ATTRIBUTE = "beforeandafter";
     private final static String INCLUDE_ELEMENT = "include";
     private final static String EXCLUDE_ELEMENT = "exclude";
     private final static String OPTIONS_ATTRIBUTE = "options";
@@ -206,6 +207,10 @@ public class DocumentParser extends DefaultHandler {
             if (!isNull(typePattern)) {
                 m_definition.getDumpPatterns().add(typePattern);
             }
+            String beforeAndAfter = attributes.getValue(DUMP_BEFOREANDAFTER_ATTRIBUTE);
+            if (isTrue(beforeAndAfter)) {
+            	m_definition.setDumpBefore(true);
+            }
         } else if (EXCLUDE_ELEMENT.equals(qName) && m_inAspects) {
             String typePattern = attributes.getValue(WITHIN_ATTRIBUTE);
             if (!isNull(typePattern)) {
@@ -258,5 +263,9 @@ public class DocumentParser extends DefaultHandler {
         return (s == null || s.length() <= 0);
     }
 
+    private boolean isTrue(String s) {
+        return (s != null && s.equals("true"));
+    }
+
 
 }
diff --git a/loadtime/testsrc/org/aspectj/weaver/loadtime/test/DocumentParserTest.java b/loadtime/testsrc/org/aspectj/weaver/loadtime/test/DocumentParserTest.java
index fddbc2794..92d35983f 100644
--- a/loadtime/testsrc/org/aspectj/weaver/loadtime/test/DocumentParserTest.java
+++ b/loadtime/testsrc/org/aspectj/weaver/loadtime/test/DocumentParserTest.java
@@ -39,6 +39,7 @@ public class DocumentParserTest extends TestCase {
         assertEquals("@Baz", def.getAspectExcludePatterns().get(0));
         assertEquals("@Whoo", def.getAspectIncludePatterns().get(0));
         assertEquals("foo..*", def.getDumpPatterns().get(0));
+        assertEquals(true,def.shouldDumpBefore());
     }
 
 }
diff --git a/loadtime/testsrc/org/aspectj/weaver/loadtime/test/simpleWithDtd.xml b/loadtime/testsrc/org/aspectj/weaver/loadtime/test/simpleWithDtd.xml
index f565e6ef1..252bc0136 100644
--- a/loadtime/testsrc/org/aspectj/weaver/loadtime/test/simpleWithDtd.xml
+++ b/loadtime/testsrc/org/aspectj/weaver/loadtime/test/simpleWithDtd.xml
@@ -3,7 +3,7 @@
 <aspectj>
     <weaver options="-showWeaveInfo">
         <include within="foo..bar.Goo+"/>
-        <dump within="foo..*"/>
+        <dump within="foo..*" beforeandafter="true"/>
     </weaver>
     <aspects>
         <exclude within="@Baz"/>
diff --git a/tests/java5/ataspectj/ataspectj/DumpTest.java b/tests/java5/ataspectj/ataspectj/DumpTest.java
index 6039f766a..4fb2b7487 100644
--- a/tests/java5/ataspectj/ataspectj/DumpTest.java
+++ b/tests/java5/ataspectj/ataspectj/DumpTest.java
@@ -21,25 +21,7 @@ import java.io.File;
 public class DumpTest extends TestCase {
 
     public static void main(String[] args) {
-        TestHelper.runAndThrowOnFailure(suite());
-    }
-
-    public static junit.framework.Test suite() {
-        return new junit.framework.TestSuite(DumpTest.class);
-    }
-
-    public void testDump() {
-        File f = new File("_ajdump/ataspectj/DumpTest.class");
-        assertFalse(f.exists());
-
-        DumpTestTheDump forceLoad = new DumpTestTheDump();
-        f = new File("_ajdump/ataspectj/DumpTestTheDump.class");
-        assertTrue(f.exists());
-        
-        // tidy up...
-        f.delete();
-        new File("_ajdump/ataspectj").delete();
-        new File("_ajdump").delete();
+    	new DumpTestTheDump().aroundMethod("DumpTest");
     }
 
 }
diff --git a/tests/java5/ataspectj/ataspectj/DumpTestTheDump.java b/tests/java5/ataspectj/ataspectj/DumpTestTheDump.java
index d2f65ff34..f33ffa85a 100644
--- a/tests/java5/ataspectj/ataspectj/DumpTestTheDump.java
+++ b/tests/java5/ataspectj/ataspectj/DumpTestTheDump.java
@@ -15,4 +15,7 @@ package ataspectj;
  * @author <a href="mailto:alex AT gnilux DOT com">Alexandre Vasseur</a>
  */
 public class DumpTestTheDump {
+	public void aroundMethod (String s) {
+		
+	}
 }
diff --git a/tests/java5/ataspectj/ataspectj/TestAroundAspect.aj b/tests/java5/ataspectj/ataspectj/TestAroundAspect.aj
new file mode 100644
index 000000000..8ae2954c3
--- /dev/null
+++ b/tests/java5/ataspectj/ataspectj/TestAroundAspect.aj
@@ -0,0 +1,18 @@
+/*******************************************************************************
+ * Copyright (c) 2005 Contributors.
+ * All rights reserved. 
+ * This program and the accompanying materials are made available 
+ * under the terms of the Eclipse Public License v1.0 
+ * which accompanies this distribution and is available at 
+ * http://eclipse.org/legal/epl-v10.html 
+ * 
+ * Contributors:
+ *   Matthew Webster         initial implementation
+ *******************************************************************************/
+package ataspectj;
+
+public aspect TestAroundAspect {
+	Object around () : execution(public void aroundMethod(..)) {
+		return proceed();
+	}
+}
diff --git a/tests/java5/ataspectj/ataspectj/aop-dumpbeforeandafter.xml b/tests/java5/ataspectj/ataspectj/aop-dumpbeforeandafter.xml
new file mode 100644
index 000000000..c2824e2be
--- /dev/null
+++ b/tests/java5/ataspectj/ataspectj/aop-dumpbeforeandafter.xml
@@ -0,0 +1,6 @@
+<?xml version="1.0"?>
+<aspectj>
+    <weaver options="-XmessageHandlerClass:ataspectj.TestHelper">
+        <dump within="ataspectj.DumpTestThe*" beforeandafter="true"/> 
+    </weaver>
+</aspectj>
diff --git a/tests/java5/ataspectj/ataspectj/aop-dumpclosure.xml b/tests/java5/ataspectj/ataspectj/aop-dumpclosure.xml
new file mode 100644
index 000000000..d73c1c23b
--- /dev/null
+++ b/tests/java5/ataspectj/ataspectj/aop-dumpclosure.xml
@@ -0,0 +1,9 @@
+<?xml version="1.0"?>
+<aspectj>
+	<aspects>
+		<aspect name="ataspectj.TestAroundAspect"/>
+	</aspects>
+    <weaver options="-Xnoinline">
+        <dump within="ataspectj.DumpTestThe*" beforeandafter="true"/> 
+    </weaver>
+</aspectj>
diff --git a/tests/java5/ataspectj/ataspectj/aop-dumpnone.xml b/tests/java5/ataspectj/ataspectj/aop-dumpnone.xml
new file mode 100644
index 000000000..425f9ad23
--- /dev/null
+++ b/tests/java5/ataspectj/ataspectj/aop-dumpnone.xml
@@ -0,0 +1,5 @@
+<?xml version="1.0"?>
+<aspectj>
+    <weaver options="-XmessageHandlerClass:ataspectj.TestHelper">
+    </weaver>
+</aspectj>
diff --git a/tests/src/org/aspectj/systemtest/ajc150/ataspectj/AtAjLTWTests.java b/tests/src/org/aspectj/systemtest/ajc150/ataspectj/AtAjLTWTests.java
index 27d485d46..5db988b07 100644
--- a/tests/src/org/aspectj/systemtest/ajc150/ataspectj/AtAjLTWTests.java
+++ b/tests/src/org/aspectj/systemtest/ajc150/ataspectj/AtAjLTWTests.java
@@ -12,6 +12,8 @@
 package org.aspectj.systemtest.ajc150.ataspectj;
 
 import org.aspectj.testing.XMLBasedAjcTestCase;
+import org.aspectj.util.FileUtil;
+
 import junit.framework.Test;
 
 import java.io.File;
@@ -65,9 +67,66 @@ public class AtAjLTWTests extends XMLBasedAjcTestCase {
         runTest("AjcLTW AroundInlineMungerTest2");
     }
 
+    public void testLTWDumpNone() {
+        runTest("LTW DumpTest none");
+
+        File f = new File("_ajdump/ataspectj/DumpTest.class");
+        assertFalse(f.exists());
+        f = new File("_ajdump/_before/ataspectj/DumpTestTheDump.class");
+        assertFalse(f.exists());
+        f = new File("_ajdump/ataspectj/DumpTestTheDump.class");
+        assertFalse(f.exists());
+    }
+
     public void testLTWDump() {
         runTest("LTW DumpTest");
-    }
+        
+        File f = new File("_ajdump/ataspectj/DumpTest.class");
+        assertFalse(f.exists());
+        f = new File("_ajdump/_before/ataspectj/DumpTestTheDump.class");
+        assertFalse(f.exists());
+        f = new File("_ajdump/ataspectj/DumpTestTheDump.class");
+        assertTrue(f.exists());
+        
+        // tidy up...
+        f = new File("_ajdump");
+        FileUtil.deleteContents(f);
+        f.delete();
+    }
+
+    public void testLTWDumpBeforeAndAfter() {
+        runTest("LTW DumpTest before and after");
+
+        File f = new File("_ajdump/ataspectj/DumpTest.class");
+        assertFalse(f.exists());
+        f = new File("_ajdump/_before/ataspectj/DumpTestTheDump.class");
+        assertTrue(f.exists());
+        f = new File("_ajdump/ataspectj/DumpTestTheDump.class");
+        assertTrue(f.exists());
+        
+        // tidy up...
+        f = new File("_ajdump");
+        FileUtil.deleteContents(f);
+        f.delete();
+    }
+
+      /* FIXME maw currently can't dump closures because the logic in 
+       * ClassLoaderWeavingAdaptor.shouldDump() relies on the World being
+       * able to resolve the name which it can't for closures.
+       */   
+//    public void testLTWDumpClosure() {
+//        runTest("LTW DumpTest closure");
+//
+//        File f = new File("_ajdump/_before/ataspectj/DumpTestTheDump$AjcClosure1.class");
+//        assertTrue(f.exists());
+//        f = new File("_ajdump/ataspectj/DumpTestTheDump$AjcClosure1.class");
+//        assertTrue(f.exists());
+//        
+//        // tidy up...
+//        f = new File("_ajdump");
+//        FileUtil.deleteContents(f);
+//        f.delete();
+//    }
 
     public void testAjcAspect1LTWAspect2_Xreweavable() {
         runTest("Ajc Aspect1 LTW Aspect2 -Xreweavable");
diff --git a/tests/src/org/aspectj/systemtest/ajc150/ataspectj/ltw.xml b/tests/src/org/aspectj/systemtest/ajc150/ataspectj/ltw.xml
index c3d0045d8..1510598fb 100644
--- a/tests/src/org/aspectj/systemtest/ajc150/ataspectj/ltw.xml
+++ b/tests/src/org/aspectj/systemtest/ajc150/ataspectj/ltw.xml
@@ -79,11 +79,32 @@
         <ant file="ajc-ant.xml" target="ltw.AroundInlineMungerTest2" verbose="true"/>
     </ajc-test>
 
+    <ajc-test dir="java5/ataspectj" title="LTW DumpTest none">
+        <compile
+                files="ataspectj/DumpTest.java,ataspectj/DumpTestTheDump.java,ataspectj/TestHelper.java"
+                options="-1.5"/>
+        <run class="ataspectj.DumpTest" ltw="ataspectj/aop-dumpnone.xml"/>
+    </ajc-test>
+
     <ajc-test dir="java5/ataspectj" title="LTW DumpTest">
         <compile
                 files="ataspectj/DumpTest.java,ataspectj/DumpTestTheDump.java,ataspectj/TestHelper.java"
                 options="-1.5"/>
-        <ant file="ajc-ant.xml" target="ltw.DumpTest" verbose="true"/>
+        <run class="ataspectj.DumpTest" ltw="ataspectj/aop-dump.xml"/>
+    </ajc-test>
+
+    <ajc-test dir="java5/ataspectj" title="LTW DumpTest before and after">
+        <compile
+                files="ataspectj/DumpTest.java,ataspectj/DumpTestTheDump.java,ataspectj/TestHelper.java"
+                options="-1.5"/>
+        <run class="ataspectj.DumpTest" ltw="ataspectj/aop-dumpbeforeandafter.xml"/>
+    </ajc-test>
+
+    <ajc-test dir="java5/ataspectj" title="LTW DumpTest closure">
+        <compile
+                files="ataspectj/DumpTest.java,ataspectj/DumpTestTheDump.java,ataspectj/TestAroundAspect.aj"
+                options="-1.5"/>
+        <run class="ataspectj.DumpTest" ltw="ataspectj/aop-dumpclosure.xml"/>
     </ajc-test>
 
     <ajc-test dir="java5/ataspectj" title="Ajc Aspect1 LTW Aspect2 -Xreweavable">
diff --git a/weaver/src/org/aspectj/weaver/tools/WeavingAdaptor.java b/weaver/src/org/aspectj/weaver/tools/WeavingAdaptor.java
index f70215328..398a0b4f3 100644
--- a/weaver/src/org/aspectj/weaver/tools/WeavingAdaptor.java
+++ b/weaver/src/org/aspectj/weaver/tools/WeavingAdaptor.java
@@ -14,6 +14,7 @@
 package org.aspectj.weaver.tools;
 
 import java.io.File;
+import java.io.FileOutputStream;
 import java.io.IOException;
 import java.io.PrintWriter;
 import java.net.URL;
@@ -187,6 +188,7 @@ public class WeavingAdaptor {
 	 */
 	public byte[] weaveClass (String name, byte[] bytes) throws IOException {
 		if (enabled) {
+
 			if (shouldWeave(name, bytes)) {
 				info("weaving '" + name + "'");
 				bytes = getWovenBytes(name, bytes);
@@ -220,7 +222,7 @@ public class WeavingAdaptor {
         return true;
     }
 
-    public boolean shouldDump(String name) {
+    protected boolean shouldDump(String name, boolean before) {
         return false;
     }
 
@@ -338,6 +340,37 @@ public class WeavingAdaptor {
 	}
 
 	/**
+	 * Dump the given bytcode in _dump/... (dev mode)
+	 *
+	 * @param name
+	 * @param b
+	 * @param before whether we are dumping before weaving
+	 * @throws Throwable
+	 */
+	protected void dump(String name, byte[] b, boolean before) {
+		String dirName = "_ajdump";
+		if (before) dirName = dirName + File.separator + "_before";
+		
+	    String className = name.replace('.', '/');
+	    final File dir;
+	    if (className.indexOf('/') > 0) {
+	        dir = new File(dirName + File.separator + className.substring(0, className.lastIndexOf('/')));
+	    } else {
+	        dir = new File(dirName);
+	    }
+	    dir.mkdirs();
+	    String fileName = dirName + File.separator + className + ".class";
+	    try {
+		    FileOutputStream os = new FileOutputStream(fileName);
+		    os.write(b);
+		    os.close();
+	    }
+	    catch (IOException ex) {
+	    	warn("unable to dump class " + name + " in directory " + dirName,ex);
+	    }
+	}
+
+	/**
 	 * Processes messages arising from weaver operations. 
 	 * Tell weaver to abort on any message more severe than warning.
 	 */
@@ -396,6 +429,11 @@ public class WeavingAdaptor {
         public WeavingClassFileProvider (String name, byte[] bytes) {
 			this.unwovenClass = new UnwovenClassFile(name,bytes);
 			this.unwovenClasses.add(unwovenClass);
+			
+			if (shouldDump(name.replace('/', '.'),true)) {
+				dump(name, bytes, true);
+			}
+
 			bcelWorld.addSourceObjectType(unwovenClass.getJavaClass());
 		}
 
@@ -422,6 +460,11 @@ public class WeavingAdaptor {
 				public void acceptResult(UnwovenClassFile result) {
 					if (wovenClass == null) {
 						wovenClass = result;
+						
+						String name = result.getClassName();
+						if (shouldDump(name.replace('/', '.'), false)) {
+							dump(name, result.getBytes(), false);
+						}
 					}
 					
 					/* Classes generated by weaver e.g. around closure advice */