RFR(S): 8173013: JVMTI tagged object access needs G1 pre-barrier

[Thomas Schatzl]

Hi Sangheon,

On Thu, 2017-02-02 at 20:00 -0800, sangheon wrote:
> Hi David,
> 
> Thank you for looking at this!
> 
> On 02/02/2017 05:20 PM, David Holmes wrote:
> > 
> > Hi Sangheon,
> > 
> > On 3/02/2017 5:11 AM, sangheon wrote:
> > > 
> > > Hi all,
> > > 
> > > Could I have some reviews for this change that adds G1 pre-
> > > barrier?
> > > 
> > > JvmtiTagHashmapEntry has a bare oop member and this is a weak
> > > reference.
> > > So any place that allows this oop to 'escape' needs the G1 pre-
> > > barrier.
> > > JvmtiEnv::GetObjectsWithTags() provides such an escape path.
> > > 
> > > For G1, in order to maintain the SATB invariants, reading the
> > > referent
> > > of a weak reference must ensure the referent is marked alive.
> > > 
> > > So this proposal includes adding the pre-barrier at
> > > TagObjectCollector::do_entry(JvmtiTagHashmapEntry* entry) which I
> > > see
> > > the only place interacts(except 'peek' operations) with the bare
> > > oop 
> > > member.
> > Pardon my GC ignorance but it seems odd to me that this barrier is 
> > inserted immediately before we create a local JNIHandle. Won't the 
> > JNIHandle ensure the object is seen as live?
> Unfortunately it isn't.
> 
> If we are using G1 and accessing the value of the referent field in a
> reference object then we need to register a non-null referent with
> the SATB barrier.
> 
> In this code path, for example:
> 
> (1) Object x is tagged.
> (2) x becomes unreachable, with the only remaining reference being
> the weak reference in the tag hashmap.
> (3) Concurrent GC starts and has completed marking, but has not yet 
> entered the remark pause where reference processing occurs.
> (4) GetObjectsWithTags is used to obtain x. As reference processing
> has not yet run, x is still present in the tag hashmap. x remains
> unmarked, because there is no read(SATB) barrier on that access.
> (5) GC remark pause runs reference processing, determines x is dead,
> so clears the tag entry, and reuses the space previously occupied by
> x.
> (6) The GetObjectsWithTags result now refers to a dead and reclaimed
> x. 
> A crash may follow.
> (From Kim Barrett's note)
> 
> FYI, there are similar treatments already for this case.
> Plz, search for "G1SATBCardTableModRefBS::enqueue", especially 
> Unsafe_GetObject().
> 
> I added this comment at the patch.
> Webrev: http://cr.openjdk.java.net/~sangheki/8173013/webrev.1

1539         // We could be accessing the referent field in a reference
1540         // object. If G1 is enabled then we need to register non-
null
1541         // referent with the SATB barrier.

I do not think this comment is complete, or at least I do not see why
the object in question needs to be a "referent field in a reference".
Maybe you meant that the tag map entry acts similar to the referent of
a j.l.ref.WeakReference, so we need to handle it the same as when a
thread accesses a weak reference in normal java code (i.e. the read
barrier).

As the problem description above indicates, this can occur with any
reference to an object where the tag map has the only (implicitly weak)
reference to as far as I can see.

I.e. maybe something like:

"The reference in this tag map could be the only (implicitly weak)
reference to that object. If we hand it out we need to keep it live wrt
SATB marking similar to other j.l.ref.Reference referents."

Probably others can provide a better description.

Otherwise it seems good.

esses 

Thanks,
  Thomas