[RFR] Java memory instrumentation for TSAN

[Man Cao]

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