Comment 1 Jim Hugunin 2004-05-10 14:11:46 EDT

This is a fairly clear bug report.  It would be even better if you also 
included the output you're observing annotated with where you think it is 
wrong.  This is particularly valuable when making a first pass through bug 
reports to assess their severity like I'm doing now.

Unfortunately, this looks like a fairly serious bug that deserves at least one 
more look before the 1.2 release.

The good news is that this bug requires a 3-deep chain of constructors 
combined with advice on some combination of initialization and 
preinitialization join points.  The bad news is that this bug manifests itself 
as silently doing the wrong thing based on the lexical order of the 
constructor declarations.  This could be a really nasty bug for a deployed 
system.

Also unfortunatley, I have no trouble believing that there's a major error 
like this in the code for inlining initializer join points.  That code (in 
BcelClassWeaver) has always been fairly brittle.

Comment 2 Laurie Hendren 2004-05-10 15:00:15 EDT

This bug was really hard to find.   We were trying to figure out exactly
what the preinit/init join points were supposed to do,  and it took us a
long time to find out that the behaviour was changing depending on the
order to constructors.

Below is a modifier ConstructorMain.java file which outputs only the key
joinpoint information and an annotated output of what the program produces
when run.   I hopes this make it easier to understand.

--------------------- ConstructorMain.java 
public class ConstructorMain {
 // Note that in the case of this() calls in a constructor, the
 // treatment of preinitialization and initialization join points are
 // not correct, and are sensitive to the order in which constructors
  // are given in the class file.
  //
  // Below we see everything is ok for preinit and init join points for
  // constructors in class B,  but are not correct for class C.  The only
  // difference is in the order in which the constructors are given.  The
  // inlining strategy for handling this() calls must be broken in ajc.

  public static void main(String args[])
    { int k = 100;
      // These are ok, note order constructors given in class
      System.out.println("----------------------");
      B b3 = new B(3+k, 4+k);
      System.out.println("----------------------");
      B b2 = new B(2 + k);
      System.out.println("----------------------");
      B b1 = new B();

      // First two are ok, but last one not ok
      System.out.println("----------------------");
      C c3 = new C(3+k, 4+k);
      System.out.println("----------------------");
      C c2 = new C(2 + k);
      System.out.println("----------------------");
      C c1 = new C();

      System.out.println("----------------------");
    }
}

class A {
  int x = 4;
  A (int x) { this.x = x; }
}

class B extends A {
  int y;
  static int k = 4;
  static int j = 5;
  static int l = 6;

  B (int x, int y) { super(x+y); this.y = x+y; }

  B (int x) { this(x+l, x+l); this.y = x+l; }

  B () { this(k+j); this.y = l; }


}
class C extends A {
  int y;
  static int k = 4;
  static int j = 5;
  static int l = 6;

  C () { this(k+j); this.y = l; }

  C (int x) { this(x+l, x+l); this.y = x+l; }

  C (int x, int y) { super(x+y); this.y = x+y; }

}

aspect ConstructorAspects {

  static private int aspectnesting = 0;

  static void message(String s)
    { for (int i=0; i<aspectnesting; i++) System.out.print("---+");
      System.out.println(s);
    }


  // call of all constructors
  pointcut allconstrcalls() :  call(*..new(..)) &&
           !within(ConstructorAspects) && !call(java.lang..new(..));

  // execution of all constructors
  pointcut allconstrexecutions() : execution(*..new(..)) &&
           !within(ConstructorAspects);
// intialization of all constructors
  pointcut allconstrinitializations() : initialization(*..new(..)) &&
           !within(ConstructorAspects);

  // preinitialization of all constructors
  pointcut allconstrpreinitializations() : preinitialization(*..new(..)) &&
          !within(ConstructorAspects);

  // before advice
  before () : !within(ConstructorAspects) &&
              (allconstrpreinitializations()  ||
              allconstrinitializations() ||
              allconstrexecutions() ) {
              message(
                  "BEFORE: " +  thisJoinPointStaticPart.getSourceLocation() +
                  " " +thisJoinPointStaticPart.toLongString());
              aspectnesting++;
              }

  // after advice
  after () returning : !within(ConstructorAspects) &&
              (allconstrpreinitializations() ||
              allconstrinitializations() ||
              allconstrexecutions() )
              {
              aspectnesting--;
              message(
                  "AFTER: " +  thisJoinPointStaticPart.getSourceLocation() +
                  " " +thisJoinPointStaticPart.toLongString());
              }

}


---------------------------------- ANNOTATED OUTPUT ---------------------

Look at the third block for the B example, and the third block for the
C example.

******************** Here are the three B examples
----------------------
BEFORE: ConstructorMain.java:45 preinitialization(B(int, int))
AFTER: ConstructorMain.java:45 preinitialization(B(int, int))
BEFORE: ConstructorMain.java:36 preinitialization(A(int))
AFTER: ConstructorMain.java:36 preinitialization(A(int))
BEFORE: ConstructorMain.java:35 initialization(A(int))
---+BEFORE: ConstructorMain.java:35 execution(A(int))
---+AFTER: ConstructorMain.java:35 execution(A(int))
AFTER: ConstructorMain.java:35 initialization(A(int))
BEFORE: ConstructorMain.java:45 initialization(B(int, int))
---+BEFORE: ConstructorMain.java:45 execution(B(int, int))
---+AFTER: ConstructorMain.java:45 execution(B(int, int))
AFTER: ConstructorMain.java:45 initialization(B(int, int))
----------------------
BEFORE: ConstructorMain.java:47 preinitialization(B(int))
AFTER: ConstructorMain.java:47 preinitialization(B(int))
BEFORE: ConstructorMain.java:36 preinitialization(A(int))
AFTER: ConstructorMain.java:36 preinitialization(A(int))
BEFORE: ConstructorMain.java:35 initialization(A(int))
---+BEFORE: ConstructorMain.java:35 execution(A(int))
---+AFTER: ConstructorMain.java:35 execution(A(int))
AFTER: ConstructorMain.java:35 initialization(A(int))
BEFORE: ConstructorMain.java:45 initialization(B(int))
---+BEFORE: ConstructorMain.java:45 execution(B(int, int))
---+AFTER: ConstructorMain.java:45 execution(B(int, int))
---+BEFORE: ConstructorMain.java:47 execution(B(int))
---+AFTER: ConstructorMain.java:47 execution(B(int))
AFTER: ConstructorMain.java:45 initialization(B(int))
----------------------
*********** THIS IS THE CORRECT behaviour ....

*********** preinit of B()
BEFORE: ConstructorMain.java:49 preinitialization(B())
AFTER: ConstructorMain.java:49 preinitialization(B())

************ here is the stuff from super()
BEFORE: ConstructorMain.java:36 preinitialization(A(int))
AFTER: ConstructorMain.java:36 preinitialization(A(int))
BEFORE: ConstructorMain.java:35 initialization(A(int))
---+BEFORE: ConstructorMain.java:35 execution(A(int))
---+AFTER: ConstructorMain.java:35 execution(A(int))
AFTER: ConstructorMain.java:35 initialization(A(int))

************ here is the initialization, with one before/after for
                 each execution of nested constructors
BEFORE: ConstructorMain.java:45 initialization(B())
---+BEFORE: ConstructorMain.java:45 execution(B(int, int))
---+AFTER: ConstructorMain.java:45 execution(B(int, int))
---+BEFORE: ConstructorMain.java:47 execution(B(int))
---+AFTER: ConstructorMain.java:47 execution(B(int))
---+BEFORE: ConstructorMain.java:49 execution(B())
---+AFTER: ConstructorMain.java:49 execution(B())
AFTER: ConstructorMain.java:45 initialization(B())
----------------------
************************** HERE ARE THE THREE C examples, first two are ok
BEFORE: ConstructorMain.java:65 preinitialization(C(int, int))
AFTER: ConstructorMain.java:65 preinitialization(C(int, int))
BEFORE: ConstructorMain.java:36 preinitialization(A(int))
AFTER: ConstructorMain.java:36 preinitialization(A(int))
BEFORE: ConstructorMain.java:35 initialization(A(int))
---+BEFORE: ConstructorMain.java:35 execution(A(int))
---+AFTER: ConstructorMain.java:35 execution(A(int))
AFTER: ConstructorMain.java:35 initialization(A(int))
BEFORE: ConstructorMain.java:65 initialization(C(int, int))
---+BEFORE: ConstructorMain.java:65 execution(C(int, int))
---+AFTER: ConstructorMain.java:65 execution(C(int, int))
AFTER: ConstructorMain.java:65 initialization(C(int, int))
----------------------


BEFORE: ConstructorMain.java:63 preinitialization(C(int))
AFTER: ConstructorMain.java:63 preinitialization(C(int))
BEFORE: ConstructorMain.java:36 preinitialization(A(int))
AFTER: ConstructorMain.java:36 preinitialization(A(int))
BEFORE: ConstructorMain.java:35 initialization(A(int))
---+BEFORE: ConstructorMain.java:35 execution(A(int))
---+AFTER: ConstructorMain.java:35 execution(A(int))
AFTER: ConstructorMain.java:35 initialization(A(int))
BEFORE: ConstructorMain.java:65 initialization(C(int))
---+BEFORE: ConstructorMain.java:65 execution(C(int, int))
---+AFTER: ConstructorMain.java:65 execution(C(int, int))
---+BEFORE: ConstructorMain.java:63 execution(C(int))
---+AFTER: ConstructorMain.java:63 execution(C(int))
AFTER: ConstructorMain.java:65 initialization(C(int))
----------------------
***************** THIS IS INCORRECT Behaviour, should look like B example,

BEFORE: ConstructorMain.java:61 preinitialization(C())
AFTER: ConstructorMain.java:61 preinitialization(C())

*************  here is the problem,  should not have another preinit,
                 this corresponds to the call this(int,int) and should
                 have been inlined.
BEFORE: ConstructorMain.java:65 preinitialization(C(int, int))
AFTER: ConstructorMain.java:65 preinitialization(C(int, int))

**************** here is the super() stuff
BEFORE: ConstructorMain.java:36 preinitialization(A(int))
AFTER: ConstructorMain.java:36 preinitialization(A(int))
BEFORE: ConstructorMain.java:35 initialization(A(int))
---+BEFORE: ConstructorMain.java:35 execution(A(int))
---+AFTER: ConstructorMain.java:35 execution(A(int))
AFTER: ConstructorMain.java:35 initialization(A(int))

***************** here is the other problem, we have two before/after
                  initialization joinpoints,  should only be one, like
                  example in B above.  This due to not inlining the
                  this(int,int) call,  and the this(int,int) call already
                  has its joinpoints woven.

BEFORE: ConstructorMain.java:65 initialization(C(int, int))
---+BEFORE: ConstructorMain.java:65 execution(C(int, int))
---+AFTER: ConstructorMain.java:65 execution(C(int, int))
AFTER: ConstructorMain.java:65 initialization(C(int, int))
BEFORE: ConstructorMain.java:63 initialization(C())
---+BEFORE: ConstructorMain.java:63 execution(C(int))
---+AFTER: ConstructorMain.java:63 execution(C(int))
---+BEFORE: ConstructorMain.java:61 execution(C())
---+AFTER: ConstructorMain.java:61 execution(C())
AFTER: ConstructorMain.java:63 initialization(C())
----------------------

Comment 3 Laurie Hendren 2004-05-11 01:53:15 EDT

I think you can probably fix this by just making sure that:
  you never inline a this() call where the body of corresponding <init> has
   already had its init and preinit joinpoints woven.

We do this in our compiler as follows:
  (1) for all methods in a class, weave everything but preinit and init
        joinpoints
  (2) for each <init> method in the class, check to see if there are preinit
        or init joinpoints,  and if so completely inline until no this() calls
        are left.
  (3) for each <init> method in the class, check to see if there are any preinit
        or init joinpoints,  and weave them

If you can't revisit methods like this in your compiler,  then you will have 
to build a dependency structure of the the <init> methods,  showing which call
the others,  and start by processing the ones which are not called by the others
and work down.

Comment 4 Andrew Clement 2004-05-11 11:32:44 EDT

This bug is due to us making a single pass over the ctors for inlining - 
rather than making multiple passes to ensure all this() calls are removed.  If 
we happen to pass over them in the right order (e.g. in the case of 'B') then 
it works, if we pass over them in the wrong order (e.g. in the case of 'C') 
then it fails.  Effectively we need to correctly implement the scheme Laurie 
describes, which we have the infrastructure to support but don't quite follow 
at the moment.

There now follows my trademark long winded explanation ...

In the example class B, the ctors look like this (in shorthand):

class B {
  B(int,int) { super(int);logic1;}
  B(int)     { this(int,int);logic2;}
  B()        { this(int);logic3;}
}

We currently make a SINGLE pass over the ctors to do inlining of 'this'.  
Let's try that over the ctors above.
Step 1: In B(int,int), no 'this' to inline.

Step 2: In B(int), inline 'this(int,int)', changing class B to this:

class B {
  B(int,int) { super(int);logic1;}
  B(int)     { super(int);logic1;logic2;}
  B()        { this(int);logic3;}
}

Step3: In B(), inline 'this(int)', changing class B to this: (remember, we are 
inlining the new version of B(int) that we created in step2

class B {
  B(int,int) { super(int);logic1;}
  B(int)     { super(int);logic1;logic2;}
  B()        { super(int);logic1;logic2;logic3;}
}

Then, we add in the advice at the join points (I've omitted execution jps):

class B {
 B(int,int){pre_start;pre_end;super(int);init_start;logic1;init_end;}
 B(int)    {pre_start;pre_end;super(int);init_start;logic1;logic2;init_end;}
 B()     {pre_start;pre_end;super(int);init_end;logic1;logic2;logic3;init_end;}
}

Looks perfect.

Now lets try that with class C:

class C { 
 C()        { this(int);logic3;}
 C(int)     { this(int,int);logic2;}
 C(int,int) { super(int);logic1;}
}

Step1: in C(), inline this(int):
class C { 
 C()        { this(int,int);logic2;logic3;}
 C(int)     { this(int,int);logic2;}
 C(int,int) { super(int);logic1;}
}

Step2: in C(int), inline this(int,int):

class C { 
 C()        { this(int,int);logic2;logic3;}
 C(int)     { super(int);logic1;logic2;}
 C(int,int) { super(int);logic1;}
}

Step3: In C(int,int), nothing to inline.. notice we have left behind the this
(int,int) call in C(), so if we annotate the JPs in this:

class B {
 C()     {pre_start;pre_end;this(int,int);init_start;logic2;logic3;init_end;}
 C(int)    {pre_start;pre_end;super(int);init_start;logic1;logic2;init_end;}
 C(int,int){pre_start;pre_end;super(int);init_start;logic1;init_end;}
}

You can see that calling C() includes some preinit JPs then it calls C
(int,int) which has some more in.

If we make a second pass at inlining we remove the secondary 'this' calls.  
The robust implementation is to repeatedly attempt this() inlining until no 
more takes place.  That is what I've implemented.  

Here is the program output with the fix in.
----------------------
BEFORE: ConstructorMain.java:45 preinitialization(B(int, int))
AFTER: ConstructorMain.java:45 preinitialization(B(int, int))
BEFORE: ConstructorMain.java:36 preinitialization(A(int))
AFTER: ConstructorMain.java:36 preinitialization(A(int))
BEFORE: ConstructorMain.java:35 initialization(A(int))
---+BEFORE: ConstructorMain.java:35 execution(A(int))
---+AFTER: ConstructorMain.java:35 execution(A(int))
AFTER: ConstructorMain.java:35 initialization(A(int))
BEFORE: ConstructorMain.java:45 initialization(B(int, int))
---+BEFORE: ConstructorMain.java:45 execution(B(int, int))
---+AFTER: ConstructorMain.java:45 execution(B(int, int))
AFTER: ConstructorMain.java:45 initialization(B(int, int))
----------------------
BEFORE: ConstructorMain.java:47 preinitialization(B(int))
AFTER: ConstructorMain.java:47 preinitialization(B(int))
BEFORE: ConstructorMain.java:36 preinitialization(A(int))
AFTER: ConstructorMain.java:36 preinitialization(A(int))
BEFORE: ConstructorMain.java:35 initialization(A(int))
---+BEFORE: ConstructorMain.java:35 execution(A(int))
---+AFTER: ConstructorMain.java:35 execution(A(int))
AFTER: ConstructorMain.java:35 initialization(A(int))
BEFORE: ConstructorMain.java:45 initialization(B(int))
---+BEFORE: ConstructorMain.java:45 execution(B(int, int))
---+AFTER: ConstructorMain.java:45 execution(B(int, int))
---+BEFORE: ConstructorMain.java:47 execution(B(int))
---+AFTER: ConstructorMain.java:47 execution(B(int))
AFTER: ConstructorMain.java:45 initialization(B(int))
----------------------
BEFORE: ConstructorMain.java:49 preinitialization(B())
AFTER: ConstructorMain.java:49 preinitialization(B())
BEFORE: ConstructorMain.java:36 preinitialization(A(int))
AFTER: ConstructorMain.java:36 preinitialization(A(int))
BEFORE: ConstructorMain.java:35 initialization(A(int))
---+BEFORE: ConstructorMain.java:35 execution(A(int))
---+AFTER: ConstructorMain.java:35 execution(A(int))
AFTER: ConstructorMain.java:35 initialization(A(int))
BEFORE: ConstructorMain.java:45 initialization(B())
---+BEFORE: ConstructorMain.java:45 execution(B(int, int))
---+AFTER: ConstructorMain.java:45 execution(B(int, int))
---+BEFORE: ConstructorMain.java:47 execution(B(int))
---+AFTER: ConstructorMain.java:47 execution(B(int))
---+BEFORE: ConstructorMain.java:49 execution(B())
---+AFTER: ConstructorMain.java:49 execution(B())
AFTER: ConstructorMain.java:45 initialization(B())
----------------------
BEFORE: ConstructorMain.java:63 preinitialization(C(int, int))
AFTER: ConstructorMain.java:63 preinitialization(C(int, int))
BEFORE: ConstructorMain.java:36 preinitialization(A(int))
AFTER: ConstructorMain.java:36 preinitialization(A(int))
BEFORE: ConstructorMain.java:35 initialization(A(int))
---+BEFORE: ConstructorMain.java:35 execution(A(int))
---+AFTER: ConstructorMain.java:35 execution(A(int))
AFTER: ConstructorMain.java:35 initialization(A(int))
BEFORE: ConstructorMain.java:63 initialization(C(int, int))
---+BEFORE: ConstructorMain.java:63 execution(C(int, int))
---+AFTER: ConstructorMain.java:63 execution(C(int, int))
AFTER: ConstructorMain.java:63 initialization(C(int, int))
----------------------
BEFORE: ConstructorMain.java:61 preinitialization(C(int))
AFTER: ConstructorMain.java:61 preinitialization(C(int))
BEFORE: ConstructorMain.java:36 preinitialization(A(int))
AFTER: ConstructorMain.java:36 preinitialization(A(int))
BEFORE: ConstructorMain.java:35 initialization(A(int))
---+BEFORE: ConstructorMain.java:35 execution(A(int))
---+AFTER: ConstructorMain.java:35 execution(A(int))
AFTER: ConstructorMain.java:35 initialization(A(int))
BEFORE: ConstructorMain.java:63 initialization(C(int))
---+BEFORE: ConstructorMain.java:63 execution(C(int, int))
---+AFTER: ConstructorMain.java:63 execution(C(int, int))
---+BEFORE: ConstructorMain.java:61 execution(C(int))
---+AFTER: ConstructorMain.java:61 execution(C(int))
AFTER: ConstructorMain.java:63 initialization(C(int))
----------------------
BEFORE: ConstructorMain.java:59 preinitialization(C())
AFTER: ConstructorMain.java:59 preinitialization(C())
BEFORE: ConstructorMain.java:36 preinitialization(A(int))
AFTER: ConstructorMain.java:36 preinitialization(A(int))
BEFORE: ConstructorMain.java:35 initialization(A(int))
---+BEFORE: ConstructorMain.java:35 execution(A(int))
---+AFTER: ConstructorMain.java:35 execution(A(int))
AFTER: ConstructorMain.java:35 initialization(A(int))
BEFORE: ConstructorMain.java:63 initialization(C())
---+BEFORE: ConstructorMain.java:63 execution(C(int, int))
---+AFTER: ConstructorMain.java:63 execution(C(int, int))
---+BEFORE: ConstructorMain.java:61 execution(C(int))
---+AFTER: ConstructorMain.java:61 execution(C(int))
---+BEFORE: ConstructorMain.java:59 execution(C())
---+AFTER: ConstructorMain.java:59 execution(C())
AFTER: ConstructorMain.java:63 initialization(C())
----------------------

Comment 5 Andrew Clement 2004-05-12 13:12:25 EDT

Fix checked in.  Fixed in AspectJ1.2rc2

Comment 6 Adrian Colyer 2004-05-13 05:24:54 EDT

fixed by Andy.