[10] RFR: 8185164: GetOwnedMonitorInfo() returns incorrect owned monitor
serguei.spitsyn at oracle.com
serguei.spitsyn at oracle.com
Sun Aug 6 08:51:00 UTC 2017
Hi Dan,
Thank you for the test update!
It looks very reasonable to me, the test became pretty solid.
Thanks,
Serguei
On 8/5/17 19: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