review (S) for 6965570: assert(!needs_patching && x->is_loaded(), "how do we know it's volatile if it's not loaded")

[Tom Rodriguez]

Thanks!

tom

On Mar 1, 2011, at 11:02 AM, Igor Veresov wrote:

> Looks good.
> 
> igor
> 
> On 3/1/11 10:32 AM, Tom Rodriguez wrote:
>> http://cr.openjdk.java.net/~never/6965570
>> 
>> 6965570: assert(!needs_patching&&  x->is_loaded(),"how do we know it's volatile if it's not loaded")
>> Reviewed-by:
>> 
>> During code generation for a volatile getfield in an outer class we've
>> managed to resolve the inner class symbolically but the constant pool
>> reference hasn't actually been resolved.  This results in a field
>> reference that should be patched so that the proper access checks are
>> performed by constant pool resolution.  In this case the field is
>> volatile and LIRGenerator expects that if the class was loaded and we
>> can see that's it actually volatile that a patch must not be needed.
>> In debug mode this asserts but in product mode it generates code to
>> uses max_jint for the field offset.  The fix is to always respect the
>> needs_patching flag.  We could makes the patching machinery handle
>> this more normally but that would complicate patching even more.  It's
>> a rare case so allowing deopt in Runtime1::patch_code to handle it is
>> the easiest way to go.
>> 
>> Part of the confusion stems from the is_loaded flag on AccessField
>> which doesn't really mean what it says, so I've removed it and pushed
>> all the needs_patching logic up into GraphBuilder.  is_initialized()
>> is similarly confusing since it really means does this static field
>> need patching.  I've eliminated that flag from AccessField and
>> captured that logic in a new method that is used instead.  Tested with
>> runthese with and without PatchALot, ctw, and the nsk jvmti tests.
>