RFR: JDK-8211279: Verify missing object equals barriers

Erik Österlund erik.osterlund at oracle.com
Wed Oct 3 09:23:03 UTC 2018


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?

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
>>



More information about the hotspot-runtime-dev mailing list