[10] RFR: 8185164: GetOwnedMonitorInfo() returns incorrect owned monitor
serguei.spitsyn at oracle.com
serguei.spitsyn at oracle.com
Sat Aug 5 02:34:48 UTC 2017
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
>>>>>>>>>>>
>>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>
-------------- next part --------------
diff -r f4315a059412 make/test/JtregNative.gmk
--- a/make/test/JtregNative.gmk Tue Aug 01 08:53:32 2017 -0700
+++ b/make/test/JtregNative.gmk Fri Aug 04 19:31:38 2017 -0700
@@ -61,6 +61,7 @@
$(HOTSPOT_TOPDIR)/test/runtime/noClassDefFoundMsg \
$(HOTSPOT_TOPDIR)/test/compiler/floatingpoint/ \
$(HOTSPOT_TOPDIR)/test/compiler/calls \
+ $(HOTSPOT_TOPDIR)/test/serviceability/jvmti/GetOwnedMonitorInfo \
$(HOTSPOT_TOPDIR)/test/serviceability/jvmti/GetNamedModule \
$(HOTSPOT_TOPDIR)/test/serviceability/jvmti/IsModifiableModule \
$(HOTSPOT_TOPDIR)/test/serviceability/jvmti/AddModuleReads \
@@ -92,6 +93,7 @@
ifeq ($(TOOLCHAIN_TYPE), solstudio)
BUILD_HOTSPOT_JTREG_LIBRARIES_LIBS_liboverflow := -lc
BUILD_HOTSPOT_JTREG_LIBRARIES_LIBS_libSimpleClassFileLoadHook := -lc
+ BUILD_HOTSPOT_JTREG_LIBRARIES_LIBS_libGetOwnedMonitorInfoTest := -lc
BUILD_HOTSPOT_JTREG_LIBRARIES_LIBS_libGetNamedModuleTest := -lc
BUILD_HOTSPOT_JTREG_LIBRARIES_LIBS_libIsModifiableModuleTest := -lc
BUILD_HOTSPOT_JTREG_LIBRARIES_LDFLAGS_libAddModuleReadsTest := -lc
diff -r f4315a059412 src/share/vm/runtime/objectMonitor.cpp
--- a/src/share/vm/runtime/objectMonitor.cpp Tue Aug 01 08:53:32 2017 -0700
+++ b/src/share/vm/runtime/objectMonitor.cpp Fri Aug 04 19:31:38 2017 -0700
@@ -307,6 +307,8 @@
{ // Change java thread status to indicate blocked on monitor enter.
JavaThreadBlockedOnMonitorEnterState jtbmes(jt, this);
+ Self->set_current_pending_monitor(this);
+
DTRACE_MONITOR_PROBE(contended__enter, this, object(), jt);
if (JvmtiExport::should_post_monitor_contended_enter()) {
JvmtiExport::post_monitor_contended_enter(jt, this);
@@ -321,8 +323,6 @@
OSThreadContendState osts(Self->osthread());
ThreadBlockInVM tbivm(jt);
- Self->set_current_pending_monitor(this);
-
// TODO-FIXME: change the following for(;;) loop to straight-line code.
for (;;) {
jt->set_suspend_equivalent();
diff -r f4315a059412 test/serviceability/jvmti/GetOwnedMonitorInfo/GetOwnedMonitorInfoTest.java
--- /dev/null Thu Jan 01 00:00:00 1970 +0000
+++ b/test/serviceability/jvmti/GetOwnedMonitorInfo/GetOwnedMonitorInfoTest.java Fri Aug 04 19:31:38 2017 -0700
@@ -0,0 +1,77 @@
+/*
+ * Copyright (c) 2017, Oracle and/or its affiliates. All rights reserved.
+ * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
+ *
+ * This code is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 only, as
+ * published by the Free Software Foundation.
+ *
+ * This code is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License
+ * version 2 for more details (a copy is included in the LICENSE file that
+ * accompanied this code).
+ *
+ * You should have received a copy of the GNU General Public License version
+ * 2 along with this work; if not, write to the Free Software Foundation,
+ * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
+ *
+ * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
+ * or visit www.oracle.com if you need additional information or have any
+ * questions.
+ */
+
+
+/**
+ * @test
+ * @summary Verifies the JVMTI GetOwnedMonitorInfo API
+ * @modules jdk.jdi
+ * @compile GetOwnedMonitorInfoTest.java
+ * @run main/othervm/native -agentlib:GetOwnedMonitorInfoTest GetOwnedMonitorInfoTest
+ */
+
+import java.io.PrintStream;
+
+public class GetOwnedMonitorInfoTest implements Runnable {
+
+ static {
+ try {
+ System.loadLibrary("GetOwnedMonitorInfoTest");
+ } catch (UnsatisfiedLinkError ule) {
+ System.err.println("Could not load GetOwnedMonitorInfoTest library");
+ System.err.println("java.library.path: "
+ + System.getProperty("java.library.path"));
+ throw ule;
+ }
+ }
+
+ native static int check();
+
+ public void run() {
+ String name = Thread.currentThread().getName();
+ try {
+ synchronized (GetOwnedMonitorInfoTest.class) {
+ Thread.sleep(100);
+ System.out.println("Thread in sync section: " + name);
+ }
+ } catch (Exception e) {
+ e.printStackTrace();
+ }
+ }
+
+ public static void main(String[] args) throws Exception {
+ Thread t1 = new Thread(new GetOwnedMonitorInfoTest());
+ Thread t2 = new Thread(new GetOwnedMonitorInfoTest());
+
+ t1.start();
+ t2.start();
+
+ t1.join();
+ t2.join();
+
+ int status = check();
+ if (status != 0) {
+ throw new RuntimeException("FAILED status returned from the agent");
+ }
+ }
+}
diff -r f4315a059412 test/serviceability/jvmti/GetOwnedMonitorInfo/libGetOwnedMonitorInfoTest.c
--- /dev/null Thu Jan 01 00:00:00 1970 +0000
+++ b/test/serviceability/jvmti/GetOwnedMonitorInfo/libGetOwnedMonitorInfoTest.c Fri Aug 04 19:31:38 2017 -0700
@@ -0,0 +1,179 @@
+/*
+ * Copyright (c) 2017, Oracle and/or its affiliates. All rights reserved.
+ * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
+ *
+ * This code is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 only, as
+ * published by the Free Software Foundation.
+ *
+ * This code is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License
+ * version 2 for more details (a copy is included in the LICENSE file that
+ * accompanied this code).
+ *
+ * You should have received a copy of the GNU General Public License version
+ * 2 along with this work; if not, write to the Free Software Foundation,
+ * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
+ *
+ * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
+ * or visit www.oracle.com if you need additional information or have any
+ * questions.
+ */
+
+#include <stdio.h>
+#include <string.h>
+#include "jvmti.h"
+#include "jni.h"
+
+#ifdef __cplusplus
+extern "C" {
+#endif
+
+#ifndef JNI_ENV_ARG
+
+#ifdef __cplusplus
+#define JNI_ENV_ARG(x, y) y
+#define JNI_ENV_PTR(x) x
+#else
+#define JNI_ENV_ARG(x,y) x, y
+#define JNI_ENV_PTR(x) (*x)
+#endif
+
+#endif
+
+#define TranslateError(err) "JVMTI error"
+
+#define PASSED 0
+#define FAILED 2
+
+static jint status = PASSED;
+
+static jint Agent_Initialize(JavaVM *jvm, char *options, void *reserved);
+
+JNIEXPORT void JNICALL
+MonitorContendedEnter(jvmtiEnv *jvmti, JNIEnv *env, jthread thread, jobject monitor) {
+ jvmtiThreadInfo threadInfo;
+ jint monitorCount;
+ jobject *ownedMonitors;
+
+ (*jvmti)->GetThreadInfo(jvmti, thread, &threadInfo);
+ (*jvmti)->GetOwnedMonitorInfo(jvmti, thread, &monitorCount, &ownedMonitors);
+
+ printf("MonitorContendedEnter: %s owns %d monitor(s)\n",
+ threadInfo.name, monitorCount);
+
+ (*jvmti)->Deallocate(jvmti, (unsigned char *)ownedMonitors);
+ (*jvmti)->Deallocate(jvmti, (unsigned char *)threadInfo.name);
+
+ if (monitorCount != 0) {
+ status = FAILED;
+ }
+}
+
+JNIEXPORT void JNICALL
+MonitorContendedEntered(jvmtiEnv *jvmti, JNIEnv *env, jthread thread, jobject monitor) {
+ jvmtiThreadInfo threadInfo;
+ jint monitorCount;
+ jobject *ownedMonitors;
+
+ (*jvmti)->GetThreadInfo(jvmti, thread, &threadInfo);
+ (*jvmti)->GetOwnedMonitorInfo(jvmti, thread, &monitorCount, &ownedMonitors);
+
+ printf("MonitorContendedEntered: %s owns %d monitor(s)\n",
+ threadInfo.name, monitorCount);
+
+ (*jvmti)->Deallocate(jvmti, (unsigned char *)ownedMonitors);
+ (*jvmti)->Deallocate(jvmti, (unsigned char *)threadInfo.name);
+
+ if (monitorCount != 1) {
+ status = FAILED;
+ }
+}
+
+JNIEXPORT jint JNICALL
+Agent_OnLoad(JavaVM *jvm, char *options, void *reserved) {
+ return Agent_Initialize(jvm, options, reserved);
+}
+
+JNIEXPORT jint JNICALL
+Agent_OnAttach(JavaVM *jvm, char *options, void *reserved) {
+ return Agent_Initialize(jvm, options, reserved);
+}
+
+JNIEXPORT jint JNICALL
+JNI_OnLoad(JavaVM *jvm, void *reserved) {
+ return JNI_VERSION_1_8;
+}
+
+static
+jint Agent_Initialize(JavaVM *jvm, char *options, void *reserved) {
+ jint res;
+ jvmtiError err;
+ jvmtiEnv *jvmti;
+ jvmtiCapabilities caps;
+ jvmtiEventCallbacks callbacks;
+
+ printf("Agent_OnLoad started\n");
+
+ res = JNI_ENV_PTR(jvm)->GetEnv(JNI_ENV_ARG(jvm, (void **) &jvmti),
+ JVMTI_VERSION_1);
+ if (res != JNI_OK || jvmti == NULL) {
+ printf(" Error: wrong result of a valid call to GetEnv!\n");
+ return JNI_ERR;
+ }
+
+ err = (*jvmti)->GetPotentialCapabilities(jvmti, &caps);
+ if (err != JVMTI_ERROR_NONE) {
+ printf("Agent_OnLoad: error in JVMTI GetPotentialCapabilities: %d\n", err);
+ return JNI_ERR;
+ }
+
+ err = (*jvmti)->AddCapabilities(jvmti, &caps);
+ if (err != JVMTI_ERROR_NONE) {
+ printf("Agent_OnLoad: error in JVMTI AddCapabilities: %d\n", err);
+ }
+
+ err = (*jvmti)->GetCapabilities(jvmti, &caps);
+ if (err != JVMTI_ERROR_NONE) {
+ printf("Agent_OnLoad: error in JVMTI GetCapabilities: %d\n", err);
+ return JNI_ERR;
+ }
+
+ if (!caps.can_generate_monitor_events) {
+ printf("Warning: Monitor events are not implemented\n");
+ }
+ if (!caps.can_get_owned_monitor_info) {
+ printf("Warning: GetOwnedMonitorInfo is not implemented\n");
+ }
+
+ callbacks.MonitorContendedEnter = &MonitorContendedEnter;
+ callbacks.MonitorContendedEntered = &MonitorContendedEntered;
+
+ err = (*jvmti)->SetEventCallbacks(jvmti, &callbacks, sizeof(jvmtiEventCallbacks));
+ if (err != JVMTI_ERROR_NONE) {
+ printf("Agent_OnLoad: error in JVMTI SetEventCallbacks: %d\n", err);
+ }
+
+ err = (*jvmti)->SetEventNotificationMode(jvmti, JVMTI_ENABLE,
+ JVMTI_EVENT_MONITOR_CONTENDED_ENTER, NULL);
+ if (err != JVMTI_ERROR_NONE) {
+ printf("Agent_OnLoad: error in JVMTI SetEventNotificationMode #1: %d\n", err);
+ }
+ err = (*jvmti)->SetEventNotificationMode(jvmti, JVMTI_ENABLE,
+ JVMTI_EVENT_MONITOR_CONTENDED_ENTERED, NULL);
+ if (err != JVMTI_ERROR_NONE) {
+ printf("Agent_OnLoad: error in JVMTI SetEventNotificationMode #2: %d\n", err);
+ }
+ printf("Agent_OnLoad finished\n");
+ return JNI_OK;
+}
+
+JNIEXPORT jint JNICALL
+Java_GetOwnedMonitorInfoTest_check(JNIEnv *env, jclass cls) {
+ return status;
+}
+
+#ifdef __cplusplus
+}
+#endif
More information about the serviceability-dev
mailing list