[10] RFR: 8185164: GetOwnedMonitorInfo() returns incorrect owned monitor
Daniel D. Daugherty
daniel.daugherty at oracle.com
Sun Aug 6 14:54:13 UTC 2017
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