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

[David Holmes]

Hi Sangheon,

On 3/02/2017 2:00 PM, 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().

Thanks for explaining. I must say that I find this treatment rather 
ad-hoc as we seem to be discovering by trial-and-error where these 
places occur. I would have hoped this was all encapsulated within the 
weakreference code itself (in this case). Does the SATB occur at a 
global safepoint?

David

> I added this comment at the patch.
> Webrev: http://cr.openjdk.java.net/~sangheki/8173013/webrev.1
>
> Thanks,
> Sangheon
>
>
>>
>> Thanks,
>> David
>>
>>> As writing stable test could take some more time, Stefan Karlsson and I
>>> did some tests to provoke this problem.
>>> ( Stefan Karlsson kindly provided the test,
>>> http://cr.openjdk.java.net/~stefank/8173013/reproducer/
>>> <http://cr.openjdk.java.net/%7Estefank/8173013/reproducer/> )
>>> With this proposed patch, the problem goes away.
>>>
>>> CR: https://bugs.openjdk.java.net/browse/JDK-8173013
>>> Webrev: http://cr.openjdk.java.net/~sangheki/8173013/webrev.0
>>> Testing: JPRT, some closed tests which use JVMTI and JDI.
>>>
>>> Thanks,
>>> Sangheon
>>>
>