[RFR] Instrumentation for basic Unsafe put/get

[Arthur Eubanks]

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