RFR: Add JVMTI raw monitor lock/unlock mechanisms

[Jean Christophe Beyler]

Done on all points I believe Man.

I added the JvmtiTagMap change and added a new test for that as well. The
test was a bit annoying due to the fact that it was difficult to explain
that the "sum" parameter was safe to be updated before/after the heap
iteration. Currently, I've had to put a mutex around it all, which seems
like overkill but it was the only way Tsan would not complain.

I'm unclear if that is right, it seems that it is due to the fact that it
does not see the VM operation as a fork/join operation perhaps? I believe
this is systemic to any JVMTI operation that has a VM operation style
callback so we might have more work there.

Webrev: http://cr.openjdk.java.net/~jcbeyler/jvmti/webrev.02/

Btw, @Arthur: the // INCLUDE_TSAN is actually questionable... it seems half
the #endif do it and half don't so I'm not sure what is the right approach.

Let me know what you think,
Jc

On Wed, Apr 24, 2019 at 4:58 PM Man Cao <manc at google.com> wrote:

> Code in HotSpot looks good. Except that we have additional instrumentation
> in jvmtiTagMap.cpp that could be added in a following change.
>
> A few comments on tests:
> In AbstractNativeLoop.java:
> Would it make sense to make all native methods "static"? They don't access
> any instance field, right?
>
> In NonRawRacyNativeLoopTest.java:
> There are two calls to createRawLock() in NonRawRacyNativeLoopRunner.
> Could the one in main() be deleted?
>
> In RawRacyNativeLoopTest.java:
> Similarly, could the createRawLock() in main() be deleted?
> I think calling createRawLock() in run() would result in each thread
> creating 50000 raw monitors, due to the way in which AbstractLoop is
> written. Can we avoid doing this so the test can be a bit faster?
>
> -Man
>
>
> On Wed, Apr 24, 2019 at 11:01 AM Jean Christophe Beyler <
> jcbeyler at google.com> wrote:
>
>> Sounds good to me.
>>
>> Here it is:
>> http://cr.openjdk.java.net/~jcbeyler/jvmti/webrev.01/
>>
>> I also took the idea of adding a  TSAN_RUNTIME_ONLY, I think it works well
>> for one liners for code readability to be honest. If we want this, I'll do
>> another pass to propagate it around the rest of the changes in a separate
>> webrev.
>> cf:
>>
>> http://cr.openjdk.java.net/~jcbeyler/jvmti/webrev.01/src/hotspot/share/tsan/tsan.hpp.html
>>
>> And now with Arthur's change, all tests now pass (still would like to fix
>> the racing tests to be more restrictive),
>> Jc
>>
>>
>> On Wed, Apr 24, 2019 at 10:18 AM Arthur Eubanks <aeubanks at google.com>
>> wrote:
>>
>> > Yeah we should have another tsan header. Kind of like how our interface
>> > for the other TSAN functions is through SharedRuntime. Only that file
>> would
>> > include tsanExternalDecls.hpp. The other files would include the new
>> header
>> > file.
>> >
>> > On Wed, Apr 24, 2019 at 10:14 AM Jean Christophe Beyler <
>> > jcbeyler at google.com> wrote:
>> >
>> >> Hi Arthur,
>> >>
>> >> What do you mean: "pulling in tsanExternalDecls.hpp" is wrong? Seems
>> like
>> >> it is what we did internally and what you were doing for the lock
>> webrev as
>> >> well, ie have that as our include header. Are you suggesting we make
>> >> another tsan header for these methods.
>> >>
>> >> Ok for all other points (except the indentation, just the way webrev
>> does
>> >> things by default, the indentation is actually correct in my code),
>> >> Jc
>> >>
>> >> On Wed, Apr 24, 2019 at 9:58 AM Arthur Eubanks <aeubanks at google.com>
>> >> wrote:
>> >>
>> >>> Overall:
>> >>> Since AnnotateRWLock* are direct TSAN calls, I think we should have a
>> >>> wrapper around them. Then we can have things like checking if the
>> lock is
>> >>> NULL, etc, in a central location. And pulling in tsanExternalDecls.hpp
>> >>> seems wrong.
>> >>> "#if INCLUDE_TSAN" ends with "#endif // INCLUDE_TSAN", could you add
>> the
>> >>> comment for the #endifs.
>> >>>
>> >>> test/hotspot/jtreg/tsan/TsanRunner.java
>> >>> Could you rename "args" to "vmArgs"?
>> >>>
>> >>> src/hotspot/share/prims/jvmtiEnv.cpp.udiff
>> >>> Indentation around 3258 is wrong.
>> >>>
>> >>>
>> >>> On Tue, Apr 23, 2019 at 2:29 PM Jean Christophe Beyler <
>> >>> jcbeyler at google.com> wrote:
>> >>>
>> >>>> Hi all,
>> >>>>
>> >>>> I added the locks for the JVMTI raw monitors and added two tests, it
>> is
>> >>>> failing the non racy ones due to the JVM locks not yet included but I
>> >>>> think
>> >>>> that will resolve itself soon :)
>> >>>>
>> >>>> I also moved the agent library to C++.
>> >>>>
>> >>>> FWIW, we also need to clean up our tests that check for failing,
>> right
>> >>>> now
>> >>>> they pass because they generically just are checking for a TSAN race
>> but
>> >>>> not specifically which one, which is a bit wrong / imprecise.
>> >>>>
>> >>>> Webrev: http://cr.openjdk.java.net/~jcbeyler/jvmti/webrev.00/
>> >>>>
>> >>>> Let me know what you think,
>> >>>> Jc
>> >>>>
>> >>>
>> >>
>> >> --
>> >>
>> >> Thanks,
>> >> Jc
>> >>
>> >
>>
>> --
>>
>> Thanks,
>> Jc
>>
>

-- 

Thanks,
Jc