RFR: JDK-8211279: Verify missing object equals barriers (original) (raw)

Zhengyu Gu zgu at redhat.com
Tue Oct 9 22:51:41 UTC 2018

Looks good to me.

Thanks,

-Zhengyu

On 10/03/2018 09:25 AM, Roman Kennke wrote:

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 CHECKUNHANDLEDOOPS is on). It also adds a develop flag VerifyObjectEquals that is used to turn on this verification. It also adds a method oopDesc::unsafeequals(..) 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::nonoopword() 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 unsafeequals(oop o1, oop o2) {  158     return (void*) o1 == (void*) o2;  159   } I think this should be called oopDesc::equalsraw() 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::unsafeequals(next, obj)) { Stray white-space, please remove. src/hotspot/share/gc/shared/referenceProcessor.hpp --------------------------------------------------  152     assert(! oopDesc::unsafeequals(currentdiscovered, firstseen), "cyclic reflist 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 verifyequals(oop o1, oop o2) { } I'm thinking this should be: virtual bool oopequalsoperatorallowed() { return true; } And let oop::operator==(...) do: assert(BarrierSet::barrierset()->oopequalsoperatorallowed(), "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

More information about the hotspot-runtime-dev mailing list