RFR: JDK-8222545: Safe klass asserts

[David Holmes]

On 18/04/2019 7:54 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.


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.