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


Reply via email to