RFR: JDK-8211279: Verify missing object equals barriers

[Roman Kennke]

Thanks for reviewing, Zhengyu!

Roman

> Looks good to me.
> 
> Thanks,
> 
> -Zhengyu
> 
> On 10/03/2018 09:25 AM, Roman Kennke wrote:
>> Hi Per,
>>
>>> On 10/01/2018 02:48 PM, Roman Kennke wrote:
>>>> Hi Per,
>>>>
>>>>
>>>>>> GCs like Shenandoah require an extra barrier for comparing objects
>>>>>> (oop==oop). It is easy to forget or overlook this. It would be very
>>>>>> useful to have a flag to turn on extra verification that catches
>>>>>> missing
>>>>>> object equality barriers.
>>>>>>
>>>>>> This change inserts an assert into == and != operators for the oop
>>>>>> class
>>>>>> in oopsHierarchy.hpp. This only gets compiled in fastdebug builds
>>>>>> (when
>>>>>> CHECK_UNHANDLED_OOPS is on).
>>>>>>
>>>>>> It also adds a develop flag VerifyObjectEquals that is used to
>>>>>> turn on
>>>>>> this verification.
>>>>>>
>>>>>> It also adds a method oopDesc::unsafe_equals(..) to be used in cases
>>>>>> where you know what what you are doing, and really want to use
>>>>>> direct ==
>>>>>> comparison without using barriers. This is used in e.g.
>>>>>> ReferenceProcessor or all over the place in ShenandoahGC.
>>>>>>
>>>>>> The change also fixes a couple of places where oops are compared to
>>>>>> non-oops like Universe::non_oop_word() to use the oop==void* operator
>>>>>> instead, so those don't falsely trip the verification.
>>>>>>
>>>>>> It doesn't make sense to turn this check on if you're not running
>>>>>> a GC
>>>>>> that needs it, unless you want to go fix all the oop==oop in the GC
>>>>>> itself.
>>>>>>
>>>>>> Bug:
>>>>>> https://bugs.openjdk.java.net/browse/JDK-8211279
>>>>>> Webrev:
>>>>>> http://cr.openjdk.java.net/~rkennke/JDK-8211279/webrev.00/
>>>>>>
>>>>>> What do you think?
>>>>>
>>>>> So this means we would have a verification option that, when enabled,
>>>>> always crashes the VM unless you run Shenandoah? That doesn't sound
>>>>> quite right to me. This should just be a noop when not using
>>>>> Shenandoah,
>>>>> don't you think?
>>>>
>>>>
>>>> Hmm, right. Let's add some BarrierSet-infrastructure to handle this,
>>>> and
>>>> remove the option (it would be a GC-'private' option). It would
>>>> probably
>>>> have looked slightly better to do this in BarrierSet::Access, next to
>>>> the Access::equals() API, but I don't feel like adding tons of
>>>> boilerplate just to add this. (Infact, this is a big red warning signal
>>>> regarding the Access API...)
>>>>
>>>> http://cr.openjdk.java.net/~rkennke/JDK-8211279/webrev.01/
>>>>
>>>> How does this look now?
>>>
>>>
>>> src/hotspot/share/oops/oop.hpp
>>> ------------------------------
>>>
>>>   157   inline static bool unsafe_equals(oop o1, oop o2) {
>>>   158     return (void*) o1 == (void*) o2;
>>>   159   }
>>>
>>> I think this should be called oopDesc::equals_raw() to follow the naming
>>> convention we have for these types of functions. Also, it should do:
>>>
>>>    return RawAccess<>::equals(o1, o2);
>>>
>>> Also, please make it a one-liner to better match the look of
>>> oopDesc::equals().
>>>
>>>
>>> src/hotspot/share/gc/shared/referenceProcessor.cpp
>>> --------------------------------------------------
>>>
>>>   477   while (! oopDesc::unsafe_equals(next, obj)) {
>>>
>>> Stray white-space, please remove.
>>>
>>>
>>> src/hotspot/share/gc/shared/referenceProcessor.hpp
>>> --------------------------------------------------
>>>
>>>   152     assert(! oopDesc::unsafe_equals(_current_discovered,
>>> _first_seen), "cyclic ref_list found");
>>>
>>> Stray white-space, please remove.
>>>
>>>
>>> src/hotspot/share/oops/accessBackend.hpp
>>> ----------------------------------------
>>>
>>>   413   static bool equals(oop o1, oop o2) { return (void*) o1 ==
>>> (void*)
>>> o2; }
>>>
>>> Stray white-spaces, please make that "(void*)o1 == (void*)o2".
>>>
>>>
>>> src/hotspot/share/gc/shared/barrierSet.hpp
>>> ------------------------------------------
>>>
>>>   134   virtual void verify_equals(oop o1, oop o2) { }
>>>
>>> I'm thinking this should be:
>>>
>>>    virtual bool oop_equals_operator_allowed() { return true; }
>>>
>>> And let oop::operator==(...) do:
>>>
>>>    assert(BarrierSet::barrier_set()->oop_equals_operator_allowed(), "Not
>>> allowed");
>>>
>>>
>>> Erik, can you live with this, or do you have any better ideas here?
>>> I'm not ecstatic about having a new function on BarrierSet just for
>>> this. Should we just make oop::operator==() private and fix all the
>>> places where it's used? One could also argue the oop::operator==() _is_
>>> the raw equals and that we should be allowed to use it. Any other ideas?
>>
>>
>> This addresses all the issues mentioned above:
>> http://cr.openjdk.java.net/~rkennke/JDK-8211279/webrev.02/
>>
>> It does not implement Erik's suggestion, because I think it's inferior.
>> I'm open to discussions though.
>>
>> Roman
>>

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: OpenPGP digital signature
URL: <https://mail.openjdk.org/pipermail/hotspot-gc-dev/attachments/20181010/ce020b26/signature.asc>