[RFR] Java memory instrumentation for TSAN

[Jean Christophe Beyler]

Hi all,

Sorry I'm late at the party:

http://cr.openjdk.java.net/~aeubanks/tsanjavamem/webrev.02/src/hotspot/cpu/x86/templateTable_x86.hpp.udiff.html
that an array has ben
  -> been

http://cr.openjdk.java.net/~aeubanks/tsanjavamem/webrev.02/src/hotspot/share/runtime/arguments.cpp.udiff.html
  -> down the pplication
    -> down the application

http://cr.openjdk.java.net/~aeubanks/tsanjavamem/webrev.02/src/hotspot/cpu/x86/templateTable_x86.cpp.udiff.html
  -> Label Done
    - Label done

Apart from that, it looks good to me, no need of a new webrev :)
Jc

On Wed, May 29, 2019 at 8:56 AM Man Cao <manc at google.com> wrote:

> Looks good.
>
> -Man
>
>
> On Tue, May 28, 2019 at 1:03 PM Arthur Eubanks <aeubanks at google.com>
> wrote:
>
> > On Fri, May 24, 2019 at 2:20 PM Man Cao <manc at google.com> wrote:
> >
> >> We can add the "ThreadSanitizerJavaMem" flag, so it is consistent with
> >> our internal implementation.
> >>
> > Added a new flag named "ThreadSanitizerJavaMemory" (no point in
> shortening
> > the name by 3 chars).
> >
> >> We can set ThreadSanitizerJavaMem's default value to true here, because
> >> all the uses of this flag should be guarded by "if (ThreadSanitizer)".
> >>
> >> Only the following places need to be changed to
> >> use ThreadSanitizerJavaMem:
> >> In sharedRuntime.cpp:
> >> #define TSAN_MEMORY_ACCESS(name)
> >> ...
> >> assert(ThreadSanitizer, "Need -XX:+ThreadSanitizer");
> >>
> > Done (asserted both ThreadSanitizer and ThreadSanitizerJavaMem).
> >
> >>
> >> In templateTable_x86.cpp:
> >> TemplateTable::tsan_observe_get_or_put()
> >> ....
> >>  if (ThreadSanitizer) {
> >>   __ jmp(Done);
> >>
> > Done.
> >
> >>
> >> TemplateTable::tsan_observe_load_or_store()
> >> It can add an assert for ThreadSanitizer, and check
> >> for ThreadSanitizerJavaMem instead.
> >>
> > Done.
> >
> >>
> >> Other minor issues:
> >> In arguments.cpp, can we change it to:
> >> TSAN_RUNTIME_ONLY(
> >>   set_mode_flags..
> >>   FLAG_SET_ERGO..
> >>   FLAG_SET_ERGO..
> >> );
> >>
> > Done. I didn't want to do this initially because I think it's ugly to
> have
> > a semicolon in the statements and at the end of the macro, but oh well.
> >
> >>
> >> In instanceKlass.cpp:
> >> Can we move "#if INCLUDE_TSAN \n #include "runtime/sharedRuntime.hpp""to
> >> the end of the include statements, so to consistent with other
> conditional
> >> includes?
> >>
> > Done.
> >
> >>
> >> In templateTable_x86.cpp:
> >> In TemplateTable::tsan_release_acquire_method()
> >>  785   ShouldNotReachHere();
> >>  786   return NULL;
> >>
> >> Can we remove "return NULL"? Or would it trigger a compiler warning?
> >>
> > Yeah it triggers a compiler warning.
> >
> >>
> >> In TemplateTable::load_field_cp_cache_entry():
> >> 2883 #ifdef INCLUDE_TSAN
> >> 2884 if (ThreadSanitizer) {
> >> 2885  // Draw a happens-before edge from the class's static initializer
> to
> >> 2886  // this lookup.
> >>
> >> This should use TSAN_RUNTIME_ONLY instead.
> >>
> >> -Man
> >>
> >
> > also added a test to check that there are no races when
> > ThreadSanitizerJavaMemory is turned off.
> >
> > new webrev: http://cr.openjdk.java.net/~aeubanks/tsanjavamem/webrev.02/
> >
>


-- 

Thanks,
Jc