RFR: 8199739: Use HeapAccess when loading oops from static fields in javaClasses.cpp

[Roman Kennke]

Am 17.03.2018 um 19:41 schrieb Kim Barrett:
>> On Mar 17, 2018, at 6:39 AM, Roman Kennke <rkennke at redhat.com> wrote:
>>
>> Am 17.03.2018 um 05:11 schrieb Kim Barrett:
>>>> On Mar 16, 2018, at 10:39 AM, Stefan Karlsson <stefan.karlsson at oracle.com> wrote:
>>>>
>>>> Hi all,
>>>>
>>>> Please review this patch to use HeapAccess<>::oop_load instead of oopDesc::load_decode_heap_oop when loading oops from static fields in javaClasses.cpp:
>>>>
>>>> http://cr.openjdk.java.net/~stefank/8199739/webrev.01/
>>>> https://bugs.openjdk.java.net/browse/JDK-8199739
>>>>
>>>> It's necessary to use HeapAccess<>::oop_load to inject load barriers for GCs that need them.
>>>>
>>>> Thanks,
>>>> StefanK
>>>
>>> ------------------------------------------------------------------------------ 
>>> src/hotspot/share/classfile/javaClasses.cpp.
>>>
>>> 1870   address addr = ik->static_field_addr(static_unassigned_stacktrace_offset);
>>> 1871   return HeapAccess<>::oop_load((HeapWord*)addr);
>>>
>>> I'm not sure this is sufficient.  Isn't static_field_addr just
>>> fundamentally broken for Shenandoah?
>>
>> It is not totally broken (I think _addr() calls resolve() which kinda
>> does what we need),
> 
> Current implementation of static_field_addr is:
> 
> address InstanceKlass::static_field_addr(int offset) { 
>   assert(offset >= InstanceMirrorKlass::offset_of_static_fields(), "has already been adjusted"); 
>   return (address)(offset + cast_from_oop<intptr_t>(java_mirror()));
> }
> 
> java_mirror() calls OopHandle::resolve(), which presently doesn’t do anything wrto Access
> (has OopHandle not yet been Accessorized?  Or is there some reason why it’s okay as is
> that I’m not thinking of right now?)  Even if it did use Access, I would expect for Shenandoah
> the access would just be a dereference of the oop* in the OopHandle, and would not chase
> the Brooks pointer, since we’re not accessing fields in the mirror object at that point.
> 

Oh oops, I confused this with typeArrayOop::*_addr(). Sorry. Yeah, we
need appropriate access barriers in there too. I think it would be best
if we either had static_field() and static_field_put() accessors in
instanceKlass, or simply go vial ik->java_mirror()->*_field() and
ik->java_mirror()->*_field_put() ?

Roman