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

[sangheon]

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

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
>>