RFR: JDK-8211279: Verify missing object equals barriers

Wed Oct 3 10:21:01 UTC 2018

Hi,

On 2018-10-03 11:23, Erik Österlund wrote:
> Hi Per and Roman,
> 
> My favourite solution to this problem would be to in the oop::operator() 
> definition assert that RawAccess<>::equals(o1, o2) == 
> Access<>::equals(o1, o2).
> This verification could have false positives where it considers things 
> safe that are in fact unsafe at runtime. On the other hand, the already 
> proposed verification could have false negatives, where a completely 
> valid == is considered not safe, despite being safe. For example if the 
> operands have already been Access<>::resolved, then it is safe to use == 
> to compare them.
> 
> In practice, I would be surprised if my proposed solution did not 
> immediately hit the assert during testing when the usage is wrong. And 
> every time it hits the assert, it would be due to provably wrong usage 
> of ==.
> 
> As a benefit, we do not have to clutter the shared barrier set with a 
> function that essentially says if (UseShenandoah) ShouldNotReachHere();
> 
> What do you think?

That would be a nicely GC agnostic and much less intrusive approach, 
which turns all this into a one-liner fix. I like it.

cheers,
Per

> 
> Thanks,
> /Erik
> 
> On 2018-10-03 10:42, Per Liden wrote:
>> 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
>>>
> 