[10] RFR: 8185164: GetOwnedMonitorInfo() returns incorrect owned monitor
serguei.spitsyn at oracle.com
serguei.spitsyn at oracle.com
Mon Aug 7 08:38:42 UTC 2017
On 8/6/17 17:42, Daniel D. Daugherty wrote:
> Solaris X64 test results with the fix look good. Here's what I ran:
>
> - internal VM/NSK JVM/TI, JDWP and JDI tests
> - JTREG jdk/test/:jdk_svc, hotspot/test/:hotspot_jprt
> hotspot/test/:hotspot_runtime_tier2,
> hotspot/test/:hotspot_runtime_tier3
>
> I've run the new test against my baseline repo and it fails nicely.
> I've run the new test against my experimental repo and it works nicely.
>
> I think this fix is ready to go... Serguei, are you planning on
> sponsoring?
Sure, Dan.
I'll be sponsoring this.
The fix itself looks good to me.
I would like to make another review pass on the test tomorrow.
Thanks,
Serguei
>
> Dan
>
> P.S.
> And thumbs up on the latest webrev...
>
>
>
> On 8/6/17 6:33 PM, Yasumasa Suenaga wrote:
>> Hi Dan,
>>
>>>> * GetOwnedMonitorInfoTest.java
>>>> - I use Thread#yield() to wait until MonitorContendedEnter
>>>> event is
>>>> fired.
>>>
>>> Please switch back to a small Thread.sleep(). Thread.yield()
>>> can be a no-op on some platforms which will make the wait
>>> loop very hot.
>> I fixed it in new webrev:
>>
>> http://cr.openjdk.java.net/~ysuenaga/JDK-8185164/webrev.03/
>>
>>
>> Thanks,
>>
>> Yasumasa
>>
>>
>> 2017-08-06 23:54 GMT+09:00 Daniel D. Daugherty
>> <daniel.daugherty at oracle.com>:
>>> On 8/6/17 1:28 AM, Yasumasa Suenaga wrote:
>>>> Hi Dan, Serguei,
>>>>
>>>> Thanks Serguei! I can run new testcase on my environment.
>>>>
>>>> Dan, I modified your patch and uploaded as webrev:
>>>>
>>>> http://cr.openjdk.java.net/~ysuenaga/JDK-8185164/webrev.02/
>>>>
>>>>
>>>> * GetOwnedMonitorInfoTest.java
>>>> - I use Thread#yield() to wait until MonitorContendedEnter
>>>> event is
>>>> fired.
>>>
>>> Please switch back to a small Thread.sleep(). Thread.yield()
>>> can be a no-op on some platforms which will make the wait
>>> loop very hot.
>>>
>>>
>>>> * libGetOwnedMonitorInfoTest.c
>>>> - I added "volatile" to "status" variable.
>>>
>>> Sorry I missed that one.
>>>
>>>
>>>> - I moved "event_has_posted = JNI_TRUE" to each before return
>>>> statement.
>>>
>>> Good catch. My original setting of event_has_posted had
>>> a narrow race.
>>>
>>> I'm good on the changes. I'll have test results later today.
>>>
>>> Dan
>>>
>>>
>>>
>>>>
>>>> Yasumasa
>>>>
>>>>
>>>> On 2017/08/06 11:33, Daniel D. Daugherty wrote:
>>>>> Hi Yasumasa and Serguei,
>>>>>
>>>>> I've made some tweaks to the test and attached an updated patch:
>>>>>
>>>>> GetOwnedMonitorInfoTest.java changes:
>>>>>
>>>>> - deleted thread 't2'
>>>>> - made changes to make monitor contention not racy:
>>>>> - added 'hasEventPosted()' native function
>>>>> - changed Main to grab the GetOwnedMonitorInfoTest.class monitor
>>>>> before launching 't1'
>>>>> - added loop to check hasEventPosted() function while holding
>>>>> the GetOwnedMonitorInfoTest.class monitor
>>>>> - moved the delay to this new loop
>>>>>
>>>>> libGetOwnedMonitorInfoTest.c changes:
>>>>>
>>>>> - added event_has_posted flag to know when MonitorContendedEnter
>>>>> event has been posted
>>>>> - added missing error checks to JVM/TI functions
>>>>> - print an error message when MonitorContendedEnter or
>>>>> MonitorContendedEntered gets an incorrect monitor count
>>>>> - print error and warnings to 'stderr'
>>>>> - add Java_GetOwnedMonitorInfoTest_hasEventPosted function
>>>>>
>>>>> I've tested the latest version of test in a repo without the fix
>>>>> and it fails (as expected). Here's some sample output:
>>>>>
>>>>> ----------System.out:(7/231)----------
>>>>> Agent_OnLoad started
>>>>> Agent_OnLoad finished
>>>>> Main starting worker thread.
>>>>> Main waiting for event.
>>>>> Thread in sync section: Thread-1
>>>>> MonitorContendedEnter: Thread-1 owns 1 monitor(s)
>>>>> MonitorContendedEntered: Thread-1 owns 1 monitor(s)
>>>>> ----------System.err:(14/931)----------
>>>>> MonitorContendedEnter: FAIL: monitorCount should be zero.
>>>>> java.lang.RuntimeException: FAILED status returned from the agent
>>>>> at
>>>>> GetOwnedMonitorInfoTest.main(GetOwnedMonitorInfoTest.java:81)
>>>>> at
>>>>> java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native
>>>>>
>>>>> Method)
>>>>> at
>>>>> java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
>>>>>
>>>>> at
>>>>> java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
>>>>>
>>>>> at
>>>>> java.base/java.lang.reflect.Method.invoke(Method.java:564)
>>>>> at
>>>>> com.sun.javatest.regtest.agent.MainWrapper$MainThread.run(MainWrapper.java:115)
>>>>>
>>>>> at java.base/java.lang.Thread.run(Thread.java:844)
>>>>>
>>>>> JavaTest Message: Test threw exception: java.lang.RuntimeException:
>>>>> FAILED status returned from the agent
>>>>> JavaTest Message: shutting down test
>>>>>
>>>>>
>>>>> Dan
>>>>>
>>>>>
>>>>> On 8/5/17 12:37 PM, serguei.spitsyn at oracle.com wrote:
>>>>>> Hi Yasumasa,
>>>>>>
>>>>>> I use the following script to run the test on linux:
>>>>>>
>>>>>> % cat run.sh
>>>>>> #!/bin/sh
>>>>>> REPO=<my_repo>
>>>>>> IMAGES=${REPO}/build/linux-x86_64-normal-server-release/images
>>>>>> export JAVA_HOME=${IMAGES}/jdk
>>>>>> export LD_LIBRARY_PATH=${IMAGES}/test/hotspot/jtreg/native
>>>>>>
>>>>>> jtreg -J-Dtest.java.opts='-server' \
>>>>>> -jdk ${JAVA_HOME} \
>>>>>> -nativepath:${LD_LIBRARY_PATH} \
>>>>>> $REPO/hotspot/test/serviceability/jvmti/GetOwnedMonitorInfo
>>>>>>
>>>>>>
>>>>>> Thanks,
>>>>>> Serguei
>>>>>>
>>>>>>
>>>>>> On 8/5/17 06:31, Yasumasa Suenaga wrote:
>>>>>>> Hi Serguei,
>>>>>>>
>>>>>>> I uploaded new webrev:
>>>>>>> http://cr.openjdk.java.net/~ysuenaga/JDK-8185164/webrev.01/
>>>>>>>
>>>>>>> I tried to run serviceability/jvmti/GetOwnedMonitorInfo but it is
>>>>>>> failed because jtreg needs -nativepath option.
>>>>>>> But I didn't know what path should I set to -nativepath.
>>>>>>>
>>>>>>>
>>>>>>> Thanks,
>>>>>>>
>>>>>>> Yasumasa
>>>>>>>
>>>>>>>
>>>>>>> On 2017/08/05 20:20, serguei.spitsyn at oracle.com wrote:
>>>>>>>> Hi Yasumasa,
>>>>>>>>
>>>>>>>> Please, merge the converted testcase.
>>>>>>>> It still might need more tweaks though.
>>>>>>>>
>>>>>>>> Thanks,
>>>>>>>> Serguei
>>>>>>>>
>>>>>>>>
>>>>>>>> On 8/5/17 03:18, Yasumasa Suenaga wrote:
>>>>>>>>> Hi Serguei,
>>>>>>>>>
>>>>>>>>> Thank you so much!
>>>>>>>>> Should I merge your testcase? Or can you push this change?
>>>>>>>>>
>>>>>>>>> I agree to your change as a reviewer.
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Yasumasa
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> On 2017/08/05 11:34, serguei.spitsyn at oracle.com wrote:
>>>>>>>>>> The updated patch attached.
>>>>>>>>>> Now the test is passed with the suggested fix and failed
>>>>>>>>>> without it.
>>>>>>>>>>
>>>>>>>>>> Thanks,
>>>>>>>>>> Serguei
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> On 8/4/17 15:45, serguei.spitsyn at oracle.com wrote:
>>>>>>>>>>> On 8/4/17 14:26, Daniel D. Daugherty wrote:
>>>>>>>>>>>> On 8/4/17 3:17 PM, serguei.spitsyn at oracle.com wrote:
>>>>>>>>>>>>> The patch is attached.
>>>>>>>>>>>>> It may need some tweaks though.
>>>>>>>>>>>>> I was not able to make it fail yet.
>>>>>>>>>>>>
>>>>>>>>>>>> I don't think the original test had "failure" detection.
>>>>>>>>>>>> You were just supposed to notice that a pending monitor
>>>>>>>>>>>> was listed under the wrong list.
>>>>>>>>>>> Nothing is listed.
>>>>>>>>>>> Strange thing is I do not see the monitor events fired.
>>>>>>>>>>> I'm using 10 for testing.
>>>>>>>>>>>
>>>>>>>>>>> Thanks,
>>>>>>>>>>> Serguei
>>>>>>>>>>>
>>>>>>>>>>>> Dan
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>> On 8/4/17 12:45, Daniel D. Daugherty wrote:
>>>>>>>>>>>>>> Thanks Serguei!
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> I happen to be doing a test run this weekend that
>>>>>>>>>>>>>> includes most
>>>>>>>>>>>>>> of the
>>>>>>>>>>>>>> JPDA stack of tests so I'll include the following in my
>>>>>>>>>>>>>> experiment:
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> $ hg log -v -r tip
>>>>>>>>>>>>>> changeset: 12872:bb66cd7c61b1
>>>>>>>>>>>>>> tag: 8185164.patch
>>>>>>>>>>>>>> tag: qtip
>>>>>>>>>>>>>> tag: tip
>>>>>>>>>>>>>> user: dcubed
>>>>>>>>>>>>>> date: Fri Aug 04 13:41:29 2017 -0600
>>>>>>>>>>>>>> files: src/share/vm/runtime/objectMonitor.cpp
>>>>>>>>>>>>>> description:
>>>>>>>>>>>>>> imported patch 8185164.patch
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> That will get the product code changes a complete round of
>>>>>>>>>>>>>> testing
>>>>>>>>>>>>>> on Solaris X64 at least... :-)
>>>>>>>>>>>>>
>>>>>>>>>>>>> Great!
>>>>>>>>>>>>>
>>>>>>>>>>>>> Thanks,
>>>>>>>>>>>>> Serguei
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Dan
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> On 8/4/17 1:31 PM, serguei.spitsyn at oracle.com wrote:
>>>>>>>>>>>>>>> Dan,
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Thank you for letting me know about this discussion.
>>>>>>>>>>>>>>> I'll try to convert the attached test case to the JTreg
>>>>>>>>>>>>>>> format.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Thanks,
>>>>>>>>>>>>>>> Serguei
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> On 8/4/17 11:16, Daniel D. Daugherty wrote:
>>>>>>>>>>>>>>>> Adding Serguei to this thread directly since he's back
>>>>>>>>>>>>>>>> from
>>>>>>>>>>>>>>>> vacation!
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> On 7/31/17 10:14 PM, David Holmes wrote:
>>>>>>>>>>>>>>>>> Hi Dan,
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> On 26/07/2017 11:52 PM, Daniel D. Daugherty wrote:
>>>>>>>>>>>>>>>>>> On 7/26/17 12:11 AM, David Holmes wrote:
>>>>>>>>>>>>>>>>>>> On 26/07/2017 10:27 AM, Yasumasa Suenaga wrote:
>>>>>>>>>>>>>>>>>>>> Hi Dan,
>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>> I've added some analysis to the bug report
>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>> Thanks!
>>>>>>>>>>>>>>>>>>>> I tried to fix this issue in
>>>>>>>>>>>>>>>>>>>> JvmtiEnvBase::get_owned_monitors() at first.
>>>>>>>>>>>>>>>>>>>> But it is difficult because we cannot know pending
>>>>>>>>>>>>>>>>>>>> monitor
>>>>>>>>>>>>>>>>>>>> if thread state is MONITOR_CONTENDED_ENTER when
>>>>>>>>>>>>>>>>>>>> get_owned_monitor() is
>>>>>>>>>>>>>>>>>>>> called.
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>> I need to look closer at this when I get back from
>>>>>>>>>>>>>>>>>>> vacation
>>>>>>>>>>>>>>>>>>> next week.
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> Seems like you're back already. :-)
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>> A pending monitor should not be reported as owned
>>>>>>>>>>>>>>>>>>> (unless
>>>>>>>>>>>>>>>>>>> the spec says otherwise) and it seems odd to me to
>>>>>>>>>>>>>>>>>>> fix the current problem
>>>>>>>>>>>>>>>>>>> by marking the monitor as pending earlier.
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> It's the updating of the _current_pending_monitor
>>>>>>>>>>>>>>>>>> field that
>>>>>>>>>>>>>>>>>> allows JvmtiEnvBase::get_locked_objects_in_frame() to
>>>>>>>>>>>>>>>>>> recognize
>>>>>>>>>>>>>>>>>> that the monitor observed in the frame is only
>>>>>>>>>>>>>>>>>> pending and
>>>>>>>>>>>>>>>>>> is not owned.
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> I put a fairly detailed note in the bug yesterday,
>>>>>>>>>>>>>>>>>> but you
>>>>>>>>>>>>>>>>>> should look at that when you're officially back!
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> Thanks for clarifying things. I also added a comment
>>>>>>>>>>>>>>>>> to the
>>>>>>>>>>>>>>>>> bug report.
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> I think the fix is sound and prevents anyone from
>>>>>>>>>>>>>>>>> observing
>>>>>>>>>>>>>>>>> the case where the monitor will be seen in the
>>>>>>>>>>>>>>>>> stack-frame, but has not yet
>>>>>>>>>>>>>>>>> been set as the "pending monitor". As far as I can
>>>>>>>>>>>>>>>>> tell it is only this case
>>>>>>>>>>>>>>>>> (GetOwnedMonitorInfo from the contended-monitor event
>>>>>>>>>>>>>>>>> callback in the
>>>>>>>>>>>>>>>>> current thread) that will be able to observe the change.
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> One scenario that I worry about here is that a
>>>>>>>>>>>>>>>> GetCurrentContendedMonitor()
>>>>>>>>>>>>>>>> call on a target thread will now be able to return a
>>>>>>>>>>>>>>>> non-NULL
>>>>>>>>>>>>>>>> value for the
>>>>>>>>>>>>>>>> object, when GetThreadState() will be able to return
>>>>>>>>>>>>>>>> something
>>>>>>>>>>>>>>>> other than
>>>>>>>>>>>>>>>> blocked (on monitor enter) for the thread.
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> I don't see anything in the JVM/TI spec that says such a
>>>>>>>>>>>>>>>> scenario is
>>>>>>>>>>>>>>>> wrong; I'm only worried about whether we have any tests
>>>>>>>>>>>>>>>> that
>>>>>>>>>>>>>>>> would catch
>>>>>>>>>>>>>>>> this slight change in behavior. In any case, one of these
>>>>>>>>>>>>>>>> operations has
>>>>>>>>>>>>>>>> to "happen first":
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> - thread is marked as blocked
>>>>>>>>>>>>>>>> - monitor is flagged as contended
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> Currently, they happen in the above order and the fix
>>>>>>>>>>>>>>>> proposes
>>>>>>>>>>>>>>>> to
>>>>>>>>>>>>>>>> change the order and I see no reason not to do it.
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> I would like the test attached to the bug to be
>>>>>>>>>>>>>>>> converted into
>>>>>>>>>>>>>>>> a native
>>>>>>>>>>>>>>>> JTREG test that lives in
>>>>>>>>>>>>>>>> hotspot/test/serviceability/jvmti.
>>>>>>>>>>>>>>>> See the
>>>>>>>>>>>>>>>> following test as a possible example:
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> hotspot/test/serviceability/jvmti/GetNamedModule
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> for how to do this... I haven't done one of these new
>>>>>>>>>>>>>>>> native
>>>>>>>>>>>>>>>> JTREG
>>>>>>>>>>>>>>>> tests myself, but I believe Serguei has...
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> Dan
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> Thanks,
>>>>>>>>>>>>>>>>> David
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> Dan
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>> Thanks,
>>>>>>>>>>>>>>>>>>> David
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>> Did you run the jdk repo's com/sun/jdi tests with
>>>>>>>>>>>>>>>>>>>>> your
>>>>>>>>>>>>>>>>>>>>> fix?
>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>> I have not done yet.
>>>>>>>>>>>>>>>>>>>> I have a trip until 28 July JST. So I will run it
>>>>>>>>>>>>>>>>>>>> after
>>>>>>>>>>>>>>>>>>>> that.
>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>> Yasumasa
>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>> On 2017/07/26 7:05, Daniel D. Daugherty wrote:
>>>>>>>>>>>>>>>>>>>>> On 7/24/17 8:40 PM, Yasumasa Suenaga wrote:
>>>>>>>>>>>>>>>>>>>>>> Hi all,
>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>> I tried to get owned monitors in
>>>>>>>>>>>>>>>>>>>>>> MonitorContendedEnter
>>>>>>>>>>>>>>>>>>>>>> JVMTI event handler.
>>>>>>>>>>>>>>>>>>>>>> However GetOwnedMonitorInfo JVMTI function returns a
>>>>>>>>>>>>>>>>>>>>>> monitor which is
>>>>>>>>>>>>>>>>>>>>>> not yet owned.
>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>> I attached reproducer to JBS. Please read README.md.
>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>> I think GetOwnedMonitorInfo() should not return a
>>>>>>>>>>>>>>>>>>>>>> pending monitor.
>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>> I uploaded webrev. Could you review?
>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>> http://cr.openjdk.java.net/~ysuenaga/JDK-8185164/webrev.00/
>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>> I hope this fix is applied to 8u or later release.
>>>>>>>>>>>>>>>>>>>>>> I cannot access JPRT. So I need a sponsor.
>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>> Thanks for the bug report. It's nice to have a
>>>>>>>>>>>>>>>>>>>>> test case
>>>>>>>>>>>>>>>>>>>>> and a proposed
>>>>>>>>>>>>>>>>>>>>> fix all in the bug report! I've added some
>>>>>>>>>>>>>>>>>>>>> analysis to
>>>>>>>>>>>>>>>>>>>>> the bug report
>>>>>>>>>>>>>>>>>>>>> and we'll need to run this fix through Oracle's
>>>>>>>>>>>>>>>>>>>>> JPDA test
>>>>>>>>>>>>>>>>>>>>> stack which
>>>>>>>>>>>>>>>>>>>>> is not (yet) open.
>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>> Did you run the jdk repo's com/sun/jdi tests with
>>>>>>>>>>>>>>>>>>>>> your
>>>>>>>>>>>>>>>>>>>>> fix?
>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>> Dan
>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>> Thanks,
>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>> Yasumasa
>>>>>>>>>>>>>>>>>>>>>
>
More information about the serviceability-dev
mailing list