[RFR] Instrumentation for Unsafe volatile put/get

[Man Cao]

Looks good. No need for new webrev for following nits:

Is this change necessary?

-  // Default constructors will call this-  AbstractLoop() {++  public
AbstractLoop() {

I'm not a fan of the names "runInTwoThreadsSync" and "syncSetup". There is
no obvious extra synchronization compared to runInTwoThreads().
Maybe "runOnceInTwoThreads" and "runOnceSetup" are better?
Another option might be to create a subclass of AbstractLoop, which
overrides runInTwoThreads(). But I don't insist on adding the subclass and
it's up to you.
There could be comments for "runInTwoThreads" and "runOnceInTwoThreads" to
document their differences.

-Man


On Fri, Sep 20, 2019 at 1:15 PM Arthur Eubanks <aeubanks at google.com> wrote:

> New webrev:
> http://cr.openjdk.java.net/~aeubanks/tsanvolatileunsafe/webrev.01/index.html
>
> On Fri, Sep 20, 2019 at 1:12 PM Arthur Eubanks <aeubanks at google.com>
> wrote:
>
>>
>>
>> On Fri, Sep 20, 2019 at 11:10 AM Man Cao <manc at google.com> wrote:
>>
>>> In unsafe.cpp:
>>> Calls to __tsan_java_acquire and __tsan_java_release should be swapped.
>>> I.e., Get*Volatile should use __tsan_java_acquire, and Put*Volatile should
>>> use __tsan_java_release.
>>>
>> Oof sorry, bad mistake, fixed. Writing the tests below actually helped
>> catch that.
>>
>>>
>>> I think the tests should check that proper use of volatile establishes
>>> happens-before, instead of "no race on volatile access". E.g.:
>>>
>>> volatile byte b = 0;
>>> int data = 0;
>>> T1:
>>> data = 42;
>>> b = 1;
>>>
>>> T2:
>>> while (b == 0);
>>> int t = data;
>>>
>> Done. Added some functionality to AbstractLoop.java to properly do this.
>>
>>>
>>> In NonRacyUnsafeVolatileLoopTest.java:
>>>
>>> private byte b = 0x42;
>>>
>>> Should this field be "volatile"?
>>>
>> I don't believe these need to be volatile, just the access itself needs
>> to be volatile. volatile is a property of a specific memory access, not a
>> field. Martin agrees.
>>
>>> -Man
>>>
>>>
>>>
>>> On Fri, Sep 20, 2019 at 8:40 AM Arthur Eubanks <aeubanks at google.com>
>>> wrote:
>>>
>>>> webrev:
>>>>
>>>> http://cr.openjdk.java.net/~aeubanks/tsanvolatileunsafe/webrev.00/index.html
>>>>
>>>> I also added a test that normal volatile accesses (not through unsafe)
>>>> are
>>>> not considered racy, since we didn't have a test for that before.
>>>>
>>>