RFR: JDK-8222545: Safe klass asserts

[Per Liden]

Hi Roman,

On 4/18/19 3:13 PM, Roman Kennke wrote:
>>>>> 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.

Ok, so the only thing you really want to do it not assert in
ObjArrayKlass::oop_size() and TypeArrayKlass::oop_size(), correct? Given 
that oopDesc::size_given_klass() exists to solve these kind of 
concurrency issues, I would have understood if your patch just removed 
the offending asserts, with the argument that you can't make these kind 
of asserts/assumptions there.

In my view, that would be much easier to swallow, compared to adding 
clue to CollectedHeap to try to workaround the asserts.

cheers,
Per

> 
> 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.