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

[Igor Veresov]

Looks good.

igor

On 2/28/11 5:37 PM, 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