RFR: 8069016: Add BarrierSet downcast support

[Jon Masamitsu]

http://cr.openjdk.java.net/~kbarrett/8069016/webrev.00/src/share/vm/utilities/fakeRttiSupport.hpp.html

   86   TagType _ctag;

I'd prefer that the variable be named _concrete_tag

or

the parameters concrete_tag be changed to ctag

   56   explicit FakeRttiSupport(TagType concrete_tag) :
   57     _tag_set(tag_bit(concrete_tag)), _ctag(concrete_tag) { }

   62   FakeRttiSupport(TagType concrete_tag, uintx tag_set) :
   63     _tag_set(tag_set), _ctag(validate_tag(concrete_tag)) { }

and function concrete_tag() be changed to ctag().

http://cr.openjdk.java.net/~kbarrett/8069016/webrev.00/src/share/vm/memory/barrierSet.hpp.frames.html

   82   // Test whether this object is of the type corresponding to bsn.
   83   bool is_a(BarrierSet::Name bsn) const { return _fake_rtti.has_tag(bsn)


For "is_a()" would a more accurate description be

// Check if the BarrierSet::Name has a tag in the tag_set

http://cr.openjdk.java.net/~kbarrett/8069016/webrev.00/src/share/vm/gc_implementation/parallelScavenge/cardTableExtension.hpp.udiff.html

+      // Concrete tag should be BarrierSet::CardTableExtension.
+      // That will presently break things in a bunch of places though.

Can you expand on these comments.  Where would the breakage occur?


http://cr.openjdk.java.net/~kbarrett/8069016/webrev.00/src/share/vm/gc_implementation/parallelScavenge/psMarkSweep.cpp.udiff.html

-    BarrierSet* bs = heap->barrier_set();
-    if (bs->is_a(BarrierSet::ModRef)) {
-      ModRefBarrierSet* modBS = (ModRefBarrierSet*)bs;
+    ModRefBarrierSet* modBS = barrier_set_cast<ModRefBarrierSet>(heap->barrier_set());


Is this one of the place that breaks because the barrier set is really a 
CardTableExtension?

http://cr.openjdk.java.net/~kbarrett/8069016/webrev.00/src/share/vm/memory/cardTableModRefBS.hpp.udiff.html

+      // Concrete tag should be BarrierSet::CardTableForRS.
+      // That will presently break things in a bunch of places though.


Expand on this comment also about what will break.

Rest looks good.

Jon


On 2/16/2015 3:16 PM, Kim Barrett wrote:
> Please review this change that ensures all barrier set downcasts are
> checked when running in debug mode. This is a step toward some
> cleanups and changes to the barrier set class hierarchy, improving
> error checking and making it easier to find places where assumptions
> are being made that would be invalidated by changes.
>
> I will need a sponsor for this change.
>
> CR:
> https://bugs.openjdk.java.net/browse/JDK-8069016
>
> Webrev:
> http://cr.openjdk.java.net/~kbarrett/8069016/webrev.00
>
> Changeset includes
>
> * New FakeRttiSupport utility class template.  This provides some
>    infrastructure for performing the checks. It is intended to
>    ease similar changes in some other class hierarchies where ad hoc
>    checking or unchecked downcasts are common, such as the
>    CollectedHeap hierarchy.
>
> * Re-implementation of BarrierSet's "fake RTTI" to use
>    FakeRttiSupport.  As part of this, BarrierSet::is_a() is now just an
>    integer bit test rather than a virtual function that needs to walk
>    up the class hierarchy.
>
> * Adds barrier_set_cast function template, which uses the improved
>    is_a() test to assert the validity of the conversion.
>
> * Adds BarrierSet::CardTableForRS barrier set name tag, so that all
>    existing classes have an associated tag.
>
> * Changes all C-style barrier set downcasts to use barrier_set_cast.  In
>    most cases, this is just a mechanical translation.  In some cases some
>    existing checks have been eliminated in favor of the check made by the
>    new mechanism.  In a few places more extensive changes have been made,
>    as detailed below:
>
> ** In various g1_write_barrier_post() implementations, downcast to
>     G1SATBCardTableLoggingModRefBS rather than G1SATBCardTableModRefBS,
>     since the code being generated includes logging.
>
> ** In PSMarkSweep::invoke_no_policy(), don't bother conditionalizing
>     clear/invalidate of barrier set on whether the bs is a modrefbs; it
>     always will be for parallel scavenge collector.  Similarly in
>     PSParallelCompact::post_compact().
>
> ** For VerifyLiveClosure constructor initialization of _bs member, just
>     use barrier_set_cast of the heap's barrier. If the previous is_a
>     conditionalization failed it would just lead to a later segfault
>     when attempting to dereference through the NULL _bs member.
>
> Testing:
> JPRT, local JTREG of test/[closed]/{gc,runtime},
> local RefWorkload using CMS, G1, ParallelOld
>