RFR: JDK-8211279: Verify missing object equals barriers

[Per Liden]

Hi Roman,

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?


cheers,
Per

> 
> It still passes hotspot/jtreg:tier1 here.
> 
> Thanks for looking at this!
> Roman
>