Bug 150671 - declare error on set of volatile field does not work
Summary: declare error on set of volatile field does not work
Status: RESOLVED FIXED
Alias: None
Product: AspectJ
Classification: Tools
Component: Compiler (show other bugs)
Version: 1.5.2   Edit
Hardware: PC Windows XP
: P3 normal (vote)
Target Milestone: 1.5.3   Edit
Assignee: Helen Beeken CLA
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2006-07-14 12:19 EDT by Arjun Singh CLA
Modified: 2012-04-03 15:48 EDT (History)
0 users

See Also:


Attachments
failing testcase (2.26 KB, patch)
2006-07-24 10:35 EDT, Helen Beeken CLA
aclement: iplog+
Details | Diff
patch containing fix (747 bytes, patch)
2006-07-24 11:27 EDT, Helen Beeken CLA
aclement: iplog+
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Arjun Singh CLA 2006-07-14 12:19:01 EDT
When an aspect has a declare error statement involving a pointcut that captures the setting of a volatile field, a compilation error is not produced.  

This bug can be reproduced as follows.  Consider the following class:

class A {
   private volatile int state;

   public void foo() {
      state = 0;
   }
}

Now consider this aspect:

aspect FSM {
   declare error: set(* A.state): "Changing state";
}

The setting of the state field in method foo() does not result in a compilation error as expected.  

I originally noticed this bug when using AJDT 1.4 with Eclipse 3.2.  However this has been reproduced and confirmed with the aspectj 1.5.2 compiler.
Comment 1 Helen Beeken CLA 2006-07-24 10:35:54 EDT
Created attachment 46703 [details]
failing testcase

Apply this patch to the tests project.

Failing testcase that fits into the AJ test suite using the example code provided by Arjun.
Comment 2 Helen Beeken CLA 2006-07-24 10:37:24 EDT
Note: this bug is not specific to deow statements. Advice is also not applied if the variable 'state' is declared volatile.

The reason this bug is happening is that in the case of the volatile variable we enter the following if statement (SignaturePattern line 307):

  if (aMember.isBridgeMethod() && !allowBridgeMethods) {
    return FuzzyBoolean.MAYBE;
  }

When 'volatile' is removed we bypass this if statement because aMember.isBridgeMethod() returns false. The reason for the difference is in ResolvedMemberImpl.isBridgeMethod() whose implementation is:

   public boolean isBridgeMethod() {
    	return (modifiers & Constants.ACC_BRIDGE)!=0;
    }

where Constants.ACC_BRIDGE = 0x0040. The field with the 'volatile' keyword has modifers = 66 which is why this returns true.
Comment 3 Andrew Clement CLA 2006-07-24 10:41:00 EDT
I thought it might be something like this.  Modifier flags sometimes 'share' bit values (presumably to keep more available for future use).  You can't have a volatile method and you can't have a bridge field - so they are safely mutually exclusive.  the problem is with our code not saying the first condition of being a bridge method is that you *are a method* ... if we also checked it was a method then the isBridgeMethod() wont return true for fields...
Comment 4 Helen Beeken CLA 2006-07-24 11:27:51 EDT
Created attachment 46711 [details]
patch containing fix

Apply this patch to the weaver project.

This patch contains a proposed fix to only return true in ResolvedMemberImpl.isBridgeMethod() if &'ing the modifiers and bridge constants returns non zero and if the ResolvedMemberImpl has kind Member.METHOD.
Comment 5 Andrew Clement CLA 2006-07-25 03:42:54 EDT
patches committed.
Comment 6 Helen Beeken CLA 2006-07-27 03:35:30 EDT
fixes available in the latest dev build