fix for Bugzilla Bug 51929

   Advice calling protected super method causing java.lang.VerifyError 'Bad access to protected data'
Also expanded test to cover protected field access as well as methods

Fix required getting the correct receiver type for both field access and method
calls to correspond to Java's complicated rules for accessing protected
members (JLSv2 6.6.2 and mentioned in passing in JVMv2 5.4.4)

diff --git a/org.aspectj.ajdt.core/src/org/aspectj/ajdt/internal/compiler/ast/AccessForInlineVisitor.java b/org.aspectj.ajdt.core/src/org/aspectj/ajdt/internal/compiler/ast/AccessForInlineVisitor.java
index d4af2905cd7850fb46e7f0aeba55d79e6f125044..c913040fe909e51a557010fd55f37328c7dc304a 100644 (file)
--- a/org.aspectj.ajdt.core/src/org/aspectj/ajdt/internal/compiler/ast/AccessForInlineVisitor.java
+++ b/org.aspectj.ajdt.core/src/org/aspectj/ajdt/internal/compiler/ast/AccessForInlineVisitor.java
@@ -35,12 +35,14 @@ import org.eclipse.jdt.internal.compiler.ast.QualifiedTypeReference;
 import org.eclipse.jdt.internal.compiler.ast.SingleNameReference;
 import org.eclipse.jdt.internal.compiler.ast.SingleTypeReference;
 import org.eclipse.jdt.internal.compiler.ast.ThisReference;
+import org.eclipse.jdt.internal.compiler.ast.TypeDeclaration;
 import org.eclipse.jdt.internal.compiler.lookup.ArrayBinding;
 import org.eclipse.jdt.internal.compiler.lookup.BlockScope;
 import org.eclipse.jdt.internal.compiler.lookup.FieldBinding;
 import org.eclipse.jdt.internal.compiler.lookup.MethodBinding;
 import org.eclipse.jdt.internal.compiler.lookup.ReferenceBinding;
 import org.eclipse.jdt.internal.compiler.lookup.TypeBinding;
+import org.eclipse.jdt.internal.compiler.lookup.VariableBinding;
 
 /**
  * Walks the body of around advice
@@ -75,29 +77,35 @@ public class AccessForInlineVisitor extends ASTVisitor {
        
        public void endVisit(SingleNameReference ref, BlockScope scope) {
                if (ref.binding instanceof FieldBinding) {
-                       ref.binding = getAccessibleField((FieldBinding)ref.binding);
+                       ref.binding = getAccessibleField((FieldBinding)ref.binding, ref.actualReceiverType);
                }
        }
 
        public void endVisit(QualifiedNameReference ref, BlockScope scope) {
-               //System.err.println("qref: " + ref + ", " + ref.binding.getClass().getName() + ", " + ref.codegenBinding.getClass().getName());
-               //System.err.println("   others: " + Arrays.asList(ref.otherBindings));
                if (ref.binding instanceof FieldBinding) {
-                       ref.binding = getAccessibleField((FieldBinding)ref.binding);
+                       ref.binding = getAccessibleField((FieldBinding)ref.binding, ref.actualReceiverType);
                }
-               if (ref.otherBindings != null) {
+               if (ref.otherBindings != null && ref.otherBindings.length > 0) {
+                       TypeBinding receiverType;
+                       if (ref.binding instanceof FieldBinding) {
+                               receiverType = ((FieldBinding)ref.binding).type;
+                       } else if (ref.binding instanceof VariableBinding) {
+                               receiverType = ((VariableBinding)ref.binding).type;
+                       } else {
+                               //!!! understand and fix this case later
+                               receiverType = ref.otherBindings[0].declaringClass;
+                       }
+                       
                        for (int i=0, len=ref.otherBindings.length; i < len; i++) {
-                               if (ref.otherBindings[i] instanceof FieldBinding) {
-                                       ref.otherBindings[i] = getAccessibleField((FieldBinding)ref.otherBindings[i]);
-                               }
+                               FieldBinding binding = ref.otherBindings[i];
+                               ref.otherBindings[i] = getAccessibleField(binding, receiverType);
+                               receiverType = binding.type;
                        }
                }
        }
 
        public void endVisit(FieldReference ref, BlockScope scope) {
-               if (ref.binding instanceof FieldBinding) {
-                       ref.binding = getAccessibleField((FieldBinding)ref.binding);
-               }
+               ref.binding = getAccessibleField(ref.binding, ref.receiverType);
        }
        public void endVisit(MessageSend send, BlockScope scope) {
                if (send instanceof Proceed) return;
@@ -105,10 +113,10 @@ public class AccessForInlineVisitor extends ASTVisitor {
                
                if (send.isSuperAccess() && !send.binding.isStatic()) {
                        send.receiver = new ThisReference(send.sourceStart, send.sourceEnd);
-                       MethodBinding superAccessBinding = getSuperAccessMethod((MethodBinding)send.binding);
+                       MethodBinding superAccessBinding = getSuperAccessMethod(send.binding);
                        AstUtil.replaceMethodBinding(send, superAccessBinding);
                } else if (!isPublic(send.binding)) {
-                       send.syntheticAccessor = getAccessibleMethod((MethodBinding)send.binding);
+                       send.syntheticAccessor = getAccessibleMethod(send.binding, send.receiverType);
                }
        }
        public void endVisit(AllocationExpression send, BlockScope scope) {
@@ -132,11 +140,11 @@ public class AccessForInlineVisitor extends ASTVisitor {
                makePublic(ref.resolvedType); //getTypeBinding(scope));  //??? might be trouble
        }
        
-       private FieldBinding getAccessibleField(FieldBinding binding) {
+       private FieldBinding getAccessibleField(FieldBinding binding, TypeBinding receiverType) {
                //System.err.println("checking field: " + binding);
                if (!binding.isValidBinding()) return binding;
                
-               makePublic(binding.declaringClass);
+               makePublic(receiverType);
                if (isPublic(binding)) return binding;
                if (binding instanceof PrivilegedFieldBinding) return binding;
                if (binding instanceof InterTypeFieldBinding) return binding;
@@ -145,9 +153,9 @@ public class AccessForInlineVisitor extends ASTVisitor {
                        binding.modifiers = AstUtil.makePackageVisible(binding.modifiers);
                }
                
-               ResolvedMember m = EclipseFactory.makeResolvedMember(binding);
+               ResolvedMember m = EclipseFactory.makeResolvedMember(binding, receiverType);
                if (inAspect.accessForInline.containsKey(m)) return (FieldBinding)inAspect.accessForInline.get(m);
-               FieldBinding ret = new InlineAccessFieldBinding(inAspect, binding);
+               FieldBinding ret = new InlineAccessFieldBinding(inAspect, binding, m);
                
                //System.err.println("   made accessor: " + ret);
                
@@ -155,10 +163,10 @@ public class AccessForInlineVisitor extends ASTVisitor {
                return ret;
        }
        
-       private MethodBinding getAccessibleMethod(MethodBinding binding) {
+       private MethodBinding getAccessibleMethod(MethodBinding binding, TypeBinding receiverType) {
                if (!binding.isValidBinding()) return binding;
                
-               makePublic(binding.declaringClass);  //???
+               makePublic(receiverType);  //???
                if (isPublic(binding)) return binding;
                if (binding instanceof InterTypeMethodBinding) return binding;
 
@@ -167,7 +175,7 @@ public class AccessForInlineVisitor extends ASTVisitor {
                }
 
                
-               ResolvedMember m = EclipseFactory.makeResolvedMember(binding);
+               ResolvedMember m = EclipseFactory.makeResolvedMember(binding, receiverType);
                if (inAspect.accessForInline.containsKey(m)) return (MethodBinding)inAspect.accessForInline.get(m);
                MethodBinding ret = world.makeMethodBinding(
                        AjcMemberMaker.inlineAccessMethodForMethod(inAspect.typeX, m)
@@ -228,4 +236,11 @@ public class AccessForInlineVisitor extends ASTVisitor {
                isInlinable = false;
        }
 
+       public boolean visit(
+               TypeDeclaration localTypeDeclaration,
+               BlockScope scope) {
+               // we don't want to transform any local anonymous classes as they won't be inlined
+               return false;
+       }
+
 }

diff --git a/org.aspectj.ajdt.core/src/org/aspectj/ajdt/internal/compiler/lookup/EclipseFactory.java b/org.aspectj.ajdt.core/src/org/aspectj/ajdt/internal/compiler/lookup/EclipseFactory.java
index 1a4e71fe0e26cbc3c02624c7dc69ce967688cbc8..e75cee9b34d512b06fd456bfb24234be2cb09f01 100644 (file)
--- a/org.aspectj.ajdt.core/src/org/aspectj/ajdt/internal/compiler/lookup/EclipseFactory.java
+++ b/org.aspectj.ajdt.core/src/org/aspectj/ajdt/internal/compiler/lookup/EclipseFactory.java
@@ -182,10 +182,14 @@ public class EclipseFactory {
        }
        
        public static ResolvedMember makeResolvedMember(MethodBinding binding) {
+               return makeResolvedMember(binding, binding.declaringClass);
+       }
+
+       public static ResolvedMember makeResolvedMember(MethodBinding binding, TypeBinding declaringType) {
                //System.err.println("member for: " + binding + ", " + new String(binding.declaringClass.sourceName));
                ResolvedMember ret =  new ResolvedMember(
                        binding.isConstructor() ? Member.CONSTRUCTOR : Member.METHOD,
-                       fromBinding(binding.declaringClass),
+                       fromBinding(declaringType),
                        binding.modifiers,
                        fromBinding(binding.returnType),
                        new String(binding.selector),
@@ -195,9 +199,13 @@ public class EclipseFactory {
        }
 
        public static ResolvedMember makeResolvedMember(FieldBinding binding) {
+               return makeResolvedMember(binding, binding.declaringClass);
+       }
+       
+       public static ResolvedMember makeResolvedMember(FieldBinding binding, TypeBinding receiverType) {
                return new ResolvedMember(
                        Member.FIELD,
-                       fromBinding(binding.declaringClass),
+                       fromBinding(receiverType),
                        binding.modifiers,
                        fromBinding(binding.type),
                        new String(binding.name),

diff --git a/org.aspectj.ajdt.core/src/org/aspectj/ajdt/internal/compiler/lookup/InlineAccessFieldBinding.java b/org.aspectj.ajdt.core/src/org/aspectj/ajdt/internal/compiler/lookup/InlineAccessFieldBinding.java
index f0b844903d36e47921981a38f66b7e82ca99090a..006cb19c1f53ee68e9a0d2ca13be62dafaadc38a 100644 (file)
--- a/org.aspectj.ajdt.core/src/org/aspectj/ajdt/internal/compiler/lookup/InlineAccessFieldBinding.java
+++ b/org.aspectj.ajdt.core/src/org/aspectj/ajdt/internal/compiler/lookup/InlineAccessFieldBinding.java
@@ -15,6 +15,7 @@ package org.aspectj.ajdt.internal.compiler.lookup;
 
 import org.aspectj.ajdt.internal.compiler.ast.AspectDeclaration;
 import org.aspectj.weaver.AjcMemberMaker;
+import org.aspectj.weaver.ResolvedMember;
 import org.eclipse.jdt.internal.compiler.ast.ASTNode;
 import org.eclipse.jdt.internal.compiler.lookup.FieldBinding;
 import org.eclipse.jdt.internal.compiler.lookup.InvocationSite;
@@ -40,17 +41,17 @@ public class InlineAccessFieldBinding extends FieldBinding {
        
        public FieldBinding baseField;
        
-       public InlineAccessFieldBinding(AspectDeclaration inAspect, FieldBinding baseField) {
+       public InlineAccessFieldBinding(AspectDeclaration inAspect, FieldBinding baseField, ResolvedMember resolvedField) {
                super(baseField, baseField.declaringClass);
 
                this.reader = new SimpleSyntheticAccessMethodBinding(
                        inAspect.factory.makeMethodBinding(
                                AjcMemberMaker.inlineAccessMethodForFieldGet(
-                                       inAspect.typeX, EclipseFactory.makeResolvedMember(baseField)
+                                       inAspect.typeX, resolvedField
                        )));
                this.writer = new SimpleSyntheticAccessMethodBinding(inAspect.factory.makeMethodBinding(
                                AjcMemberMaker.inlineAccessMethodForFieldSet(
-                                       inAspect.typeX, EclipseFactory.makeResolvedMember(baseField)
+                                       inAspect.typeX, resolvedField
                        )));
                        
                this.constant = ASTNode.NotAConstant;

diff --git a/tests/ajcTests.xml b/tests/ajcTests.xml
index 4f68d8e006ef79811467d273e98ef87c2e29a3e8..ded12e25e83a57ddb04f14ebbc0de7c598f4532d 100644 (file)
--- a/tests/ajcTests.xml
+++ b/tests/ajcTests.xml
@@ -7172,4 +7172,10 @@
         <compile files="TraceWithInnerV2.aj"/>
         <run class="Main"/>
     </ajc-test>
+    
+       <ajc-test dir="bugs/protectedvf"
+               title="mail list VerifyError with protected access">
+               <compile files="main/Driver.java,main/p2/AbstractTest.aj,main/p1/ConcreteTest.aj"/>
+               <run class="main.Driver"/>
+       </ajc-test>
 </suite>

diff --git a/tests/ajcTestsFailing.xml b/tests/ajcTestsFailing.xml
index 33a3a5d14d6fba3a95d525957825cb12781eebc5..cd8ba3e476cf6f7715a2cb41476edf8b035ae066 100644 (file)
--- a/tests/ajcTestsFailing.xml
+++ b/tests/ajcTestsFailing.xml
@@ -133,12 +133,6 @@
         <run class="InterfaceInitializerOrder"/>
     </ajc-test>
 
-       <ajc-test dir="bugs/protectedvf"
-               title="mail list VerifyError with protected access">
-               <compile files="main/Driver.java,main/p2/AbstractTest.aj,main/p1/ConcreteTest.aj"/>
-               <run class="main.Driver"/>
-               </ajc-test>
-
        <ajc-test dir="bugs/fieldsOnInterfaces"
                pr="52107"
                title="declare String field on interface">

diff --git a/tests/bugs/protectedvf/main/Driver.java b/tests/bugs/protectedvf/main/Driver.java
index 6590760ebb01ccd97c6458c20c24df84c959574e..5ca85cd3246aa78c86701dcb8d5dcf5f9da4c89d 100644 (file)
--- a/tests/bugs/protectedvf/main/Driver.java
+++ b/tests/bugs/protectedvf/main/Driver.java
@@ -9,10 +9,10 @@ public class Driver{
        }
 
        private void doOtherStuff() {
-               System.out.println("doing other stuff");
+               //System.out.println("doing other stuff");
        }
 
        private void doStuff() {
-               System.out.println("doing stuff");
+               //System.out.println("doing stuff");
        }
 }

diff --git a/tests/bugs/protectedvf/main/p1/ConcreteTest.aj b/tests/bugs/protectedvf/main/p1/ConcreteTest.aj
index be044e544145e7823f139f652c0b888f94e248de..26f213b0c940312ef17a085b2d91d7f1a0c63c2a 100644 (file)
--- a/tests/bugs/protectedvf/main/p1/ConcreteTest.aj
+++ b/tests/bugs/protectedvf/main/p1/ConcreteTest.aj
@@ -10,14 +10,16 @@ final aspect ConcreteTest extends AbstractTest {
        protected pointcut pc2(): execution(* Driver.doOtherStuff());\r
 \r
        Object around(): pc2() {\r
-               System.out.println("adding to the other stuff");\r
+               //System.out.println("adding to the other stuff");\r
                /*If we comment out the next line we don't get a verify error.*/\r
+               ConcreteTest ct = this;\r
+               System.out.println("test: " + s + ", " + this.s + ", " + ct.s);\r
                System.out.println("The value of the field when replacing is " + getField());\r
                return proceed();\r
        }\r
 \r
        protected void hook() {\r
-               /*This doesn't cause a verify error seemably because the advice calling it is in AbstractTest*/\r
+               /*This doesn't cause a verify error because this code is not inlined*/\r
                System.out.println("The value of the field is " + getField());\r
        }\r
 }
\ No newline at end of file

diff --git a/tests/bugs/protectedvf/main/p2/AbstractTest.aj b/tests/bugs/protectedvf/main/p2/AbstractTest.aj
index 0c8b040c4fe3c30fd8408f4292e45f22cfd09cf5..748af744d3471b8a03f633e003daa53da7a632af 100644 (file)
--- a/tests/bugs/protectedvf/main/p2/AbstractTest.aj
+++ b/tests/bugs/protectedvf/main/p2/AbstractTest.aj
@@ -3,11 +3,13 @@ package main.p2;
 public abstract aspect AbstractTest {\r
 \r
        private int field;\r
+       protected String s = "test";\r
 \r
        protected abstract pointcut pc();\r
 \r
        Object around(): pc() {\r
                this.field++;\r
+               s += "-1";\r
                hook();\r
                return proceed();\r
        }\r