From 8755f774ae286eefbe3826dab592f1a22d0f6b95 Mon Sep 17 00:00:00 2001 From: Andy Clement Date: Mon, 21 Jan 2019 10:20:04 -0800 Subject: [PATCH] 389678: Better support for overweaving More testcases for overweaving and better handling of WeaverStateInfo to avoid the dreaded problems deserialized the 'special key' used to store diffs. With these changes once a class is woven via overweaving we switch the diff we store in the weaverstateinfo to 0 byte array (indicating overweaving happened for later weavers that see it). We also stop writing the special 'key' into the attribute and avoid looking-for and attempting to replace it at the end of weaving. --- .../org/aspectj/weaver/WeaverMessages.java | 11 +- .../org/aspectj/weaver/WeaverStateInfo.java | 33 ++++- .../aspectj/weaver/weaver-messages.properties | 1 + .../systemtest/ajc193/Ajc193Tests.java | 68 +++++++++- .../org/aspectj/systemtest/ajc193/ajc193.xml | 118 ++++++++++++++++++ .../aspectj/weaver/bcel/BcelClassWeaver.java | 8 +- .../org/aspectj/weaver/bcel/BcelWeaver.java | 35 +++--- .../org/aspectj/weaver/bcel/LazyClassGen.java | 17 ++- 8 files changed, 259 insertions(+), 32 deletions(-) diff --git a/org.aspectj.matcher/src/org/aspectj/weaver/WeaverMessages.java b/org.aspectj.matcher/src/org/aspectj/weaver/WeaverMessages.java index 736220cd2..5a64a77e8 100644 --- a/org.aspectj.matcher/src/org/aspectj/weaver/WeaverMessages.java +++ b/org.aspectj.matcher/src/org/aspectj/weaver/WeaverMessages.java @@ -1,18 +1,19 @@ /******************************************************************************* - * Copyright (c) 2004 IBM Corporation and others. + * Copyright (c) 2004-2019 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://www.eclipse.org/legal/epl-v10.html - * - * Contributors: - * IBM Corporation - initial API and implementation *******************************************************************************/ package org.aspectj.weaver; import java.text.MessageFormat; import java.util.ResourceBundle; +/** + * @author Andy Clement + * @author IBM + */ public class WeaverMessages { private static ResourceBundle bundle = ResourceBundle.getBundle("org.aspectj.weaver.weaver-messages"); @@ -183,6 +184,8 @@ public class WeaverMessages { public static final String HAS_MEMBER_NOT_ENABLED = "hasMemberNotEnabled"; + public static final String MUST_KEEP_OVERWEAVING_ONCE_START = "mustKeepOverweavingOnceStart"; + // @AspectJ public static final String RETURNING_FORMAL_NOT_DECLARED_IN_ADVICE = "returningFormalNotDeclaredInAdvice"; public static final String THROWN_FORMAL_NOT_DECLARED_IN_ADVICE = "thrownFormalNotDeclaredInAdvice"; diff --git a/org.aspectj.matcher/src/org/aspectj/weaver/WeaverStateInfo.java b/org.aspectj.matcher/src/org/aspectj/weaver/WeaverStateInfo.java index b9fc99da8..f0dcbf270 100644 --- a/org.aspectj.matcher/src/org/aspectj/weaver/WeaverStateInfo.java +++ b/org.aspectj.matcher/src/org/aspectj/weaver/WeaverStateInfo.java @@ -1,5 +1,5 @@ /* ******************************************************************* - * Copyright (c) 2002 Palo Alto Research Center, Incorporated (PARC). + * Copyright (c) 2002-2019 Palo Alto Research Center, Incorporated (PARC). * All rights reserved. * This program and the accompanying materials are made available * under the terms of the Eclipse Public License v1.0 @@ -38,6 +38,8 @@ import org.aspectj.weaver.AjAttribute.WeaverVersionInfo; * themselves If we are reweavable then we also have: Short: Number of aspects that touched this type in some way when it was * previously woven The fully qualified name of each type Int: Length of class file data (i.e. the unwovenclassfile) * Byte[]: The class file data, compressed if REWEAVABLE_COMPRESSION_BIT set. + * + * @author Andy Clement */ public class WeaverStateInfo { private List typeMungers; @@ -230,6 +232,21 @@ public class WeaverStateInfo { } } + private final static byte[] NO_BYTES = new byte[0]; + + /** + * If the weaver is ever invoked in over weaving mode, we should + * not include the key when writing out, it won't be replaced later. + * If we turn off the reweaving flag that unfortunately removes + * the 'what aspects have been woven into this type' list which we + * want to keep as it helps overweaving avoid weaving an aspect in + * twice. + */ + public void markOverweavingInUse() { + reweavableDiffMode = false; + unwovenClassFile = NO_BYTES; + } + public void addConcreteMunger(ConcreteTypeMunger munger) { typeMungers.add(new Entry(munger.getAspectType(), munger.getMunger())); } @@ -258,6 +275,10 @@ public class WeaverStateInfo { return oldStyle; } + public byte[] getUnwovenClassFileData() { + return unwovenClassFile; + } + public byte[] getUnwovenClassFileData(byte wovenClassFile[]) { if (unwovenClassFileIsADiff) { unwovenClassFile = applyDiff(wovenClassFile, unwovenClassFile); @@ -334,10 +355,12 @@ public class WeaverStateInfo { } } else { classData = new byte[unwovenClassFileSize]; - int bytesread = s.read(classData); - if (bytesread != unwovenClassFileSize) { - throw new IOException("ERROR whilst reading reweavable data, expected " + unwovenClassFileSize - + " bytes, only found " + bytesread); + if (unwovenClassFileSize != 0) { + int bytesread = s.read(classData); + if (bytesread != unwovenClassFileSize) { + throw new IOException("ERROR whilst reading reweavable data, expected " + unwovenClassFileSize + + " bytes, only found " + bytesread); + } } } diff --git a/org.aspectj.matcher/src/org/aspectj/weaver/weaver-messages.properties b/org.aspectj.matcher/src/org/aspectj/weaver/weaver-messages.properties index f47b3e38b..2745ae9e7 100644 --- a/org.aspectj.matcher/src/org/aspectj/weaver/weaver-messages.properties +++ b/org.aspectj.matcher/src/org/aspectj/weaver/weaver-messages.properties @@ -181,6 +181,7 @@ noParameterizedDeclaringTypesInCall=can't use parameterized type patterns for th noRawTypePointcutReferences=cannot use a raw type reference to refer to a pointcut in a generic type (use a parameterized reference instead) hasMemberNotEnabled=the type pattern {0} can only be used when the -XhasMember option is set +mustKeepOverweavingOnceStart=the type {0} was previously subject to overweaving and after that can only be woven again in overweaving mode # Java5 features used in pre-Java 5 environment atannotationNeedsJava5=the @annotation pointcut expression is only supported at Java 5 compliance level or above diff --git a/tests/src/org/aspectj/systemtest/ajc193/Ajc193Tests.java b/tests/src/org/aspectj/systemtest/ajc193/Ajc193Tests.java index 255d7d871..801dd12f6 100644 --- a/tests/src/org/aspectj/systemtest/ajc193/Ajc193Tests.java +++ b/tests/src/org/aspectj/systemtest/ajc193/Ajc193Tests.java @@ -9,8 +9,10 @@ package org.aspectj.systemtest.ajc193; import java.io.File; +import org.aspectj.apache.bcel.classfile.JavaClass; import org.aspectj.testing.XMLBasedAjcTestCase; import org.aspectj.testing.XMLBasedAjcTestCaseForJava10OrLater; +import org.aspectj.weaver.WeaverStateInfo; import junit.framework.Test; @@ -19,8 +21,72 @@ import junit.framework.Test; */ public class Ajc193Tests extends XMLBasedAjcTestCaseForJava10OrLater { + public void testComplexOverweaving1() { + // This is the same code as the other test but overweaving OFF + runTest("overweaving"); + } + + public void testComplexOverweaving2() throws Exception { + // This is the same code as the other test but overweaving ON + runTest("overweaving 2"); + // Asserting the weaver state info in the tests that will drive overweaving behaviour: + + // After step 1 of the test, MyAspect will have been applied. + JavaClass jc = getClassFrom(new File(ajc.getSandboxDirectory(),"ow1.jar"), "Application"); + WeaverStateInfo wsi = getWeaverStateInfo(jc); + assertEquals("[LMyAspect;]", wsi.getAspectsAffectingType().toString()); + assertTrue(wsi.getUnwovenClassFileData().length>0); + + // After overweaving, MyAspect2 should also be getting applied but the unwovenclassfile + // data has been blanked out - because we can no longer use it, only overweaving is possible + // once one overweaving step is done + jc = getClassFrom(ajc.getSandboxDirectory(), "Application"); + wsi = getWeaverStateInfo(jc); + assertEquals("[LMyAspect2;, LMyAspect;]", wsi.getAspectsAffectingType().toString()); + assertEquals(0,wsi.getUnwovenClassFileData().length); + } + + // Two steps of overweaving + public void testComplexOverweaving3() throws Exception { + // This is the same code as the other test but overweaving ON + runTest("overweaving 3"); + // Asserting the weaver state info in the tests that will drive overweaving behaviour: + + // After step 1 of the test, MyAspect will have been applied. + JavaClass jc = getClassFrom(new File(ajc.getSandboxDirectory(),"ow1.jar"), "Application"); + WeaverStateInfo wsi = getWeaverStateInfo(jc); + assertEquals("[LMyAspect;]", wsi.getAspectsAffectingType().toString()); + assertTrue(wsi.getUnwovenClassFileData().length>0); + + // After overweaving, MyAspect2 should also be getting applied but the unwovenclassfile + // data has been blanked out - because we can no longer use it, only overweaving is possible + // once one overweaving step is done + jc = getClassFrom(new File(ajc.getSandboxDirectory(),"ow3.jar"), "Application"); + wsi = getWeaverStateInfo(jc); + assertEquals("[LMyAspect2;, LMyAspect;]", wsi.getAspectsAffectingType().toString()); + assertEquals(0,wsi.getUnwovenClassFileData().length); + + jc = getClassFrom(ajc.getSandboxDirectory(), "Application"); + wsi = getWeaverStateInfo(jc); + assertEquals("[LMyAspect3;, LMyAspect2;, LMyAspect;]", wsi.getAspectsAffectingType().toString()); + assertEquals(0,wsi.getUnwovenClassFileData().length); + } + + // overweaving then attempt non overweaving - should fail + public void testComplexOverweaving4() throws Exception { + // This is the same code as the other test but overweaving ON + runTest("overweaving 4"); + // Asserting the weaver state info in the tests that will drive overweaving behaviour: + + // After step 1 of the test, MyAspect will have been applied. + JavaClass jc = getClassFrom(new File(ajc.getSandboxDirectory(),"ow1.jar"), "Application"); + WeaverStateInfo wsi = getWeaverStateInfo(jc); + assertEquals("[LMyAspect;]", wsi.getAspectsAffectingType().toString()); + assertTrue(wsi.getUnwovenClassFileData().length>0); + } + // Altered version of this test from org.aspectj.systemtest.ajc150.Enums for 542682 - public void decpOnEnumNotAllowed_xlints() { + public void testDecpOnEnumNotAllowed_xlints() { runTest("wildcard enum match in itd"); } diff --git a/tests/src/org/aspectj/systemtest/ajc193/ajc193.xml b/tests/src/org/aspectj/systemtest/ajc193/ajc193.xml index d5ed0f1c1..f391c50d7 100644 --- a/tests/src/org/aspectj/systemtest/ajc193/ajc193.xml +++ b/tests/src/org/aspectj/systemtest/ajc193/ajc193.xml @@ -2,6 +2,124 @@ + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + diff --git a/weaver/src/org/aspectj/weaver/bcel/BcelClassWeaver.java b/weaver/src/org/aspectj/weaver/bcel/BcelClassWeaver.java index b4eaa3dd4..83919ed50 100644 --- a/weaver/src/org/aspectj/weaver/bcel/BcelClassWeaver.java +++ b/weaver/src/org/aspectj/weaver/bcel/BcelClassWeaver.java @@ -538,8 +538,12 @@ class BcelClassWeaver implements IClassWeaver { if (inReweavableMode) { WeaverStateInfo wsi = clazz.getOrCreateWeaverStateInfo(true); wsi.addAspectsAffectingType(aspectsAffectingType); - wsi.setUnwovenClassFileData(ty.getJavaClass().getBytes()); - wsi.setReweavable(true); + if (!world.isOverWeaving()) { + wsi.setUnwovenClassFileData(ty.getJavaClass().getBytes()); + wsi.setReweavable(true); + } else { + wsi.markOverweavingInUse(); + } } else { clazz.getOrCreateWeaverStateInfo(false).setReweavable(false); } diff --git a/weaver/src/org/aspectj/weaver/bcel/BcelWeaver.java b/weaver/src/org/aspectj/weaver/bcel/BcelWeaver.java index 4306602e7..f83a79379 100644 --- a/weaver/src/org/aspectj/weaver/bcel/BcelWeaver.java +++ b/weaver/src/org/aspectj/weaver/bcel/BcelWeaver.java @@ -1364,15 +1364,12 @@ public class BcelWeaver { if (!alreadyConfirmedReweavableState.contains(requiredTypeSignature)) { ResolvedType rtx = world.resolve(UnresolvedType.forSignature(requiredTypeSignature), true); boolean exists = !rtx.isMissing(); - if (!exists) { - world.getLint().missingAspectForReweaving.signal(new String[] { rtx.getName(), className }, + if (!world.isOverWeaving()) { + if (!exists) { + world.getLint().missingAspectForReweaving.signal(new String[] { rtx.getName(), className }, classType.getSourceLocation(), null); - // world.showMessage(IMessage.ERROR, WeaverMessages.format(WeaverMessages.MISSING_REWEAVABLE_TYPE, - // requiredTypeName, className), classType.getSourceLocation(), null); - } else { - if (world.isOverWeaving()) { - // System.out.println(">> Removing " + requiredTypeName + " from weaving process: " - // + xcutSet.deleteAspect(rtx)); + // world.showMessage(IMessage.ERROR, WeaverMessages.format(WeaverMessages.MISSING_REWEAVABLE_TYPE, + // requiredTypeName, className), classType.getSourceLocation(), null); } else { // weaved in aspect that are not declared in aop.xml // trigger an error for now @@ -1386,8 +1383,8 @@ public class BcelWeaver { world.showMessage(IMessage.INFO, WeaverMessages.format(WeaverMessages.VERIFIED_REWEAVABLE_TYPE, rtx.getName(), rtx.getSourceLocation().getSourceFile()), null, null); } + alreadyConfirmedReweavableState.add(requiredTypeSignature); } - alreadyConfirmedReweavableState.add(requiredTypeSignature); } } } @@ -1396,11 +1393,21 @@ public class BcelWeaver { // ().getFileName(), wsi.getUnwovenClassFileData())); // new: reweavable default with clever diff if (!world.isOverWeaving()) { - byte[] bytes = wsi.getUnwovenClassFileData(classType.getJavaClass().getBytes()); - WeaverVersionInfo wvi = classType.getWeaverVersionAttribute(); - JavaClass newJavaClass = Utility.makeJavaClass(classType.getJavaClass().getFileName(), bytes); - classType.setJavaClass(newJavaClass, true); - classType.getResolvedTypeX().ensureConsistent(); + byte[] ucfd = wsi.getUnwovenClassFileData(); + if (ucfd.length == 0) { + // Size 0 indicates the class was previously overwoven, so you need to be overweaving now! + world.getMessageHandler().handleMessage( + MessageUtil.error( + WeaverMessages.format(WeaverMessages.MUST_KEEP_OVERWEAVING_ONCE_START, + className))); +// onType.getName(), annoX.getTypeName(), annoX.getValidTargets()), +// decA.getSourceLocation())); + } else { + byte[] bytes = wsi.getUnwovenClassFileData(classType.getJavaClass().getBytes()); + JavaClass newJavaClass = Utility.makeJavaClass(classType.getJavaClass().getFileName(), bytes); + classType.setJavaClass(newJavaClass, true); + classType.getResolvedTypeX().ensureConsistent(); + } } // } else { // classType.resetState(); diff --git a/weaver/src/org/aspectj/weaver/bcel/LazyClassGen.java b/weaver/src/org/aspectj/weaver/bcel/LazyClassGen.java index 95ef524b5..83189e1d4 100644 --- a/weaver/src/org/aspectj/weaver/bcel/LazyClassGen.java +++ b/weaver/src/org/aspectj/weaver/bcel/LazyClassGen.java @@ -533,9 +533,14 @@ public final class LazyClassGen { myGen.addAttribute(Utility.bcelAttribute(new AjAttribute.WeaverVersionInfo(), getConstantPool())); } - // 352389: don't add another one (there will already be one there and this new one won't deserialize correctly) - if (!world.isOverWeaving() || !myGen.hasAttribute(WeaverState.AttributeName)) { - if (myType != null && myType.getWeaverState() != null) { + // see 389678: TODO more finessing possible here? + if (world.isOverWeaving()) { + if (myGen.hasAttribute(WeaverState.AttributeName) && myType!=null && myType.getWeaverState() != null) { + myGen.removeAttribute(myGen.getAttribute(WeaverState.AttributeName)); + myGen.addAttribute(Utility.bcelAttribute(new AjAttribute.WeaverState(myType.getWeaverState()), getConstantPool())); + } + } else { + if (!myGen.hasAttribute(WeaverState.AttributeName) && myType != null && myType.getWeaverState() != null) { myGen.addAttribute(Utility.bcelAttribute(new AjAttribute.WeaverState(myType.getWeaverState()), getConstantPool())); } } @@ -750,8 +755,8 @@ public final class LazyClassGen { public byte[] getJavaClassBytesIncludingReweavable(BcelWorld world) { writeBack(world); byte[] wovenClassFileData = myGen.getJavaClass().getBytes(); - // At 1.6 stackmaps are optional - // At 1.7 or later stackmaps are required (if not turning off the verifier) + // At 1.6 stackmaps are optional, whilst at 1.7 and later they + // are required (unless turning off the verifier) if ((myGen.getMajor() == Constants.MAJOR_1_6 && world.shouldGenerateStackMaps()) || myGen.getMajor() > Constants.MAJOR_1_6) { if (!AsmDetector.isAsmAround) { throw new BCException("Unable to find Asm for stackmap generation (Looking for 'aj.org.objectweb.asm.ClassReader'). Stackmap generation for woven code is required to avoid verify errors on a Java 1.7 or higher runtime"); @@ -760,7 +765,7 @@ public final class LazyClassGen { } WeaverStateInfo wsi = myType.getWeaverState();// getOrCreateWeaverStateInfo(); - if (wsi != null && wsi.isReweavable()) { // && !reweavableDataInserted + if (wsi != null && wsi.isReweavable() && !world.isOverWeaving()) { // && !reweavableDataInserted // reweavableDataInserted = true; return wsi.replaceKeyWithDiff(wovenClassFileData); } else { -- 2.39.5