[aarch64-port-dev ] RFR: JDK-8143584 Aarch64: Load constant pool tag and class status with load acquire

[Hui Shi]

Thanks Andrew! You're right. StoreStore is added after class initialization
status setting. Load acquire for class initialization status check should
be unnecessary even on aarch64.


InstanceKlass._layout_helper is written in ClassFileParser::parseClassFile
when resolving class and create InstanceKlass. InstanceKlass._init_state is
updated in InstanceKlass::set_initialization_state_and_notify_impl when
initializing class. They can happen in different threads in my
understanding. InstanceKlass’ content (for fields like _layout_helper)
should be correct for all reference no matter class is initialized or not
after class resolutions.



I haven't find explicit code synchronize InstanceKlass' content. In
SystemDictionary::load_instance_class, it uses MutexLocker and this might
help.


new webrev in http://cr.openjdk.java.net/~hshi/8143584/webrev_v2/


Regards

Shi Hui

On 24 November 2015 at 22:05, Andrew Haley <aph at redhat.com> wrote:

> On 11/24/2015 01:23 PM, Hui Shi wrote:
>
> > Checking similar places for aarch64 runtime, more places needs similar
> fix:
> > 1. Instanceof is similar with checkcast
> > 2. "ldc", similar with load resolved class in checkcast, need load
> acquire
> > when loading tag in constant pool
> > 3. "new" loads tag for resolved class.
> > 4. "new" Loads and checks class initialized status, class init status
> > update is guarded with orderaccess::storestore, need guarantee order
> > between load class initialize status and load "instance_size in
> > InstanceKlass".
> > 5. C1 runtime checks if class initialize status when generate code for
> > fast_new_instance_init_check_id, similar with "new".
>
> I'm looking at 4, the InstanceKlass::init_state change:
>
> @@ -3326,7 +3328,8 @@
>
>    // make sure klass is initialized & doesn't have finalizer
>    // make sure klass is fully initialized
> -  __ ldrb(rscratch1, Address(r4, InstanceKlass::init_state_offset()));
> +  __ lea(rscratch1, Address(r4, InstanceKlass::init_state_offset()));
> +  __ ldarb(rscratch1, rscratch1);
>    __ cmp(rscratch1, InstanceKlass::fully_initialized);
>    __ br(Assembler::NE, slow_case);
>
> While this looks reasonable enough, the init_state accessors in
> InstanceKlass don't seem to use fences. Therefore, as far as I can
> see, the set_init_state (fully_initialized) doesn't have a
> happens-before relationship with the load of init_state above.
>
> This is in eager_initialize_impl():
>
>     // linking successfull, mark class as initialized
>     this_k->set_init_state (fully_initialized);
>     this_k->fence_and_clear_init_lock();
>
> Note that the call to fence_and_clear_init_lock() (which contains a
> storestore) is *after* the set_init_state().  If set_init_state()
> needs to synchronize with the reading code, a storestore must come
> *before* set_init_state() is called.
>
> The rest look OK.
>
> Andrew.
>