RFR: JDK-8222545: Safe klass asserts

[Roman Kennke]

>>>> To add a little more detail, I could move the change up into 
>>>> is_objArray(), but I don't want to expose it to any non-assert 
>>>> paths. Therefore I could do 2 different impls there, guarded by 
>>>> #ifdef ASSERT but I don't think it's a good idea to behave 
>>>> differently under ASSERT, that kindof defeats the point of assert, 
>>>> right?
>>>>
>>>> What do you think ?
>>>
>>> I don't follow your argument. Under asserts you need to access the 
>>> klass pointer "safely" but otherwise you do not. So there are two 
>>> behaviours related to accessing the klass pointer anyway. I'd rather 
>>> see that encapsulated in the accessor.
>>>
>>> I assume it's not just asserts but any debug only code that wants to 
>>> access the klass pointer.
>>
>> In general, for any runtime calls into oopDesc::klass() the access 
>> should be safe. The acrobatics is only necessary for *GC-internal* 
> 
> This is the part I don't quite understand, and goes back to my initial 
> question. Why are you doing these operations on from-space objects? I'm 
> thinking you should be in a position in the GC to make sure this can 
> never happen. If you need to do that in the GC (which is fine), then the 
> GC could apply a "resolve" function to get the to-space object, and call 
> size() (or whatever) on that object. This shouldn't have to leak out of 
> the GC, right?

It is a problem when we are about to evacuate an object. Then we need to 
know its size in order to allocate and copy an appropriate chunk. The 
problem is that this part is racy: two threads (e.g. two Java threads 
via barrier, or one Java thread vs one GC thread) might compete over 
this: both would create a copy of the object, but ultimately only one 
would succeed (by CASing the fwd pointer). Therefore, getting hold of 
the object size is racy, by design, and this requires to resolve the 
_klass. Now, we can do that ahead of time, and call 
oopDesc::size_given_klass() and all would be good, except that 
size_given_klass() asserts that the object is indeed of the given klass, 
and hence fetches _klass again, which, at this point, is racy. Solving 
this inside the GC would require to basically copy all the machinery to 
get hold of object size into the GC. Are you asking me to do that?

About the oop_oop_iterate() paths, I will revisit them. We should never 
actually need to iterate over a from-space object.

Roman


> cheers,
> Per
> 
>> calls, which can happen in 'unsafe' situations, where decoding the 
>> Klass* would be necessary. The way I do it is to call into acccessors 
>> like size_given_klass() or the oop_oop_iterate() methods, that take a 
>> pre-resolved Klass* as argument. But that only works if those 
>> code-paths don't call into klass() themselves. This is what this patch 
>> addresses.
>>
>> I guess we could put the call via safe_klass() into the accessors, but 
>> it would widen its exposure unnecessarily. If we don't want special 
>> paths for ASSERT via non-ASSERT there, it could actually be done in 
>> the GC backend I suppose, but the way I proposed it seems minimal 
>> exposure of GC fluff.
>>
>> Alternatively, it could be argued that we're in the Klass* instance 
>> already anyway, and what is the point of asserting it again, but I 
>> also see that we might want to ensure that we're not calling anything 
>> typeArray-ish on an objArray by accident.
>>
>> Roman
>>
>>> Thanks,
>>> David
>>> -----
>>>
>>>> Roman
>>>>
>>>>
>>>> Am 17. April 2019 18:59:09 MESZ schrieb Roman Kennke 
>>>> <rkennke at redhat.com>:
>>>>
>>>>             Various code paths in oopDesc, Klass and their 
>>>> subclasses assert
>>>>             something that fetches the object's _klass field. With 
>>>> upcoming
>>>>             Shenandoah's changes this is not always safe and 
>>>> requires an
>>>>             additional
>>>>             indirection.
>>>>
>>>>             The trouble here is that we can, for example, call
>>>>             Klass::oop_oop_iterate() with a pre-resolved Klass*, 
>>>> instead of
>>>>             oopDesc::oop_iterate() which would call oopDesc::klass() on
>>>>             its own,
>>>>             which would be racy on some GC internal call paths, but 
>>>> we can't
>>>>             (currently) control some calls to klass() further down the
>>>>             call stack
>>>>             (all in asserts).
>>>>
>>>>             We'd also like a way to ensure that non-GC calls to klass()
>>>>             are sane.
>>>>
>>>>             Bug:
>>>>             https://bugs.openjdk.java.net/browse/JDK-8222545
>>>>             Webrev:
>>>>             http://cr.openjdk.java.net/~rkennke/JDK-8222545/webrev.00/
>>>>             Testing:
>>>>             hotspot_gc_shenandoah with and without the prototype,
>>>>             hotspot/tier1
>>>>
>>>>             The change introduces only two ASSERT-level GC-interfaces,
>>>>             and afaict,
>>>>             this with JDK-8222537 will be all that we need for the 
>>>> upcoming
>>>>             elimination of forward pointers in Shenandoah. Notice that
>>>>             one assert in
>>>>             objArrayKlass is strengthened from is_array() to
>>>>             is_objArray(), but that
>>>>             seems only sane in that context.
>>>>
>>>>             Can I please get reviews?
>>>>
>>>>
>>>>         This looks very awkward to me. Using:
>>>>
>>>>         Universe::heap()->safe_klass(obj)->is_objArray_klass()
>>>>
>>>>         instead of the obvious:
>>>>
>>>>         obj->is_objArray()
>>>>
>>>>         is very unintuitive. Can this not be handled inside is_objArray
>>>>         (and
>>>>         is_typeArray) ?
>>>>
>>>>
>>>>     Not really. Then it would get exposed to many more code paths, 
>>>> most of
>>>>     which don't actually need it/don't want it, and many of which are
>>>>     outside of asserts, and rely on the usual klass() with the 
>>>> sanity assert
>>>>     there instead. I am open for suggestions, but it would have to be
>>>>     restricted to ASSERT code IMO, and ideally with as few as 
>>>> possible GC
>>>>     interface additions.
>>>>
>>>>     Roman
>>>>
>>>>
>>>> -- 
>>>> Diese Nachricht wurde von meinem Android-Gerät mit K-9 Mail gesendet.