RFR: JDK-8211279: Verify missing object equals barriers

[Zhengyu Gu]

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
>