[10] RFR: 8185164: GetOwnedMonitorInfo() returns incorrect owned monitor

serguei.spitsyn at oracle.com serguei.spitsyn at oracle.com
Sat Aug 5 18:37:34 UTC 2017


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