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

Sat Aug 5 10:18:18 UTC 2017

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