RFR: JDK-8222545: Safe klass asserts

[Per Liden]

Hi Roman,

On 4/18/19 1:45 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?

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.