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

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 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? cheers, Per It still passes hotspot/jtreg:tier1 here. Thanks for looking at this! Roman

More information about the hotspot-runtime-dev mailing list