RFR: JDK-8211279: Verify missing object equals barriers

[Roman Kennke]

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