[RFR] Instrumentation for Unsafe volatile put/get

[Arthur Eubanks]

On Fri, Sep 20, 2019 at 5:34 PM Man Cao <manc at google.com> wrote:

> Looks good. No need for new webrev for following nits:
>
> Is this change necessary?
>
> -  // Default constructors will call this-  AbstractLoop() {++  public AbstractLoop() {
>
> I don't see why we should restrict it to a specific package.

> I'm not a fan of the names "runInTwoThreadsSync" and "syncSetup". There is
> no obvious extra synchronization compared to runInTwoThreads().
>
It's equivalent to adding a barrier at the end of each iteration in each
thread (I think).

> Maybe "runOnceInTwoThreads" and "runOnceSetup" are better?
>
I initially sort of liked this, but "once" is clearly not right, the whole
point of the setup is that it's run multiple times. I think I'll keep the
name for now. If somebody wants to change the name in the future, go for it.

> 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.
>
That seems very confusing. I think separating it out into two different
methods is clearer.

> There could be comments for "runInTwoThreads" and "runOnceInTwoThreads" to
> document their differences.
>
Done.

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