[RFR] Instrumentation for basic Unsafe put/get

[Arthur Eubanks]

On Thu, Sep 19, 2019 at 11:56 AM Jean Christophe Beyler <jcbeyler at google.com>
wrote:

> Hi Arthur,
>
> Tiny nit:
>
> http://cr.openjdk.java.net/~aeubanks/tsannormalunsafe/webrev.01/src/hotspot/share/prims/unsafe.cpp.udiff.html
>
> There is a:
> +    } else { \
>
> that is not needed in Unsafe_GetReference.
>
Done and pushed.

>
> Apart from that : LGTM && great job :),
> Jc
>
>
>
> On Thu, Sep 19, 2019 at 11:19 AM Man Cao <manc at google.com> wrote:
>
>> 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.
>> >>> >
>> >>
>> >>
>>
>
>
> --
>
> Thanks,
> Jc
>