[10] RFR: 8185164: GetOwnedMonitorInfo() returns incorrect owned monitor
Daniel D. Daugherty
daniel.daugherty at oracle.com
Mon Aug 7 00:42:37 UTC 2017
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?
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