Re: RFR: Fix race condition in jdwp

2018-04-11 Thread Andrew Leonard 
Hi Serguei,
Thank you for raising the bug.
I had a chat with one of my colleagues who could recreate it, and it's 
probably related to the handshaking that is done in the particular 
scenario. So with the JCK harness:

com.sun.jck.lib.ExecJCKTestOtherJVMCmd LD_LIBRARY_PATH=/javatest/lib/jck
/jck8b/natives/linux_x86-64 /projects/jck/jdwp/j2sdk-image/bin/java 
-Xdump:system:none -Xdump:system:events=gpf+abort+traceassert+corruptcache 
-Xdump:snap:none -Xdump:snap:events=gpf+abort+traceassert+corruptcache 
-Xdump:java:none -Xdump:java:events=gpf+abort+traceassert+corruptcache 
-Xdump:heap:none -Xdump:heap:events=gpf+abort+traceassert+corruptcache -
Xfuture -agentlib:jdwp=server=y,transport=dt_socket,address=localhost
:35000,suspend=y -classpath /javatest/lib/jck
/JCK8b-b03/JCK-runtime-8b/classes -Djava.security.policy=/javatest/lib/jck
/JCK8b-b03/JCK-runtime-8b/lib/jck.policy 
javasoft.sqe.jck.lib.jpda.jdwp.DebuggeeLoader -waittime=600 
-msgSwitch=ub1604x64vm10:38636 -componentName=
ArrayReference.GetValues.getvalues002

Note that the JCK test harness starts the target process, attaches to it, 
and sends the resume command 
in a very short time with no handshaking.

That may not help..but hopefully helps explain things a bit? It's the 
timing of the resume command during the test that is crucial, resuming 
before the VM initialization is complete will trigger it.

Thanks
Andrew

Andrew Leonard
Java Runtimes Development
IBM Hursley
IBM United Kingdom Ltd
Phone internal: 245913, external: 01962 815913
internet email: andrew_m_leon...@uk.ibm.com 




From:   "serguei.spit...@oracle.com" <serguei.spit...@oracle.com>
To: Andrew Leonard <andrew_m_leon...@uk.ibm.com>
Cc: serviceability-dev@openjdk.java.net
Date:   11/04/2018 09:57
Subject:    Re: RFR: Fix race condition in jdwp



Hi Andrew,

I've filed the bug:
  https://bugs.openjdk.java.net/browse/JDK-8201409

Also, this is a webrev with your patch:
  
http://cr.openjdk.java.net/~sspitsyn/webrevs/2018/8201409-jdwp-initsync.ibm.1/


I agree that creating a standalone test is tricky here.

I've added usleep(1) into the eventHelper_reportVMInit()
and ran the JTreg com/sun/jdi tests with my JDK build.
However, none of the tests failed with the failure mode you described.
So that I'm puzzled a little bit.
I suspect that some specific debugLoop commands were used in your 
scenario.

It is still possible that I've missed something here.
Will try to double check everything.

Thanks,
Serguei


On 4/11/18 01:29, Andrew Leonard wrote:
Thanks Serguei, 
I terms of a standalone testcase it is quite tricky, as due to the nature 
of the issue which took a lot of investigation to solve it's very timing 
dependent and will only occur randomly. It can be forced as I indicated 
below by adding a "sleep" in the VMInit report code but that's not a 
testcase, however the issue was originally found in our JCK testing for 
IBMJava8, testcase test.jck8b.runtime.vm.jdwp, but again only happened 
intermittently. Sort of like "performance" type issues we're not always 
going to be able to create a testcase that will always "fail" if the fix 
is not present. 
Your thoughts? 
Cheers 
Andrew 

Andrew Leonard
Java Runtimes Development
IBM Hursley
IBM United Kingdom Ltd
Phone internal: 245913, external: 01962 815913
internet email: andrew_m_leon...@uk.ibm.com 




From:"serguei.spit...@oracle.com" <serguei.spit...@oracle.com> 
To:Andrew Leonard <andrew_m_leon...@uk.ibm.com> 
Cc:serviceability-dev@openjdk.java.net 
Date:    11/04/2018 01:02 
Subject:Re: RFR: Fix race condition in jdwp 



Hi Andrew,

Okay, I'll file a bug on this topic.
But do you have a standalone test demonstrating this issue?

Thanks,
Serguei


On 4/10/18 06:23, Andrew Leonard wrote: 
Hi Serguei, 
I don't have access to the bug database to raise one, are you able to 
please? 

Summary: JDWP debugger initialization hangs intermittently 
Description: If during the JDWP setup initialization the VM initialization 
takes slightly longer than the main debug initialization thread a "hang" 
situation can occur. This has been seen in testcase 
test.jck8b.runtime.vm.jdwp and can also be recreated easily by adding a 10 
second sleep to the beginning of the 
src/jdk.jdwp.agent/share/native/libjdwp/eventHelper.c method 
eventHelper_reportVMInit() . 
First seen: JDK8 
Recreated: JDK11 

Thanks 
Andrew 

Andrew Leonard
Java Runtimes Development
IBM Hursley
IBM United Kingdom Ltd
Phone internal: 245913, external: 01962 815913
internet email: andrew_m_leon...@uk.ibm.com 




From:"serguei.spit...@oracle.com" <serguei.spit...@oracle.com> 
To:Andrew Leonard <andrew_m_leon...@uk.ibm.com>, 
serviceability-dev@openjdk.java.net 
Date:09/04/2018 23:03 
Subject:Re: RFR: Fix race condition in jdwp 



Hi Andrew,

The patch itself looks reasonable.
However, in ord

Re: RFR: Fix race condition in jdwp

2018-04-11 Thread serguei.spit...@oracle.com 

  
  
Hi Andrew,
  
  I've filed the bug:
    https://bugs.openjdk.java.net/browse/JDK-8201409
  
  Also, this is a webrev with your patch:
   
http://cr.openjdk.java.net/~sspitsyn/webrevs/2018/8201409-jdwp-initsync.ibm.1/
  
  I agree that creating a standalone test is tricky here.
  
  I've added usleep(1) into the eventHelper_reportVMInit()
  and ran the JTreg com/sun/jdi tests with my JDK build.
  However, none of the tests failed with the failure mode you
  described.
  So that I'm puzzled a little bit.
  I suspect that some specific debugLoop commands were used in your
  scenario.
  
  It is still possible that I've missed something here.
  Will try to double check everything.
  
  Thanks,
  Serguei
  
  
  On 4/11/18 01:29, Andrew Leonard wrote:


  
  Thanks Serguei,
  
  I terms of a standalone
testcase it is quite tricky, as due to the nature of the issue
which took
a lot of investigation to solve it's very timing dependent and
will only
occur randomly. It can be forced as I indicated below by adding
a "sleep"
in the VMInit report code but that's not a testcase, however the
issue
was originally found in our JCK testing for IBMJava8, testcase test.jck8b.runtime.vm.jdwp,
but again only happened intermittently. Sort of like
"performance"
type issues we're not always going to be able to create a
testcase that
will always "fail" if the fix is not present.
  
  Your thoughts?
  
  Cheers
  
  Andrew
  
  
  Andrew Leonard
Java Runtimes Development
IBM Hursley
IBM United Kingdom Ltd
Phone internal: 245913, external: 01962 815913
internet email: andrew_m_leon...@uk.ibm.com 
  
  
  
  
  
  From:
       "serguei.spit...@oracle.com"
<serguei.spit...@oracle.com>
  
  To:
       Andrew
Leonard <andrew_m_leon...@uk.ibm.com>
  
  Cc:
       serviceability-dev@openjdk.java.net
  
  Date:
       11/04/2018
01:02
      
      Subject:
           Re:
RFR: Fix race condition in jdwp
  
  
  
  
  
  Hi Andrew,

Okay, I'll file a bug on this topic.
But do you have a standalone test demonstrating this issue?

Thanks,
Serguei


On 4/10/18 06:23, Andrew Leonard wrote:
  
  Hi Serguei, 
I don't have access to the bug database to raise one, are you
able to please?


Summary: JDWP debugger initialization hangs intermittently 
Description: If during the JDWP setup initialization the VM
initialization
takes slightly longer than the main debug initialization thread
a "hang"
situation can occur. This has been seen in testcase
test.jck8b.runtime.vm.jdwp
and can also be recreated easily by adding a 10 second sleep to
the beginning
of the src/jdk.jdwp.agent/share/native/libjdwp/eventHelper.c
method eventHelper_reportVMInit()
. 
First seen: JDK8 
Recreated: JDK11 

Thanks 
Andrew 

Andrew Leonard
Java Runtimes Development
IBM Hursley
IBM United Kingdom Ltd
Phone internal: 245913, external: 01962 815913
internet email: andrew_m_leon...@uk.ibm.com





From:        "serguei.spit...@oracle.com"
  <serguei.spit...@oracle.com>

To:        Andrew Leonard <andrew_m_leon...@uk.ibm.com>,
  serviceability-dev@openjdk.java.net
    
    Date:        09/04/2018 23:03 
Subject:        Re: RFR: Fix race condition in jdwp

  
  

  
  Hi Andrew,
  
  The patch itself looks reasonable.
  However, in order to proceed with it, a bug report with a
  standalone
  test case demonstrating the issue is needed.
  
  Thanks,
  Serguei
  
  
  On 4/9/18 09:07, Andrew Leonard wrote:
  > Hi,
  > We discovered in our testing with OpenJ9 that a race
  condition can
  
  > occur in the jdwp under certain circumstances, and we
  were able to
  
  > force the same issue with Hotspot. Normally, the event
  helper thread
  
  > suspends all threads, then the debug loop in the listener
  thread 
  > receives a command to resume. The debugger may deadlock
  if the debu

Re: RFR: Fix race condition in jdwp

2018-04-10 Thread serguei.spit...@oracle.com 

  
  
Hi Andrew,
  
  Okay, I'll file a bug on this topic.
  But do you have a standalone test demonstrating this issue?
  
  Thanks,
  Serguei
  
  
  On 4/10/18 06:23, Andrew Leonard wrote:


  
  Hi Serguei,
  
  I don't have access
to the bug database to raise one, are you able to please?
  
  
  Summary: JDWP
debugger initialization hangs intermittently
  
  Description: If
during the JDWP setup initialization the VM initialization takes
slightly
longer than the main debug initialization thread a "hang"
situation
can occur. This has been seen in testcase test.jck8b.runtime.vm.jdwp
  and can also
be recreated easily by adding a 10 second sleep to the beginning
of the
src/jdk.jdwp.agent/share/native/libjdwp/eventHelper.c method eventHelper_reportVMInit()
. 
  
  First seen: JDK8
  
  Recreated: JDK11
  
  
  Thanks
  
  Andrew
  
  
  Andrew Leonard
Java Runtimes Development
IBM Hursley
IBM United Kingdom Ltd
Phone internal: 245913, external: 01962 815913
internet email: andrew_m_leon...@uk.ibm.com 
  
  
  
  
  
  From:
       "serguei.spit...@oracle.com"
<serguei.spit...@oracle.com>
  
  To:
       Andrew
Leonard <andrew_m_leon...@uk.ibm.com>,
serviceability-dev@openjdk.java.net
  
  Date:
       09/04/2018
23:03
  
  Subject:
           Re:
    RFR: Fix race condition in jdwp
  
  
  
  
  
  Hi Andrew,
  
  The patch itself looks reasonable.
  However, in order to proceed with it, a bug report with a
  standalone
  test case demonstrating the issue is needed.
  
  Thanks,
  Serguei
  
  
  On 4/9/18 09:07, Andrew Leonard wrote:
  > Hi,
  > We discovered in our testing with OpenJ9 that a race
  condition can
  
  > occur in the jdwp under certain circumstances, and we
  were able to
  
  > force the same issue with Hotspot. Normally, the event
  helper thread
  
  > suspends all threads, then the debug loop in the listener
  thread 
  > receives a command to resume. The debugger may deadlock
  if the debug
  
  > loop in the listener thread starts processing commands
  (e.g. resume
  
  > threads) before the event helper completes the
  initialization (and
  
  > suspends threads).
  >
  > This patch adds synchronization to ensure the event
  helper completes
  
  > the initialization sequence before debugger commands are
  processed.
  >
  > Please can I find a sponsor for this contribution? Patch
  below..
  >
  > Many thanks
  >
  > Andrew
  >
  >
  >
  > diff --git
  a/src/jdk.jdwp.agent/share/native/libjdwp/debugInit.c 
  > b/src/jdk.jdwp.agent/share/native/libjdwp/debugInit.c
  > --- a/src/jdk.jdwp.agent/share/native/libjdwp/debugInit.c
  > +++ b/src/jdk.jdwp.agent/share/native/libjdwp/debugInit.c
  > @@ -1,5 +1,5 @@
  >  /*
  > - * Copyright (c) 1998, 2017, Oracle and/or its
  affiliates. All rights
  
  > reserved.
  > + * Copyright (c) 1998, 2018, 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
  > @@ -58,6 +58,7 @@
  >  static jboolean vmInitialized;
  >  static jrawMonitorID initMonitor;
  >  static jboolean initComplete;
  > +static jboolean VMInitComplete;
  >  static jbyte currentSessionID;
  >
  >  /*
  > @@ -617,6 +618,35 @@
  >  debugMonitorExit(initMonitor);
  >  }
  >
  > +/*
  > + * Signal VM initialization is complete.
  > + */
  > +void
  > +signalVMInitComplete(void)
  > +{
  > +    /*
  > + * VM Initialization is complete
  > + */
  > +    LOG_MISC(("signal VM initialization complete"));
  > +    debugMonitorEnter(initMonitor);
  > +    VMInitCompl

Re: RFR: Fix race condition in jdwp

2018-04-10 Thread Andrew Leonard 
Hi Serguei,
I don't have access to the bug database to raise one, are you able to 
please?

Summary: JDWP debugger initialization hangs intermittently
Description: If during the JDWP setup initialization the VM initialization 
takes slightly longer than the main debug initialization thread a "hang" 
situation can occur. This has been seen in testcase 
test.jck8b.runtime.vm.jdwp and can also be recreated easily by adding a 10 
second sleep to the beginning of the 
src/jdk.jdwp.agent/share/native/libjdwp/eventHelper.c method 
eventHelper_reportVMInit() . 
First seen: JDK8
Recreated: JDK11

Thanks
Andrew

Andrew Leonard
Java Runtimes Development
IBM Hursley
IBM United Kingdom Ltd
Phone internal: 245913, external: 01962 815913
internet email: andrew_m_leon...@uk.ibm.com 




From:   "serguei.spit...@oracle.com" <serguei.spit...@oracle.com>
To: Andrew Leonard <andrew_m_leon...@uk.ibm.com>, 
serviceability-dev@openjdk.java.net
Date:   09/04/2018 23:03
Subject:    Re: RFR: Fix race condition in jdwp



Hi Andrew,

The patch itself looks reasonable.
However, in order to proceed with it, a bug report with a standalone
test case demonstrating the issue is needed.

Thanks,
Serguei


On 4/9/18 09:07, Andrew Leonard wrote:
> Hi,
> We discovered in our testing with OpenJ9 that a race condition can 
> occur in the jdwp under certain circumstances, and we were able to 
> force the same issue with Hotspot. Normally, the event helper thread 
> suspends all threads, then the debug loop in the listener thread 
> receives a command to resume. The debugger may deadlock if the debug 
> loop in the listener thread starts processing commands (e.g. resume 
> threads) before the event helper completes the initialization (and 
> suspends threads).
>
> This patch adds synchronization to ensure the event helper completes 
> the initialization sequence before debugger commands are processed.
>
> Please can I find a sponsor for this contribution? Patch below..
>
> Many thanks
>
> Andrew
>
>
>
> diff --git a/src/jdk.jdwp.agent/share/native/libjdwp/debugInit.c 
> b/src/jdk.jdwp.agent/share/native/libjdwp/debugInit.c
> --- a/src/jdk.jdwp.agent/share/native/libjdwp/debugInit.c
> +++ b/src/jdk.jdwp.agent/share/native/libjdwp/debugInit.c
> @@ -1,5 +1,5 @@
>  /*
> - * Copyright (c) 1998, 2017, Oracle and/or its affiliates. All rights 
> reserved.
> + * Copyright (c) 1998, 2018, 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
> @@ -58,6 +58,7 @@
>  static jboolean vmInitialized;
>  static jrawMonitorID initMonitor;
>  static jboolean initComplete;
> +static jboolean VMInitComplete;
>  static jbyte currentSessionID;
>
>  /*
> @@ -617,6 +618,35 @@
>  debugMonitorExit(initMonitor);
>  }
>
> +/*
> + * Signal VM initialization is complete.
> + */
> +void
> +signalVMInitComplete(void)
> +{
> +/*
> + * VM Initialization is complete
> + */
> +LOG_MISC(("signal VM initialization complete"));
> +debugMonitorEnter(initMonitor);
> +VMInitComplete = JNI_TRUE;
> +debugMonitorNotifyAll(initMonitor);
> +debugMonitorExit(initMonitor);
> +}
> +
> +/*
> + * Wait for VM initialization to complete.
> + */
> +void
> +debugInit_waitVMInitComplete(void)
> +{
> +debugMonitorEnter(initMonitor);
> +while (!VMInitComplete) {
> +debugMonitorWait(initMonitor);
> +}
> +debugMonitorExit(initMonitor);
> +}
> +
>  /* All process exit() calls come from here */
>  void
>  forceExit(int exit_code)
> @@ -672,6 +702,7 @@
>  LOG_MISC(("Begin initialize()"));
>  currentSessionID = 0;
>  initComplete = JNI_FALSE;
> +VMInitComplete = JNI_FALSE;
>
>  if ( gdata->vmDead ) {
>  EXIT_ERROR(AGENT_ERROR_INTERNAL,"VM dead at initialize() time");
> diff --git a/src/jdk.jdwp.agent/share/native/libjdwp/debugInit.h 
> b/src/jdk.jdwp.agent/share/native/libjdwp/debugInit.h
> --- a/src/jdk.jdwp.agent/share/native/libjdwp/debugInit.h
> +++ b/src/jdk.jdwp.agent/share/native/libjdwp/debugInit.h
> @@ -1,5 +1,5 @@
>  /*
> - * Copyright (c) 1998, 2015, Oracle and/or its affiliates. All rights 
> reserved.
> + * Copyright (c) 1998, 2018, 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
> @@ -39,4 +39,7 @@
>  void debugInit_exit(jvmtiError, const char *);
>  void forceExit(int);
>
> +void debugInit_waitVMInitComplete(void);
> +void signalVMInitComplete(void);
> +
>

Re: RFR: Fix race condition in jdwp

2018-04-10 Thread Andrew Leonard 
Hi David,
The existing "initComplete" in debugInit.c logic is the debug main thread 
initialization complete logic, that does not handle nor needs to wait for 
the asynchronous VM initialization that will be reported but must be 
completed before the debug loop can start processing cmds.
Thanks
Andrew

Andrew Leonard
Java Runtimes Development
IBM Hursley
IBM United Kingdom Ltd
Phone internal: 245913, external: 01962 815913
internet email: andrew_m_leon...@uk.ibm.com 




From:   David Holmes <david.hol...@oracle.com>
To: Andrew Leonard <andrew_m_leon...@uk.ibm.com>, 
serviceability-dev@openjdk.java.net
Date:   10/04/2018 02:06
Subject:    Re: RFR: Fix race condition in jdwp



Hi Andrew,

On 10/04/2018 2:07 AM, Andrew Leonard wrote:
> Hi,
> We discovered in our testing with OpenJ9 that a race condition can occur 

> in the jdwp under certain circumstances, and we were able to force the 
> same issue with Hotspot. Normally, the event helper thread suspends all 
> threads, then the debug loop in the listener thread receives a command 
> to resume. The debugger may deadlock if the debug loop in the listener 
> thread starts processing commands (e.g. resume threads) before the event 

> helper completes the initialization (and suspends threads).
> 
> This patch adds synchronization to ensure the event helper completes the 

> initialization sequence before debugger commands are processed.

How does this relate to the existing debugInit_waitInitComplete? I don't 
see why we would want two initialization synchronization mechanisms. ??

Thanks,
David

> Please can I find a sponsor for this contribution? Patch below..
> 
> Many thanks
> 
> Andrew
> 
> 
> 
> diff --git a/src/jdk.jdwp.agent/share/native/libjdwp/debugInit.c 
> b/src/jdk.jdwp.agent/share/native/libjdwp/debugInit.c
> --- a/src/jdk.jdwp.agent/share/native/libjdwp/debugInit.c
> +++ b/src/jdk.jdwp.agent/share/native/libjdwp/debugInit.c
> @@ -1,5 +1,5 @@
>   /*
> - * Copyright (c) 1998, 2017, Oracle and/or its affiliates. All rights 
> reserved.
> + * Copyright (c) 1998, 2018, 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
> @@ -58,6 +58,7 @@
>   static jboolean vmInitialized;
>   static jrawMonitorID initMonitor;
>   static jboolean initComplete;
> +static jboolean VMInitComplete;
>   static jbyte currentSessionID;
> 
>   /*
> @@ -617,6 +618,35 @@
>   debugMonitorExit(initMonitor);
>   }
> 
> +/*
> + * Signal VM initialization is complete.
> + */
> +void
> +signalVMInitComplete(void)
> +{
> +/*
> + * VM Initialization is complete
> + */
> +LOG_MISC(("signal VM initialization complete"));
> +debugMonitorEnter(initMonitor);
> +VMInitComplete = JNI_TRUE;
> +debugMonitorNotifyAll(initMonitor);
> +debugMonitorExit(initMonitor);
> +}
> +
> +/*
> + * Wait for VM initialization to complete.
> + */
> +void
> +debugInit_waitVMInitComplete(void)
> +{
> +debugMonitorEnter(initMonitor);
> +while (!VMInitComplete) {
> +debugMonitorWait(initMonitor);
> +}
> +debugMonitorExit(initMonitor);
> +}
> +
>   /* All process exit() calls come from here */
>   void
>   forceExit(int exit_code)
> @@ -672,6 +702,7 @@
>   LOG_MISC(("Begin initialize()"));
>   currentSessionID = 0;
>   initComplete = JNI_FALSE;
> +VMInitComplete = JNI_FALSE;
> 
>   if ( gdata->vmDead ) {
>   EXIT_ERROR(AGENT_ERROR_INTERNAL,"VM dead at initialize() time");
> diff --git a/src/jdk.jdwp.agent/share/native/libjdwp/debugInit.h 
> b/src/jdk.jdwp.agent/share/native/libjdwp/debugInit.h
> --- a/src/jdk.jdwp.agent/share/native/libjdwp/debugInit.h
> +++ b/src/jdk.jdwp.agent/share/native/libjdwp/debugInit.h
> @@ -1,5 +1,5 @@
>   /*
> - * Copyright (c) 1998, 2015, Oracle and/or its affiliates. All rights 
> reserved.
> + * Copyright (c) 1998, 2018, 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
> @@ -39,4 +39,7 @@
>   void debugInit_exit(jvmtiError, const char *);
>   void forceExit(int);
> 
> +void debugInit_waitVMInitComplete(void);
> +void signalVMInitComplete(void);
> +
>   #endif
> diff --git a/src/jdk.jdwp.agent/share/native/libjdwp/debugLoop.c 
> b/src/jdk.jdwp.agent/share/native/libjdwp/debugLoop.c
> --- a/src/jdk.jdwp.agent/share/native/libjdwp/debugLoop.c
> +++ b/src/jdk.jdwp.agent/share/native/libjdwp/debugLoop.c
> @@ -1,5 +1,5

Re: RFR: Fix race condition in jdwp

2018-04-09 Thread David Holmes 

Hi Andrew,

On 10/04/2018 2:07 AM, Andrew Leonard wrote:

Hi,
We discovered in our testing with OpenJ9 that a race condition can occur 
in the jdwp under certain circumstances, and we were able to force the 
same issue with Hotspot. Normally, the event helper thread suspends all 
threads, then the debug loop in the listener thread receives a command 
to resume. The debugger may deadlock if the debug loop in the listener 
thread starts processing commands (e.g. resume threads) before the event 
helper completes the initialization (and suspends threads).


This patch adds synchronization to ensure the event helper completes the 
initialization sequence before debugger commands are processed.


How does this relate to the existing debugInit_waitInitComplete? I don't 
see why we would want two initialization synchronization mechanisms. ??


Thanks,
David


Please can I find a sponsor for this contribution? Patch below..

Many thanks

Andrew



diff --git a/src/jdk.jdwp.agent/share/native/libjdwp/debugInit.c 
b/src/jdk.jdwp.agent/share/native/libjdwp/debugInit.c

--- a/src/jdk.jdwp.agent/share/native/libjdwp/debugInit.c
+++ b/src/jdk.jdwp.agent/share/native/libjdwp/debugInit.c
@@ -1,5 +1,5 @@
  /*
- * Copyright (c) 1998, 2017, Oracle and/or its affiliates. All rights 
reserved.
+ * Copyright (c) 1998, 2018, 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
@@ -58,6 +58,7 @@
  static jboolean vmInitialized;
  static jrawMonitorID initMonitor;
  static jboolean initComplete;
+static jboolean VMInitComplete;
  static jbyte currentSessionID;

  /*
@@ -617,6 +618,35 @@
  debugMonitorExit(initMonitor);
  }

+/*
+ * Signal VM initialization is complete.
+ */
+void
+signalVMInitComplete(void)
+{
+    /*
+ * VM Initialization is complete
+ */
+    LOG_MISC(("signal VM initialization complete"));
+    debugMonitorEnter(initMonitor);
+    VMInitComplete = JNI_TRUE;
+    debugMonitorNotifyAll(initMonitor);
+    debugMonitorExit(initMonitor);
+}
+
+/*
+ * Wait for VM initialization to complete.
+ */
+void
+debugInit_waitVMInitComplete(void)
+{
+    debugMonitorEnter(initMonitor);
+    while (!VMInitComplete) {
+    debugMonitorWait(initMonitor);
+    }
+    debugMonitorExit(initMonitor);
+}
+
  /* All process exit() calls come from here */
  void
  forceExit(int exit_code)
@@ -672,6 +702,7 @@
  LOG_MISC(("Begin initialize()"));
  currentSessionID = 0;
  initComplete = JNI_FALSE;
+    VMInitComplete = JNI_FALSE;

  if ( gdata->vmDead ) {
      EXIT_ERROR(AGENT_ERROR_INTERNAL,"VM dead at initialize() time");
diff --git a/src/jdk.jdwp.agent/share/native/libjdwp/debugInit.h 
b/src/jdk.jdwp.agent/share/native/libjdwp/debugInit.h

--- a/src/jdk.jdwp.agent/share/native/libjdwp/debugInit.h
+++ b/src/jdk.jdwp.agent/share/native/libjdwp/debugInit.h
@@ -1,5 +1,5 @@
  /*
- * Copyright (c) 1998, 2015, Oracle and/or its affiliates. All rights 
reserved.
+ * Copyright (c) 1998, 2018, 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
@@ -39,4 +39,7 @@
  void debugInit_exit(jvmtiError, const char *);
  void forceExit(int);

+void debugInit_waitVMInitComplete(void);
+void signalVMInitComplete(void);
+
  #endif
diff --git a/src/jdk.jdwp.agent/share/native/libjdwp/debugLoop.c 
b/src/jdk.jdwp.agent/share/native/libjdwp/debugLoop.c

--- a/src/jdk.jdwp.agent/share/native/libjdwp/debugLoop.c
+++ b/src/jdk.jdwp.agent/share/native/libjdwp/debugLoop.c
@@ -1,5 +1,5 @@
  /*
- * Copyright (c) 1998, 2017, Oracle and/or its affiliates. All rights 
reserved.
+ * Copyright (c) 1998, 2018, 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
@@ -98,6 +98,7 @@
  standardHandlers_onConnect();
  threadControl_onConnect();

+    debugInit_waitVMInitComplete();
  /* Okay, start reading cmds! */
  while (shouldListen) {
      if (!dequeue()) {
diff --git a/src/jdk.jdwp.agent/share/native/libjdwp/eventHelper.c 
b/src/jdk.jdwp.agent/share/native/libjdwp/eventHelper.c

--- a/src/jdk.jdwp.agent/share/native/libjdwp/eventHelper.c
+++ b/src/jdk.jdwp.agent/share/native/libjdwp/eventHelper.c
@@ -1,5 +1,5 @@
  /*
- * Copyright (c) 1998, 2017, Oracle and/or its affiliates. All rights 
reserved.
+ * Copyright (c) 1998, 2018, 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
@@ -580,6 +580,7 @@
      (void)threadControl_suspendThread(command->thread, JNI_FALSE);
  }

+    signalVMInitComplete();
  outStream_initCommand(, uniqueID(), 0x0,
  JDWP_COMMAND_SET(Event),
  JDWP_COMMAND(Event,

Re: RFR: Fix race condition in jdwp

2018-04-09 Thread serguei.spit...@oracle.com 

Hi Andrew,

The patch itself looks reasonable.
However, in order to proceed with it, a bug report with a standalone
test case demonstrating the issue is needed.

Thanks,
Serguei


On 4/9/18 09:07, Andrew Leonard wrote:

Hi,
We discovered in our testing with OpenJ9 that a race condition can 
occur in the jdwp under certain circumstances, and we were able to 
force the same issue with Hotspot. Normally, the event helper thread 
suspends all threads, then the debug loop in the listener thread 
receives a command to resume. The debugger may deadlock if the debug 
loop in the listener thread starts processing commands (e.g. resume 
threads) before the event helper completes the initialization (and 
suspends threads).


This patch adds synchronization to ensure the event helper completes 
the initialization sequence before debugger commands are processed.


Please can I find a sponsor for this contribution? Patch below..

Many thanks

Andrew



diff --git a/src/jdk.jdwp.agent/share/native/libjdwp/debugInit.c 
b/src/jdk.jdwp.agent/share/native/libjdwp/debugInit.c

--- a/src/jdk.jdwp.agent/share/native/libjdwp/debugInit.c
+++ b/src/jdk.jdwp.agent/share/native/libjdwp/debugInit.c
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 1998, 2017, Oracle and/or its affiliates. All rights 
reserved.
+ * Copyright (c) 1998, 2018, 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
@@ -58,6 +58,7 @@
 static jboolean vmInitialized;
 static jrawMonitorID initMonitor;
 static jboolean initComplete;
+static jboolean VMInitComplete;
 static jbyte currentSessionID;

 /*
@@ -617,6 +618,35 @@
 debugMonitorExit(initMonitor);
 }

+/*
+ * Signal VM initialization is complete.
+ */
+void
+signalVMInitComplete(void)
+{
+    /*
+ * VM Initialization is complete
+ */
+    LOG_MISC(("signal VM initialization complete"));
+    debugMonitorEnter(initMonitor);
+    VMInitComplete = JNI_TRUE;
+    debugMonitorNotifyAll(initMonitor);
+    debugMonitorExit(initMonitor);
+}
+
+/*
+ * Wait for VM initialization to complete.
+ */
+void
+debugInit_waitVMInitComplete(void)
+{
+    debugMonitorEnter(initMonitor);
+    while (!VMInitComplete) {
+    debugMonitorWait(initMonitor);
+    }
+    debugMonitorExit(initMonitor);
+}
+
 /* All process exit() calls come from here */
 void
 forceExit(int exit_code)
@@ -672,6 +702,7 @@
 LOG_MISC(("Begin initialize()"));
 currentSessionID = 0;
 initComplete = JNI_FALSE;
+    VMInitComplete = JNI_FALSE;

 if ( gdata->vmDead ) {
     EXIT_ERROR(AGENT_ERROR_INTERNAL,"VM dead at initialize() time");
diff --git a/src/jdk.jdwp.agent/share/native/libjdwp/debugInit.h 
b/src/jdk.jdwp.agent/share/native/libjdwp/debugInit.h

--- a/src/jdk.jdwp.agent/share/native/libjdwp/debugInit.h
+++ b/src/jdk.jdwp.agent/share/native/libjdwp/debugInit.h
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 1998, 2015, Oracle and/or its affiliates. All rights 
reserved.
+ * Copyright (c) 1998, 2018, 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
@@ -39,4 +39,7 @@
 void debugInit_exit(jvmtiError, const char *);
 void forceExit(int);

+void debugInit_waitVMInitComplete(void);
+void signalVMInitComplete(void);
+
 #endif
diff --git a/src/jdk.jdwp.agent/share/native/libjdwp/debugLoop.c 
b/src/jdk.jdwp.agent/share/native/libjdwp/debugLoop.c

--- a/src/jdk.jdwp.agent/share/native/libjdwp/debugLoop.c
+++ b/src/jdk.jdwp.agent/share/native/libjdwp/debugLoop.c
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 1998, 2017, Oracle and/or its affiliates. All rights 
reserved.
+ * Copyright (c) 1998, 2018, 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
@@ -98,6 +98,7 @@
 standardHandlers_onConnect();
 threadControl_onConnect();

+    debugInit_waitVMInitComplete();
 /* Okay, start reading cmds! */
 while (shouldListen) {
     if (!dequeue()) {
diff --git a/src/jdk.jdwp.agent/share/native/libjdwp/eventHelper.c 
b/src/jdk.jdwp.agent/share/native/libjdwp/eventHelper.c

--- a/src/jdk.jdwp.agent/share/native/libjdwp/eventHelper.c
+++ b/src/jdk.jdwp.agent/share/native/libjdwp/eventHelper.c
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 1998, 2017, Oracle and/or its affiliates. All rights 
reserved.
+ * Copyright (c) 1998, 2018, 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
@@ -580,6 +580,7 @@
     (void)threadControl_suspendThread(command->thread, JNI_FALSE);
 }

+    signalVMInitComplete();
 outStream_initCommand(, uniqueID(), 0x0,
 JDWP_COMMAND_SET(Event),
 JDWP_COMMAND(Event, Composite));



Andrew Leonard
Java Runtimes

RFR: Fix race condition in jdwp

2018-04-09 Thread Andrew Leonard 
Hi,
We discovered in our testing with OpenJ9 that a race condition can occur 
in the jdwp under certain circumstances, and we were able to force the 
same issue with Hotspot. Normally, the event helper thread suspends all 
threads, then the debug loop in the listener thread receives a command to 
resume. The debugger may deadlock if the debug loop in the listener thread 
starts processing commands (e.g. resume threads) before the event helper 
completes the initialization (and suspends threads).
This patch adds synchronization to ensure the event helper completes the 
initialization sequence before debugger commands are processed.

Please can I find a sponsor for this contribution? Patch below..
Many thanks
Andrew

diff --git a/src/jdk.jdwp.agent/share/native/libjdwp/debugInit.c 
b/src/jdk.jdwp.agent/share/native/libjdwp/debugInit.c
--- a/src/jdk.jdwp.agent/share/native/libjdwp/debugInit.c
+++ b/src/jdk.jdwp.agent/share/native/libjdwp/debugInit.c
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 1998, 2017, Oracle and/or its affiliates. All rights 
reserved.
+ * Copyright (c) 1998, 2018, 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
@@ -58,6 +58,7 @@
 static jboolean vmInitialized;
 static jrawMonitorID initMonitor;
 static jboolean initComplete;
+static jboolean VMInitComplete;
 static jbyte currentSessionID;
 
 /*
@@ -617,6 +618,35 @@
 debugMonitorExit(initMonitor);
 }
 
+/*
+ * Signal VM initialization is complete.
+ */
+void
+signalVMInitComplete(void)
+{
+/*
+ * VM Initialization is complete
+ */
+LOG_MISC(("signal VM initialization complete"));
+debugMonitorEnter(initMonitor);
+VMInitComplete = JNI_TRUE;
+debugMonitorNotifyAll(initMonitor);
+debugMonitorExit(initMonitor);
+}
+
+/*
+ * Wait for VM initialization to complete.
+ */
+void
+debugInit_waitVMInitComplete(void)
+{
+debugMonitorEnter(initMonitor);
+while (!VMInitComplete) {
+debugMonitorWait(initMonitor);
+}
+debugMonitorExit(initMonitor);
+}
+
 /* All process exit() calls come from here */
 void
 forceExit(int exit_code)
@@ -672,6 +702,7 @@
 LOG_MISC(("Begin initialize()"));
 currentSessionID = 0;
 initComplete = JNI_FALSE;
+VMInitComplete = JNI_FALSE;
 
 if ( gdata->vmDead ) {
 EXIT_ERROR(AGENT_ERROR_INTERNAL,"VM dead at initialize() time");
diff --git a/src/jdk.jdwp.agent/share/native/libjdwp/debugInit.h 
b/src/jdk.jdwp.agent/share/native/libjdwp/debugInit.h
--- a/src/jdk.jdwp.agent/share/native/libjdwp/debugInit.h
+++ b/src/jdk.jdwp.agent/share/native/libjdwp/debugInit.h
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 1998, 2015, Oracle and/or its affiliates. All rights 
reserved.
+ * Copyright (c) 1998, 2018, 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
@@ -39,4 +39,7 @@
 void debugInit_exit(jvmtiError, const char *);
 void forceExit(int);
 
+void debugInit_waitVMInitComplete(void);
+void signalVMInitComplete(void);
+
 #endif
diff --git a/src/jdk.jdwp.agent/share/native/libjdwp/debugLoop.c 
b/src/jdk.jdwp.agent/share/native/libjdwp/debugLoop.c
--- a/src/jdk.jdwp.agent/share/native/libjdwp/debugLoop.c
+++ b/src/jdk.jdwp.agent/share/native/libjdwp/debugLoop.c
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 1998, 2017, Oracle and/or its affiliates. All rights 
reserved.
+ * Copyright (c) 1998, 2018, 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
@@ -98,6 +98,7 @@
 standardHandlers_onConnect();
 threadControl_onConnect();
 
+debugInit_waitVMInitComplete();
 /* Okay, start reading cmds! */
 while (shouldListen) {
 if (!dequeue()) {
diff --git a/src/jdk.jdwp.agent/share/native/libjdwp/eventHelper.c 
b/src/jdk.jdwp.agent/share/native/libjdwp/eventHelper.c
--- a/src/jdk.jdwp.agent/share/native/libjdwp/eventHelper.c
+++ b/src/jdk.jdwp.agent/share/native/libjdwp/eventHelper.c
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 1998, 2017, Oracle and/or its affiliates. All rights 
reserved.
+ * Copyright (c) 1998, 2018, 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
@@ -580,6 +580,7 @@
 (void)threadControl_suspendThread(command->thread, JNI_FALSE);
 }
 
+signalVMInitComplete();
 outStream_initCommand(, uniqueID(), 0x0,
   JDWP_COMMAND_SET(Event),
   JDWP_COMMAND(Event, Composite));



Andrew Leonard
Java Runtimes Development
IBM Hursley
IBM United Kingdom Ltd
Phone internal: 245913, external: 01962 815913
internet email: