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

[Daniel D. Daugherty]

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.

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
>>>>>>>>>
>>>>>>
>>>>
>>>
>>
>