[RFR] Instrumentation for basic Unsafe put/get

[Jean Christophe Beyler]

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.

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