Advanced - Powered by Google


   
Log In
New Account
  
 
Home
My Page
Project Tree
Project Openings
ASM
          
 
 
Summary
Tracker
Lists
News
CVS
Files
SVN
              
 

Tracker: Bugs

Submit New | Browse | Admin | ExportToXml

[ #316360 ] MethodWriter.visitFrame(F_NEW,...) requires explicit invocation for the implicit frame at offset zero

Date:
2012-08-05 23:55
Priority:
5
Submitted By:
Mitch Rudominer (rudominer)
Assigned To:
Nobody (None)
Category:
core
State:
Closed
Summary:
MethodWriter.visitFrame(F_NEW,...) requires explicit invocation for the implicit frame at offset zero

Detailed description:
I am using ClassReader with EXPAND_FRAMES followed by some method adapters followed by ClassWriter with COMPUTE_MAXS. My method adapters require the insertion of new frames, and I accomplish this by invoking super.visitFrame(F_NEW,...) to insert uncompressed frames and allowing ASM to compress the frames. I noticed that most of the time the first frame that was supposed to appear in the generated bytecode was missing. I studied the source code of MethodWriter and I understand why: writeFrame() expects previousFrame to be set, previousFrame is set in endFrame(), and with F_NEW and COMPUTE_MAXS, endFrame is only ever invoked from from the public visitFrame(). This implies that visitFrame() must be explicitly invoked for the implicit frame at offset zero. But its not that simple. At first I tried always explicitly invoking visitFrame() for the implicit frame at offset zero. But the problem is that there is some bytecode (I think it is bytecode generated from Java source in which the method starts with a while loop) in which ClassReader will explicitly invoke visitFrame() itself for the implicit frame at offset zero. This meant that, along with my invocation of visitFrame(), there were two invocations of visitFrame() for the implicit frame, and so there actually was a frame at offset zero in the generated bytecode. It turns out that this is OK, the Verifier doesn't mind the explicit frame at offset zero. But the problem is worse. It turns out that there is some bytecode for which ClassReader will invoke visitFrame() *twice* for the implicit frame at offset zero. Along with my invocation of visitFrame() this means that visitFrame() was being invoked *three* times for the implicit frame at offset zero. This is a problem.: The first invocation causes MethodWriter to set previousFrame. The second invocation causes MethodWriter to write an explicit frame at offset zero. The third invocation causes MethodWrtier to write a second explicit frame with an offset delta of -1. The offset delta of -1 causes the bytecode to be invalid. Even javap gets an exception trying to parse it. I see at least two ASM bugs here: (1) MethodWriter should not require an explcit invocation of visitFrame() for the implicit frame at offset zero. (2) MethodWriter should not write an offset delta of -1 if visitFrame() is invoked twice at the same offset. My work around for this bug is the following: I have a visitor called InitialStackFrameVisitor which sits just before ClassWriter. InitialStackFrameVisitor will explicitly invoke visitFrame for the implicit frame at offset zero. But it will also keep track of every visit call and it will remember whether there has been an instruction written since the last time a frame was written. If it detects that visitFrame() is called twice at the same offset, it will not forward the second call to visitFrame() on to ClassWriter.

Add A Comment:

Please login

Followup

Message
Date: 2012-10-01 20:33
Sender: rudominer
Logged In: YES 
user_id=32429

Eric,

Yes the patch works! Thanks you very much. 

Also thank you for building the jar file for me. That was 
easier for me than working with the patch.

More detail: I used the new jar file and I removed my 
InitialStackFrameVisitor and I ran my test suite and all the 
tests passed. That gives me confidence that your fix works 
for me.

The patch for issue 316352 also works.

Thank you again
-Mitch
Date: 2012-09-29 10:50
Sender: ebruneton
Logged In: YES 
user_id=128

Please let us know if the patch is ok for you, as well as
the one for issue 316352. We would like to close these bugs
and include the patches in the upcoming 4.1 release.

Regarding the check for offset_delta, the patch includes a
check for this in CheckMethodAdapter.
Date: 2012-09-22 18:50
Sender: rudominer
Logged In: YES 
user_id=32429

Hi Eric,
Thank you for your detailed response, and thank you for the 
patch. I will try it out as soon as I can.

Obviously I don't have nearly the breadth of understanding 
of the different ways in which people use ASM as you do. I 
only understand the limited manner in which I use ASM. From 
this perspective your third solution seems the best. If 
breaking existing code is to be avoided at all costs you 
might add a flag (maybe a new argument to the ClassWriter 
constructor?) to switch between the two modes. (I don't love 
this solution but it would avoid breaking anybody.)

Regarding adding code during visitCode(), I would point out 
that you do this in section 3.2.4 of the ASM User Guide. 
(Aside: I love the ASM User Guide. Thank you for writing it 
so well.)

Regarding the offset_delta=-1 issue: Sure I understand why 
it is happening and I agree that ASM should not allow 
consecutive frames at the same offset. I was questioning 
whether it is the right solution to have MethodWriter 
blindly write the value -1 into the bytecode (as opposed to, 
say, throwing an exception or silently omitting the second 
frame). The problem with the current behavior, from my 
perspective, is that the resulting bytecode cannot be parsed 
by javap. Because of this it took me a really long time to 
figure out the cause of the resulting ClassFormatError. I 
know that ASM avoids doing a lot of error checking in order 
to stay lean. But here is an instance in which a certain 
integer being negative implies a guaranteed 
ClassFormatError. It might be worth the check.

-Mitch
Date: 2012-09-22 12:04
Sender: ebruneton
Logged In: YES 
user_id=128

Here is a patched version of asm4.jar for 'newpatch.patch'.
Date: 2012-09-22 12:03
Sender: ebruneton
Logged In: YES 
user_id=128

So I see 3 solutions to this, but all of them may break
existing code:
- require an explicit visit of the implicit first frame
before any instruction, as it has always been, but fixed
with the proposed ClassReader patch. Then as you discovered
this implies that visitCode cannot be used to insert code
(instead one must manually detect the first instruction by
overriding all visitXxxInsn methods). This "mistake" is
quite common (e.g. in AdviceAdapter), so fixing it would
require many changes in users code.
- call visitFrame for this first implicit frame before
visitCode. This is much less likely to break existing code I
think, but seems like a hack.
- never visit the implicit first frame, and recompute it in
MethodWriter (we still need to compute it in ClassReader to
uncompress the others; so this computation would be done
twice). Again this may break existing code relying on the
current behavior. On the other hand it fixes existing code
using visitCode to insert instructions.

The last option is probably the less bad. I attach a patch
implementing it.

About the offset_delta = -1 issue, the class file format
simply does not allow, by construction, two frames to have
the same offset (and this is done on purpose, because the
verifier would not know how to merge them). For this, the
offset between two consecutive frames is stored as an
unsigned value representing the offset minus 1.

For the same reasons, ASM does no allow consecutive frames
at the same offset. If you add a frame, you must take care
of merging it with the existing frame for this offset, if any.
Date: 2012-09-20 19:41
Sender: rudominer
Logged In: YES 
user_id=32429

Correction to my previous post:
Final paragraph:
s/Your patch to MethodWriter/Your patch to ClassReader/
Date: 2012-09-20 19:38
Sender: rudominer
Logged In: YES 
user_id=32429

Hi Eric,

I finally managed to test your patch for this bug and I'm 
sorry to say it did not work. I am still having similar 
issues.

Upon further reflection it seems to me that this fix could 
not possibly have worked for my issue and I should have 
realized it right away. It is not possible for ClassReader 
to be the sole party responsible for invoking visitFrame() 
on the implicit frame at offset zero. This is because 
ClassReader must invoke visitCode() before it invokes 
visitFrame() and method adapters are allowed to add code 
during visitCode(). 

That is in fact what is happening in my case. I want to add 
code at the start of a method and so I do it from within 
visitCode().

My workaround of having InitialStackFrameVisitor in line 
just before ClassWriter works because 
InitialStackFrameVisitor invokes visitFrame() for the 
implicit frame during visitCode().

Here is a bit more detail: In order to test out your patch I 
removed my InitialStackFrameVisitor. As mentioned above, I 
have a MethodVisitor that inserts code during visitCode(). 
That method visitor also invokes visitFrame() because it 
adds a new jump target.

Your patch to MethodWriter will then invoke visitFrame() 
again later, thinking it is visiting the implicit frame but 
actually it is not because I have already added code. The 
result is: java.lang.ClassFormatError: StackMapTable format 
error: access beyond the end of attribute. This is due, as I 
wrote about earlier in this bug, to visitFrame() being 
invoked twice at the same offset and consequently computing 
an offset delta of -1.
Date: 2012-09-01 16:38
Sender: ebruneton
Logged In: YES 
user_id=128

I updated the Javadoc of visitFrame in MethodVisitor. I also
added checks for this in CheckMethodAdapter (see
http://websvn.ow2.org/revision.php?repname=asm&path=%2Ftrunk%2F&r
ev=1637).
Can you confirm that this solves your problem?
Date: 2012-08-29 18:28
Sender: rudominer
Logged In: YES 
user_id=32429

OK, got it. Thanks for the instructions.

Also thanks for the explanation. Given your explanation I 
would make one suggestion: The documentation of ClassWriter 
and the ASM Guide should explain that in the case that the 
ClassWriter/MethodWriter is being used directly (i.e. 
without ClassReader) that it is necessary for the caller to 
explicitly invoke visitFrame for the implicit frame. If I 
recall correctly the example of using ClassWriter in the ASM 
Guide does not use F_NEW and so there is, as far as I can 
tell, no way for a user to know that he needs to do this.
Date: 2012-08-19 14:18
Sender: ebruneton
Logged In: YES 
user_id=128

You can download the latest sources from
http://forge.ow2.org/svnsnapshots/asm-svn-latest.tar.gz,
apply the patch, and simply include the classes in your
project (or build the jars with 'ant jar' from the 'asm'
subdirectory).

Currently ClassReader with EXPAND_FRAMES generates the
implicit frame for you, and class adapters expect this (as
well as MethodWriter). But there is a bug, namely that if a
method does not have a stack map table, then EXPAND_FRAMES
does not generate this implicit frame. This is an
inconsistent behavior, which could cause bugs in class
adapters (and I think the bug you got was precisely an
example of this). This is why I propose to fix ClassReader
rather than modifying MethodWriter. Before that I want to be
sure that this fix also solves your problem.

I attached an updated patch, for the latest svn version.
Date: 2012-08-11 23:11
Sender: rudominer
Logged In: YES 
user_id=32429

Thanks Eric!

I'm not currently building ASM from source so I will have to 
figure out how to do that to test your patch.

One thought right away: It is possible to experience this 
bug without using ClassReader. If I want to generate new 
bytecode with frames using visitFrame(F_NEW,...), currently 
I have to call visitFrame() for the initial implicit frame 
in order to initialize MethodWriter. In my opinion it would 
be better if I did not have to do this. So I think an ideal 
fix to this bug will be a change to MethodWriter as opposed 
to ClassReader.

But I am going to try out your patch.

Thanks again,
Mitch
Date: 2012-08-07 19:12
Sender: ebruneton
Logged In: YES 
user_id=128

Does the attached patch solve the problem (if you remove
your InitialStackFrameVisitor)?
Date: 2012-08-06 00:20
Sender: rudominer
Logged In: YES 
user_id=32429

One clarification to my description: It turns out that the 
verifier actually *requires* an explicit frame at offset zero 
if offset zero is a branch target. This explains why 
ClassReader sometimes invokes visitFrame at offset zero. This 
clarification does not change anything substantive in the bug 
report.

Attached Files:

Name Description Download
patched-asm4.jar Download
newpatch.patch Download
ClassReader.java.patch.new Download
ClassReader.java.patch Download

Changes:

Field Old Value Date By
close_date2012-10-14 17:052012-10-14 17:05ebruneton
resolution_idNone2012-10-14 17:05ebruneton
status_idOpen2012-10-14 17:05ebruneton
File Added2327: patched-asm4.jar2012-09-22 12:04ebruneton
File Added2326: newpatch.patch2012-09-22 12:03ebruneton
File Added2323: ClassReader.java.patch.new2012-08-19 14:19ebruneton
File Added2320: ClassReader.java.patch2012-08-07 19:12ebruneton

Copyright © 1999-2008, OW2 Consortium | contact | webmaster.