RFR: JDK-8211279: Verify missing object equals barriers

Roman Kennke rkennke at redhat.com
Wed Oct 3 12:04:30 UTC 2018


The problem is that it would get false positive *in most cases*. With
the solution that I proposed, one would do a run on purpose to catch
missing eq barriers, and find all the relevant places *before it
actually blows up*. The solution you propose would lead to rare
headscratching failures instead. I'd rather prefer the explicit
flag-controlled verification.

Roman


Am 03.10.18 um 11:23 schrieb Erik Österlund:
> 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
>>>
> 

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: OpenPGP digital signature
URL: <http://mail.openjdk.java.net/pipermail/hotspot-gc-dev/attachments/20181003/006cec7e/signature.asc>


More information about the hotspot-gc-dev mailing list