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

Sat Aug 5 11:20:55 UTC 2017

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