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