[RFR] Instrumentation for basic Unsafe put/get

[Man Cao]

Looks good. Thanks for improving the tests!

-Man


On Thu, Sep 19, 2019 at 9:52 AM Arthur Eubanks <aeubanks at google.com> wrote:

> New webrev:
> http://cr.openjdk.java.net/~aeubanks/tsannormalunsafe/webrev.01/index.html
>
> On Wed, Sep 18, 2019 at 4:34 PM Man Cao <manc at google.com> wrote:
>
>> For Unsafe_GetReference/Unsafe_PutReference, do you really need the
>> backslashes "\" for code within TSAN_RUNTIME_ONLY?
>>
>> In Unsafe_GetReference:
>> void* addr = index_oop_from_field_offset_long(JNIHandles::resolve(obj),
>> offset);
>> "JNIHandles::resolve(obj)" could be replaced with "p", right?
>>
> Done
>
>>
>> For RacyUnsafe*Test, the stack trace check could be a bit more relaxed to
>> avoid flaky tests. E.g. in RacyUnsafePutIntLoopTest.java:
>> .shouldContain(" #1 sun.misc.Unsafe.getInt(Ljava/lang/Object;J)I
>> Unsafe.java:");
>> could become:
>> .shouldMatch(" #1 sun.misc.Unsafe.(get|put)Int\(Ljava/lang/Object;JI?)I?
>> Unsafe.java:");
>>
> I separated put and get into their own tests. I also made run()
> synchronized, and did the normal read/write in run2() synchronized, so the
> only racy access is the Unsafe access in run2() and the accesses in run().
>
>>
>>
>> In RacyUnsafePutStringLoopTest.java, Character.toString(c) may not create
>> new String object or change the reference value of the field x.
>> Can we update the value of the character as in the original test, so it
>> changes? I.e. char c = (char) (x.charAt(0) + 1);
>>
> I don't think it matters, right? Anyway, changed it.
>
>>
>>
>> Nits:
>>
>>  337 UNSAFE_ENTRY(void, Unsafe_Put##Type(JNIEnv *env, jobject unsafe, jobject obj, jlong offset, java_type x)) { \ 338   TSAN_RUNTIME_ONLY( \ 339   void* addr = index_oop_from_field_offset_long(JNIHandles::resolve(obj), offset); \ 340     __tsan_write##size##_pc(addr, SharedRuntime::tsan_code_location(0, 0)) \ 341   ); \
>>
>> Missing a ";" after __tsan_write##size##_pc.
>> Indention for "void* addr = ..." is off.
>>
> Done
>
>>
>> In AbstractLoop.java:
>> * The second thread calls this method, defaults to calling run().
>> I think the original comments for run() and run2() are better:
>> For run(): Implement only this method for symmetric behavior.
>> For run2(): Override this method for asymmetric behavior.
>>
> Done
>
>>
>> -Man
>>
>>
>> On Tue, Sep 17, 2019 at 9:37 AM Arthur Eubanks <aeubanks at google.com>
>> wrote:
>>
>>> Forgot to mention:
>>> This does slow down usage of Unsafe in the interpreter when the TSAN
>>> feature is enabled at compile time, but since the interpreter is so slow,
>>> it doesn't matter anyway. C1/C2 have their own implementation of Unsafe,
>>> so
>>> those should not be affected.
>>>
>>> On Tue, Sep 17, 2019 at 9:34 AM Arthur Eubanks <aeubanks at google.com>
>>> wrote:
>>>
>>> > webrev:
>>> >
>>> http://cr.openjdk.java.net/~aeubanks/tsannormalunsafe/webrev.00/index.html
>>> >
>>> > This change adds instrumentation for the interpreter Unsafe
>>> > implementation, only for Unsafe_{Get,Put}{Reference,primitive}. Other
>>> > Unsafe instrumentation will come later.
>>> >
>>
>>