RFR: Add JVMTI raw monitor lock/unlock mechanisms

[Man Cao]

Just discussed with JC that the following will not work,
because TSAN_RUNTIME_ONLY defines its own scope, and it will immediately
call the destructor:
TSAN_RUNTIME_ONLY(TsanMutexScope tms(lock()));


-Man


On Mon, Apr 29, 2019 at 2:58 PM Arthur Eubanks <aeubanks at google.com> wrote:

> I don't see the comment change in src/hotspot/share/prims/jvmtiEnv.cpp.
> Other than that, LGTM. (no need for another webrev)
>
> On Mon, Apr 29, 2019 at 2:47 PM Jean Christophe Beyler <
> jcbeyler at google.com> wrote:
>
>> Hi Man & Arthur,
>>
>> New webrev is here:
>> http://cr.openjdk.java.net/~jcbeyler/jvmti/webrev.05/
>>
>> Let me know what you both think :)
>> Jc
>>
>>
>> -------------------------------------
>>
>> On Mon, Apr 29, 2019 at 2:18 PM Man Cao <manc at google.com> wrote:
>>
>>> webrev.04 looks good to me.
>>> Minor comment is that I'd add TODO or comment about
>>> pthread_mutex_lock/unlock should be removed from PerObjectCallback()
>>> and Java_JvmtiTaggerLoopRunner_iterateOverTags()
>>> in libAbstractNativeLoop.cpp, once we have handled the lock creation race.
>>> No need for a webrev for this.
>>>
>>>
>> Done :)
>>
>>
>>> For the lock creation race in JvmtiTagMap::JvmtiTagMap(), I think there
>>> is a valid C++ data race in double-checked locking
>>> in JvmtiTagMap::tag_map_for().
>>> Ideally accesses to JvmtiEnvBase::_tag_map should use
>>> OrderAccess::load_acquire() and OrderAccess::release_store(), and we should
>>> make TSAN aware of these fence semantics, then TSAN would not complain
>>> about the lock creation race.
>>> I can try sending serviceability-dev@ a change for OrderAccess.
>>>
>>
>> For what it's worth, even if you fix that, you still will get a race from
>> TSAN since tsan does not care about this particular race; it cares about
>> the lock creation in the instance itself. You would need to fix that and
>> not the setting of this tag map.
>> Good news is that you can use the jvmtiTaggertest to see if you are right
>> and succeed :)
>>
>>
>>>
>>> Lastly for Arthur's comment:
>>> > test/hotspot/jtreg/tsan/RawRacyNativeLoopTest.java:
>>> > Does "lock" have to be static?
>>> ThreadLocals are typically private static fields by convention:
>>> https://docs.oracle.com/en/java/javase/11/docs/api/java.base/java/lang/ThreadLocal.html
>>> .
>>>
>>> -Man
>>>
>>>
>>> On Mon, Apr 29, 2019 at 1:10 PM Arthur Eubanks <aeubanks at google.com>
>>> wrote:
>>>
>>>> src/hotspot/share/prims/jvm.cpp: is the cast to (void *) necessary?
>>>> -  return new Mutex(Mutex::native, "JVM_RawMonitorCreate");
>>>> +  void *mon = (void*) new Mutex(Mutex::native, "JVM_RawMonitorCreate");
>>>> +
>>>> +  TSAN_RAW_LOCK_CREATE(mon);
>>>> +  return mon;
>>>>
>>>> src/hotspot/share/prims/jvmtiEnv.cpp: the comment shouldn't be there,
>>>> or it should be updated
>>>> +  // A wait is modeled in Tsan as a simple release-acquire pair.
>>>> +  // The matching acquire annotation is below.
>>>> +  TSAN_RAW_LOCK_ACQUIRED(rmonitor);
>>>> +
>>>>    switch (r) {
>>>>    case ObjectMonitor::OM_INTERRUPTED:
>>>>      return JVMTI_ERROR_INTERRUPT;
>>>>    case ObjectMonitor::OM_ILLEGAL_MONITOR_STATE:
>>>>      return JVMTI_ERROR_NOT_MONITOR_OWNER;
>>>>
>>>> src/hotspot/share/prims/jvmtiTagMap.cpp:
>>>> It seems wrong to be mixing in Tsan specific stuff with MutexLocker. It
>>>> makes the separation of Tsan-specific features and the rest of the JVM less
>>>> clear. I would prefer something like
>>>> #if INCLUDE_TSAN
>>>> TsanMutexScope tsanMutexScope(lock());
>>>> #endif
>>>> as opposed to replacing MutexLocker with TsanVisibleMutexLocker.
>>>> TsanVisibleMutexLocker uses multiple inheritance? ew. What about having
>>>> the two superclasses as two member variables instead?
>>>>
>>>> I don't understand "good thing" in the comments below:
>>>> +  // This means that this lock is created under a mutex (good thing)
>>>> but then,
>>>> +  // subsequent uses do have a lock to protect it (good thing because
>>>> not
>>>> +  // needed in this case), however TSAN sees it as being needed
>>>> because:
>>>> +  //  - Another thread can come and get the newly created JvmtiTagMap
>>>> without a
>>>> +  //  lock and acquire the lock.
>>>> +  //  - This provokes a race for TSAN on the lock itself, though there
>>>> is no
>>>> +  //  real issue.
>>>>
>>>> test/hotspot/jtreg/tsan/RawRacyNativeLoopTest.java:
>>>> Does "lock" have to be static?
>>>>
>>>> On Fri, Apr 26, 2019 at 2:43 PM Jean Christophe Beyler <
>>>> jcbeyler at google.com> wrote:
>>>>
>>>>> 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
>>>>>
>>>>
>>
>> --
>>
>> Thanks,
>> Jc
>>
>