RFR: Add JVMTI raw monitor lock/unlock mechanisms

Fri Apr 26 21:43:35 UTC 2019

Seems like I forgot to say two things:

1) First here is the link to the newest webrev:
http://cr.openjdk.java.net/~jcbeyler/jvmti/webrev.04/
2) JTREG does not really allow two different native files, so as far as I
can tell all other examples do what I've done here for -agentlib and
System.loadlibrary

Thanks,
Jc

On Fri, Apr 26, 2019 at 9:50 AM Jean Christophe Beyler <jcbeyler at google.com>
wrote:

> Hi Man,
>
> New webrev is here:
>
> On Thu, Apr 25, 2019 at 10:41 PM Man Cao <manc at google.com> wrote:
>
>> It seems there are a few missing instrumentation in jvmtiTagMap.cpp:
>> 1. There should be a call to TSAN_RAW_LOCK_CREATE(&_lock) in
>> JvmtiTagMap::JvmtiTagMap(JvmtiEnv* env), and a call to
>> TSAN_RAW_LOCK_DESTROY(&_lock) in JvmtiTagMap::~JvmtiTagMap().
>>
>
> So funny thing I can't do that actually. When I do, TSAN complains about a
> race between the create and the lock itself. I don't know why it didn't
> show up in our internal testing but I've added a note to it. I'm going to
> run some tests on our version to see if I'm right that it just didn't show
> up due to how we were using it (enough time between the create and the
> iterate to not show up). But my study of the code this morning seems to
> lead to : yes TSAN sees it as a race but it is not really one.
>
>
>> 2. TsanVisibleMutexLocker should replace MutexLocker in
>> JvmtiTagMap::get_objects_with_tags().
>>
>
> Done, there was another one as well that now I did the same to. Same as
> above, I think we just did the minimal that we saw, but not the full set.
>
> (Note: I think I'm missing other tag callbacks that should be checked for
> races, I'll check the code and see in a second phase; probably with a more
> extensive study of the create/lock tsan race)
>
>
>> Will adding these fix the issue with "sum" in the test?
>>
>
> No they don't, I still have the same issue, the problem is that sum is
> changed without a lock in user code, and I fail to see how to do a "fence"
> for tsan from user to JVM and back. Except if we have a solution right now,
> I'd rather figure this out in a subsequent webrev as this webrev is already
> growing quite a bit, if that's ok :-)
>
>
>>
>> For the new tests, there is "-agentlib:AbstractNativeLoop" on the
>> command line, and they will do System.loadLibrary("AbstractNativeLoop")
>> again in class initializer. I'm not sure if this could cause any unexpected
>> behavior, but it is probably better to avoid doing this. Could we put
>> JVMTI-related code in a new file?
>>
>> In RawRacyNativeLoopTest, I think there is a race on the "private long
>> lock" field itself. Two threads will write different values to the same
>> field, so TSAN will likely report a race on this field regardless of what
>> happens in JVMTI code.
>>
>
> Done, added a threadlocal instead to make it really localized. This is yet
> another reason I'd like our race detection in our tests to check the
> signatures but we need symbolization first (hint hint).
> Also tested and it now fails with the right race.
>
>
>>
>>
>> Minor comments:
>> I think a better name for TsanSimulatedMutexLocker could be
>> TsanMutexScope, because TsanSimulatedMutexLocker has nothing to do with the
>> MutexLocker class. Also, this class could be a subclase of StackObj.
>>
>
> Done, I think the StackObj would have been caught when running this in
> fastdebug, which we have not yet set up to be done by default in our
> testing...
>
>
>> I'd keep our internal comment about "Simulates barrier synchronization on
>> safepoint" in doit() functions in jvmtiTagMap.cpp. I think they are quite
>> useful to explain why we need the TSAN instrumentation.
>>
>
> Done.
>
>
>> In libAbstractNativeLoop.cpp, why "include <string.h>" but I don't see
>> any string functions?
>>
>
> memset is defined there :)
>
>
>
>>
>> -Man
>>
>>
>> On Thu, Apr 25, 2019 at 3:01 PM Jean Christophe Beyler <
>> jcbeyler at google.com> wrote:
>>
>>> 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
>>>
>>
>
> --
>
> Thanks,
> Jc
>

-- 

Thanks,
Jc