RFR (S): 8223177: Data race on JvmtiEnvBase::_tag_map in double-checked locking

[David Holmes]

On 2/05/2019 6:25 am, Man Cao wrote:
> I have moved the set_tag_map back to the constructor:
> https://cr.openjdk.java.net/~manc/8223177/webrev.01/

Looks good - thanks. I doubt there will be any additional use of this 
constructor.

David
-----

> -Man
> 
> 
> On Wed, May 1, 2019 at 11:05 AM Man Cao <manc at google.com 
> <mailto:manc at google.com>> wrote:
> 
>     Thanks for the review.
>     I moved set_tag_map out of the constructor because the release store
>     is only required in the double-checked locking pattern.
>     If the constructor is called in a single-threaded context, or if
>     _tag_map is always protected by a lock, then it could use the normal
>     store instead.
>     Currently it doesn't matter since the constructor is only called
>     inside the double-checked locking.
>     I'm OK either way. Do you prefer to keep it inside the constructor?
> 
>     -Man
> 
> 
>     On Wed, May 1, 2019 at 4:02 AM David Holmes <david.holmes at oracle.com
>     <mailto:david.holmes at oracle.com>> wrote:
> 
>         Hi Man,
> 
>         On 1/05/2019 11:51 am, Man Cao wrote:
>          > Hi all,
>          >
>          > Can I have reviews for this small change that adds memory
>         fences for
>          > double-checked locking?
>          > We found this race while working on the Java ThreadSanitizer
>         project.
>          >
>          > Webrev: https://cr.openjdk.java.net/~manc/8223177/webrev.00/
>          > Bug: https://bugs.openjdk.java.net/browse/JDK-8223177
> 
>         Looks fine. One query in jvmtiTagMap.cpp - Was there a
>         particular reason
>         you moved the set_tag_map out of the constructor? (It's a common
>         pattern
>         when objects are bi-directionally linked to do it in the
>         constructor.)
> 
>         Thanks,
>         David
> 
>          > -Man
>