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

serguei.spitsyn at oracle.com serguei.spitsyn at oracle.com
Fri Aug 4 22:45:53 UTC 2017 

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