[RFR]: Lock instrumentation

[Man Cao]

Great to hear that the test works now.

For the macro, I think as long as the use of the macro looks like a scope
(like JRT_LEAF / JRT_END), or a function call (like JFR_ONLY), the style
should be OK.

-Man


On Wed, Apr 24, 2019 at 10:45 AM Jean Christophe Beyler <jcbeyler at google.com>
wrote:

> Yeah for the test, I'm actually adding that macro now in the JVMTI patch
> so we can see what it looks and decide :), then I can refactor the other
> changes to the same style in a later webrev,
> Jc
>
> On Wed, Apr 24, 2019 at 10:37 AM Arthur Eubanks <aeubanks at google.com>
> wrote:
>
>> With JC's fix to the tests and
>> the ReleaseJavaMonitorsClosure::do_monitor() fix, now all the tests pass!
>>
>> I'm mixed on the TSAN_RUNTIME_ONLY() suggestion, but either way it can
>> come in a later change.
>>
>> On Tue, Apr 23, 2019 at 1:23 PM Man Cao <manc at google.com> wrote:
>>
>>> In synchronizer.cpp, in ReleaseJavaMonitorsClosure::do_monitor(), the
>>> TSAN instrumentation should be inside the code block of the conditional "if
>>> (mid->owner() == THREAD) {".
>>> Otherwise looks good. No need for a new webrev after fixing this.
>>>
>>> Regarding the "#if INCLUDE_TSAN" macro, I think we could define a macro
>>> in macros.hpp like the following:
>>> #if INCLUDE_TSAN
>>> #define TSAN_RUNTIME_ONLY(code) if (ThreadSanitizer) { code }
>>> #else
>>> #define TSAN_RUNTIME_ONLY(code)
>>> #endif
>>>
>>> Then it could simplify some (but not all) of the instrumentation.
>>>
>>> -Man
>>>
>>>
>>> On Tue, Apr 23, 2019 at 9:49 AM Jean Christophe Beyler <
>>> jcbeyler at google.com> wrote:
>>>
>>>> Hi Arthur,
>>>>
>>>> Looks good to me, I was looking at if we could factorize a bit the
>>>> assertions and what-not but for now it's fine.
>>>>
>>>> (I did notice that we let creep in the #endif // INCLUDE_TSAN which does
>>>> not seem to be that much the style of the general code base so we might
>>>> want to remove them at some point)
>>>>
>>>> But that's a detail, LGTM,
>>>> Jc
>>>>
>>>>
>>>> On Tue, Apr 23, 2019 at 9:29 AM Arthur Eubanks <aeubanks at google.com>
>>>> wrote:
>>>>
>>>> > This changes adds instrumentation for various locks in the JVM. (not
>>>> > including JVMTI)
>>>> > http://cr.openjdk.java.net/~aeubanks/tsanlock/webrev.00
>>>> >
>>>> > Note the NonRacy tests still don't pass for some reason. I believe
>>>> it's due
>>>> > to dynamic linking of libtsan.so.
>>>> >
>>>>
>>>>
>>>> --
>>>>
>>>> Thanks,
>>>> Jc
>>>>
>>>
>
> --
>
> Thanks,
> Jc
>