Comment 1 Eugene Kuleshov 2006-10-13 19:11:55 EDT

Created attachment 51991 [details]
class file that cause this failure

Comment 2 Andrew Clement 2006-10-16 10:05:17 EDT

a tricky bug.  the problem appears to be with the diff stored in the class file for recovering the unwoven data.  I have recreated it by trying to weave a very simple aspect into the type attached here.  The problem is more likely to be with the first step though, that creates the embedded diff data.  Is there any way I could have the inputs to the first stage of compilation? (the configurable bean and the aspect?) Is the Configurable bean type built from source with the aspect?

Comment 3 Eugene Kuleshov 2006-10-16 11:04:00 EDT

(In reply to comment #2)
> a tricky bug.  the problem appears to be with the diff stored in the class file
> for recovering the unwoven data.  I have recreated it by trying to weave a very
> simple aspect into the type attached here.  The problem is more likely to be
> with the first step though, that creates the embedded diff data.  Is there any
> way I could have the inputs to the first stage of compilation? (the
> configurable bean and the aspect?) Is the Configurable bean type built from
> source with the aspect?

Andy, does the stored diff contain any method offsets or references to the class constant pool?

In my case, original class had been compiled from the source and adviced by iajc. Then that class is instrumented by custom ASM-based bytecode transformer. So, it is possible that this transformer change the constant pool and the method offsets, because ASM does not know AJ's custom attributes and just pass their content trough as is.

Here are the source for configurable bean. It is been advised by @Configurable advice that is packaged with Spring 2.0.

-------------
package com.tctest.spring.aj;

import java.io.Serializable;

import org.springframework.beans.factory.annotation.Configurable;

@Configurable
public class ConfigurableBean implements Serializable {
  private static final long serialVersionUID = 1L;
  
  private String property1;
  private String property2;
  
  public ConfigurableBean() {
  }
  
  public String getProperty1() {
    return this.property1;
  }
  
  public String getProperty2() {
    return this.property2;
  }
  
  public void setProperty1(String property1) {
    this.property1 = property1;
  }
  
  public void setProperty2(String property2) {
    this.property2 = property2;
  }
  
}

Comment 4 Andrew Clement 2006-10-16 11:26:16 EDT

Oh .......

The diff is of a particular format that we determined was efficient and can be applied by us to a class to get us back to the unwoven class - it is below the level of the class structure - all it knows about a class file is that its a constant pool followed by 'bytes of stuff', it doesn't know anything about method offsets or field descriptors or anything.  We support 'reweaving' in this because undoing the weaving to reweave the type or reweaving already woven bytecode is too complex.  So - it looks like your secondary transform meant the diff could no longer be applied (or rather we did apply it but it produces bad bytes)

The options seem to be:

1. compile time weave aspect1, LTW with aspect2, apply your transform
2. apply your transform, compile time weave aspect1, LTW aspect2

or similar variants, but we don't support other styles of transform occurring in-between two aspectj weaving steps (perhaps a harsh restriction).  Intermediate transforms effectively make the code impossible to reweave - all I could probably manage is a nicer error than the ClassFormatException...

Can you not capture your transform as an aspect? that would make all the problems go away...

Comment 5 Matthew Webster 2006-10-16 11:51:40 EDT

Andy,

When using the compressed from of -Xreweavable (the default) should we employ some form of fingerprint or check-sum (perhaps just file size) to ensure the byte-code has not been modified? Is this what you are alluding to when when you say "all I could probably manage is a nicer error than the ClassFormatException..."? This fingerprint would then be used by the "processing reweavable type" logic to determine the integrity of the class.

Comment 6 Eugene Kuleshov 2006-10-16 12:22:37 EDT

(In reply to comment #4)
> The diff is of a particular format that we determined was efficient and can be
> applied by us to a class to get us back to the unwoven class - it is below the
> level of the class structure - all it knows about a class file is that its a
> constant pool followed by 'bytes of stuff', it doesn't know anything about
> method offsets or field descriptors or anything.  We support 'reweaving' in
> this because undoing the weaving to reweave the type or reweaving already woven
> bytecode is too complex.  So - it looks like your secondary transform meant the
> diff could no longer be applied (or rather we did apply it but it produces bad
> bytes)

This is definetely unfortunate...

I guess the only way to make reweaving work after such intermediate transformations is to somehow isolate method instructions introduced/removed at the 1st weaving step. Constant pool changes could be made more resistant if you'd always add to the constant pool and make those diffs not to use any offsets in the pool... e.g. somehow similar to how ASM handle constant pool data.

> The options seem to be:
> 1. compile time weave aspect1, LTW with aspect2, apply your transform
> 2. apply your transform, compile time weave aspect1, LTW aspect2

Right. I already checked and both of them works.

> or similar variants, but we don't support other styles of transform occurring
> in-between two aspectj weaving steps (perhaps a harsh restriction). 
> Intermediate transforms effectively make the code impossible to reweave - all I
> could probably manage is a nicer error than the ClassFormatException...

Would it be possible to allow to ignore this error and log a warning or something?
I also wonder if you can somehow check if reweaved advice is the same as the one that already woven in?

> Can you not capture your transform as an aspect? that would make all the
> problems go away...

I am afraid this problem will became quite common soon since not all transformations can be implemented on AspectJ (at least in real world). For example, instrumentation that is done by JDO/JPA implementations...

Comment 7 Andrew Clement 2006-10-16 12:39:07 EDT

A complex diff that takes into account instructions/members/constant pool entries added and removed would slow us down quite a lot ... and not everyone wants to reweave so it is a shame to make everyone suffer for it.  So next you might say make it optional - but i'd say no-one takes into account whether they need their output to be reweavable or not and would set it off, causing problems for someone further down the line.

> Would it be possible to allow to ignore this error and log a warning
> or something?

When you say 'ignore the error' - what would we then weave? would we weave the already previously woven code and assume it is safe?  We can't do that - the first weave will have introduced all sorts of artifacts and juggled instructions around - all of which will be different join points for the secondary weave.

> I also wonder if you can somehow check if reweaved advice is the same as 
> the one that already woven in?

Yes, we could do that - but I believe it is a lot of work - for things like inlined around advice, potentially a lot of extra meta data to store so we can identify what went were previously, bloating class files further.

This is the first time this problem has been hit - I agree it may become more of an issue in the future.

Comment 8 Eugene Kuleshov 2006-10-16 13:13:57 EDT

(In reply to comment #7)
> A complex diff that takes into account instructions/members/constant pool
> entries added and removed would slow us down quite a lot ... and not everyone
> wants to reweave so it is a shame to make everyone suffer for it.  So next you
> might say make it optional - but i'd say no-one takes into account whether they
> need their output to be reweavable or not and would set it off, causing
> problems for someone further down the line.

It not necessary have to slow things down. All depends how you do it. :-)

> > Would it be possible to allow to ignore this error and log a warning
> > or something?
> 
> When you say 'ignore the error' - what would we then weave? would we weave the
> already previously woven code and assume it is safe?  We can't do that - the
> first weave will have introduced all sorts of artifacts and juggled
> instructions around - all of which will be different join points for the
> secondary weave.

I meant just completely skip the ltw weaving and leave class as is, don't even apply the diff.

> > I also wonder if you can somehow check if reweaved advice is the same as 
> > the one that already woven in?
> 
> Yes, we could do that - but I believe it is a lot of work - for things like
> inlined around advice, potentially a lot of extra meta data to store so we can
> identify what went were previously, bloating class files further.

Depends how you do that. For instance you can just calculate and save check sum (e.g. md5) for each aspect class aplied to the instrumented class. Then, if those check sums are the same in the runtime, you don't need to reweave...

Comment 9 Matthew Webster 2006-10-17 04:26:05 EDT

>I meant just completely skip the ltw weaving and leave class as is, don't even
>apply the diff.
No. A user, either with a pointcut or aop.xml configuration, can explicitly exclude a class from weaving. On the other hand the compiler cannot exclude a class because it tried and failed to weave it.

>Depends how you do that. For instance you can just calculate and save check sum
>(e.g. md5) for each aspect class aplied to the instrumented class. Then, if
>those check sums are the same in the runtime, you don't need to reweave...
We would not only have to calculate check-sums for all the aspects concerned (and store the numbers for those used at each stage in the byte-code) but also for each class to see if it had changed. How expensive is md5 generation? Also what if intermediate, non-AspectJ instrumentation introduced new join points?

Comment 10 Eugene Kuleshov 2006-10-17 04:40:41 EDT

(In reply to comment #9)
> >I meant just completely skip the ltw weaving and leave class as is, don't even
> >apply the diff.
> No. A user, either with a pointcut or aop.xml configuration, can explicitly
> exclude a class from weaving. On the other hand the compiler cannot exclude a
> class because it tried and failed to weave it.

Makes sense.

> >Depends how you do that. For instance you can just calculate and save check sum
> >(e.g. md5) for each aspect class aplied to the instrumented class. Then, if
> >those check sums are the same in the runtime, you don't need to reweave...
> We would not only have to calculate check-sums for all the aspects concerned
> (and store the numbers for those used at each stage in the byte-code) but also
> for each class to see if it had changed. 

Won't be enough to only include aspects for matched joinpoints?

> How expensive is md5 generation? 

Not that much actually and it runs on a fixed memory boundaries, so it should be quite fast when bytecode is already in memory. BTW, md5 is calculated when computing implicit serialVersionUid for serializable classes, but you can use simpler and shorter checksum, like crc32.

> Also what if intermediate, non-AspectJ instrumentation introduced new join points?

Good point! Can AspectJ only instrument those new join points? :-)

Comment 11 Matthew Webster 2006-10-17 08:06:44 EDT

>Won't be enough to only include aspects for matched joinpoints?
I think there's a chicken and egg problem here. We don't know which aspects will match until we weave and we won't weave unless we think something has changed. Generating the md5 for each aspect is not a problem as it will be done once but doing it for every class is new overhead.

>Good point! Can AspectJ only instrument those new join points? :-)
The 3rd party instrumentation may introduce join points which may be matched by an aspect. This aspect, not necessarily authored by the same owner, may reasonably assume all classes that should match will match. If you miss some out the resulting behaviour may be unpredictable.

Comment 12 Eugene Kuleshov 2006-10-17 08:55:44 EDT

(In reply to comment #11)
> >Won't be enough to only include aspects for matched joinpoints?
> I think there's a chicken and egg problem here. We don't know which aspects
> will match until we weave and we won't weave unless we think something has
> changed. 

My thinking was along these lines:

-- at compile-time AJ could resolve all the matching advices and record those that are applied
-- at load-time AJ could again if there are matching advices, then check which of those already applied (or require to be reapplied due to advice change) and then apply the new ones.

> Generating the md5 for each aspect is not a problem as it will be done
> once but doing it for every class is new overhead.

You would have to do it anyways in order to check if class had been changed.

> >Good point! Can AspectJ only instrument those new join points? :-)
> The 3rd party instrumentation may introduce join points which may be matched by
> an aspect. This aspect, not necessarily authored by the same owner, may
> reasonably assume all classes that should match will match. If you miss some
> out the resulting behaviour may be unpredictable.

But that will be a different/new aspect, so it will be applied to whathever matches at that time. Responsibility would be on that intermediate instrumentation...

Comment 13 Matthew Webster 2006-10-18 04:28:55 EDT

There is a contract here.
1. When we weave a class we ensure it will execute on any JVM
2. When we weave a class we ensure it can be woven again
3. When we weave a class we ensure it can be instrumented by a 3rd party

What we don't guarantee is that between 2 AspectJ weaving steps a 3rd party can modify the code.

>> Can you not capture your transform as an aspect? that would make all the
>> problems go away...

>I am afraid this problem will became quite common soon since not all
>transformations can be implemented on AspectJ (at least in real world). For
>example, instrumentation that is done by JDO/JPA implementations...
Yes but can _you_ implement _your_ transform, the subject of _this_ bug report, using AspectJ? This would solve other problems that we have identified such as whether or not new join points should be matched or reconciling precedence.

Coexistence with other byte-code enhancement technologies will probably required multiple -javagents which is not supported in Java 5. 

Comment 14 Eugene Kuleshov 2006-10-18 04:54:56 EDT

(In reply to comment #13)
> >I am afraid this problem will became quite common soon since not all
> >transformations can be implemented on AspectJ (at least in real world). For
> >example, instrumentation that is done by JDO/JPA implementations...
> Yes but can _you_ implement _your_ transform, the subject of _this_ bug report,
> using AspectJ? This would solve other problems that we have identified such as
> whether or not new join points should be matched or reconciling precedence.

I am afraid that is not an option. The complexity of transformation is quite high and certain things can't be done in AspectJ. As I said, my transformation is not unique in that sense.

> Coexistence with other byte-code enhancement technologies will probably
> required multiple -javagents which is not supported in Java 5. 

What do you mean? It is allowed to specify multiple agents.

Comment 15 Matthew Webster 2006-10-18 05:34:49 EDT

>I am afraid that is not an option. The complexity of transformation is quite
>high and certain things can't be done in AspectJ. As I said, my transformation
>is not unique in that sense.
While we are not trying to take over the world :-) it would be interesting to discover what you need to do that isn't currently supported by AspectJ.

>What do you mean? It is allowed to specify multiple agents.
I was (mistakenly) thinking of the older interface. You can specify -javaagent multiple times although the option specification (http://java.sun.com/j2se/1.5.0/docs/api/java/lang/instrument/package-summary.html) is not explicit about the order in which they will be invoked: it is assumed that it's the order in which they are configured. In which case the solution is to specify -javaagent:aspectjweaver.jar first.