Re: 8220175: serviceability/dcmd/framework/VMVersionTest.java fails with a timeout

[Gary Adams]

Any chance this will also fix JDK-8226367 ?

On 6/20/19, 10:39 PM, Chris Plummer wrote:

Thanks for looking into this.

Chris

On 6/20/19 6:42 PM, Daniil Titov wrote:

Thank you, Chris and Serguei, for reviewing this change.

I did more testing with a test program on Linux that repeats 
get_namespace_pid() method and reads from the real file ( the copy of 
/proc//status).
I paused the program after the first several lines ( but not "NSpid:" 
line)  where processed and deleted all lines in the status file 
before " NSpid: ..." line in order
to this line became the first in the edited file. After that the 
program continues in the while loop but it seems as the original file 
content was
already buffered and the program just continues over the original 
unedited lines and successfully found the match.


Best regards,
Daniil

On 6/19/19, 9:13 PM, "Chris Plummer"  wrote:

 Hi Daniil,
  I think your fix is good, although I wonder about the 
robustness of this
 function given that the status file can change while it is 
reading it. I

 wonder if it can return a false negative because it never saw the
 matching line. This could happen if a line gets deleted, causing 
the
 matching line to suddenly be earlier in the file, possibly 
before the
 current location being read. Anyway, that's not really related 
to the
 current issue or fix, but if you think it might be an issue 
maybe a bug

 should be filed for it.
  thanks,
  Chris
  On 6/19/19 9:02 PM, Daniil Titov wrote:
> Please review the change that fixes an intermittent failure of 
serviceability/dcmd/framework/* tests on Linux platform.

>
> The problem here is that get_namespace_pid() method, that is called 
by mmap_attach_shared () that in turn is called by PerfMemory::attach(),
> tries to read the namespace pid information from /proc//status 
file. However, it doesn't check that the error indicator associated with
> stream is set that results in the endless  loop (lines 664-677) if 
the process terminates after /proc//status was opened (line 659)

> and checked for null (line 661).
>
>658  snprintf(fname, sizeof(fname), "/proc/%d/status", vmid);
> 659  FILE *fp = fopen(fname, "r");
> 660
> 661  if (fp) {
> 662int pid, nspid;
> 663int ret;
> 664while (!feof(fp)) {
> 665  ret = fscanf(fp, "NSpid: %d %d", , );
> 666  if (ret == 1) {
> 667break;
> 668  }
> 669  if (ret == 2) {
> 670retpid = nspid;
> 671break;
> 672  }
> 673  for (;;) {
> 674int ch = fgetc(fp);
> 675if (ch == EOF || ch == (int)'\n') break;
> 676  }
> 677}
> 678fclose(fp);
> 679  }
>
> The fix adds the check for the error indicator to ensure that the 
"while" loop terminates properly if the file no longer exists.

>
> Issues [3] and [4] have the same cause and will be closed as 
duplicates of this issue.

>
> Testing: Mach5 hotspot_serviceability tests succeeded, tier1,tier2, 
and tier3 tests are in progress.

>
> [1] Webrev: http://cr.openjdk.java.net/~dtitov/8220175/webrev.01/
> [2] Bug: https://bugs.openjdk.java.net/browse/JDK-8220175
> [3] https://bugs.openjdk.java.net/browse/JDK-8223600
> [4] https://bugs.openjdk.java.net/browse/JDK-8217351
>
> Thanks!
> -Daniil
>
>

Re: RFR: JDK-8224642: Test sun/tools/jcmd/TestJcmdSanity.java fails: Bad file descriptor

[Gary Adams]

It is not a new flag.
It is the socket fd.
This aligns with the hPipe used in the windows impl.

On 6/19/19, 12:46 PM, Chris Plummer wrote:

Hi Gary,

Is there a reason you've chosen an int flag rather than a boolean? 
Other than that the changes look fine.


thanks,

Chris

On 6/19/19 7:31 AM, Gary Adams wrote:

That would be consistent with the windows detach() synchronization.

Updated patch is attached.

On 6/19/19, 8:14 AM, Langer, Christoph wrote:

Hi Gary,

looks good overall. I however think the block should also be 
synchronized to avoid issues when multiple threads attempt to close 
the stream.


Cheers
Christoph


-Original Message-
From: Gary Adams
Sent: Mittwoch, 19. Juni 2019 13:59
To: Langer, Christoph
Cc: OpenJDK Serviceability;
Schmelter, Ralf
Subject: Re: RFR: JDK-8224642: Test sun/tools/jcmd/TestJcmdSanity.java
fails: Bad file descriptor

I think everyone is in agreement now that preventing the double close
is the best way to handle this failure. If there are no further 
comments,

I'll push the attached patch on Thurs morning to the jdk/jdk repos.

I'll also close JDK-8223361 as a duplicate.

On 6/19/19, 2:36 AM, Langer, Christoph wrote:

Hi Gary,

I think overall it would be better to fix the InputStream to be 
tolerant to
multiple calls to close, as Ralf pointed out. Maybe someone else on 
some

other place runs into this again because he/she relies on the correct
implementation of Closeable.
However, as a quick fix I can also imagine to do use a single 
resource like

this:

try (InputStreamReader isr = new

InputStreamReader(hvm.executeJCmd(line), "UTF-8")) {

Then we'd also have a single close call per instance.

But if you do that, you should at least open a bug to track fixing 
of the

InputStream implementation...

Best regards
Christoph


-Original Message-
From: 
serviceability-dev

On

Behalf Of gary.ad...@oracle.com
Sent: Dienstag, 18. Juni 2019 12:08
To: OpenJDK Serviceability
Subject: RFR: JDK-8224642: Test 
sun/tools/jcmd/TestJcmdSanity.java fails:

Bad file descriptor

The workaround below passed 1000 testruns on linux-x64-debug.

A more localized fix simply moves the stream reader out of the
try with resources, so only one close is applied to the underlying
socket. I'll run this test through 1000 testruns today.

Looking for a final review for this change.

diff --git a/src/jdk.jcmd/share/classes/sun/tools/jcmd/JCmd.java
b/src/jdk.jcmd/share/classes/sun/tools/jcmd/JCmd.java
--- a/src/jdk.jcmd/share/classes/sun/tools/jcmd/JCmd.java
+++ b/src/jdk.jcmd/share/classes/sun/tools/jcmd/JCmd.java
@@ -122,8 +122,8 @@
if (line.trim().equals("stop")) {
break;
}
-try (InputStream in = hvm.executeJCmd(line);
- InputStreamReader isr = new InputStreamReader(in,
"UTF-8")) {
+try (InputStream in = hvm.executeJCmd(line)) {
+InputStreamReader isr = new 
InputStreamReader(in, "UTF-8");

// read to EOF and just print output
char c[] = new char[256];
    int n;

On 6/17/19 3:23 PM, Gary Adams wrote:

https://bugs.openjdk.java.net/browse/JDK-8224642

I may have a handle on what is going wrong with the
TestJcmdSanity test and the bad file descriptor.

A change made in April 2019 placed the input stream and reader
within the same try with resources block. This has the effect of
calling the
SocketInputStream close method twice for each command processed.

https://bugs.openjdk.java.net/browse/JDK-8222491
http://hg.openjdk.java.net/jdk/jdk/rev/4224f26b2e7f

The last set of tests in the TestJcmdSanity test attempts to 
process ~100

VM.version commands in a loop. Since the closes are handled
when the objects are collected it may come at an inopportune time.

I'm testing the fix below to ensure a second close becomes a noop.
It may be better to revisit the earlier change that set up the 
double

close calls.

diff --git
a/src/jdk.attach/linux/classes/sun/tools/attach/VirtualMachineImpl.java 



b/src/jdk.attach/linux/classes/sun/tools/attach/VirtualMachineImpl.java 


---
a/src/jdk.attach/linux/classes/sun/tools/attach/VirtualMachineImpl.java 


+++

b/src/jdk.attach/linux/classes/sun/tools/attach/VirtualMachineImpl.java 


@@ -233,7 +233,7 @@
* InputStream for the socket connection to get target VM
*/
   private class SocketInputStream extends InputStream {
-int s;
+int s = -1;

   public SocketInputStream(int s) {
   this.s = s;
@@ -261,7 +261,10 @@
   }

   public void close() throws IOException {
+if (s != -1) {
   VirtualMachineImpl.close(s);
+s = -1;
+}
   }
   }

Re: RFR: JDK-8224642: Test sun/tools/jcmd/TestJcmdSanity.java fails: Bad file descriptor

[Gary Adams]

That would be consistent with the windows detach() synchronization.

Updated patch is attached.

On 6/19/19, 8:14 AM, Langer, Christoph wrote:

Hi Gary,

looks good overall. I however think the block should also be synchronized to 
avoid issues when multiple threads attempt to close the stream.

Cheers
Christoph


-Original Message-
From: Gary Adams
Sent: Mittwoch, 19. Juni 2019 13:59
To: Langer, Christoph
Cc: OpenJDK Serviceability;
Schmelter, Ralf
Subject: Re: RFR: JDK-8224642: Test sun/tools/jcmd/TestJcmdSanity.java
fails: Bad file descriptor

I think everyone is in agreement now that preventing the double close
is the best way to handle this failure. If there are no further comments,
I'll push the attached patch on Thurs morning to the jdk/jdk repos.

I'll also close JDK-8223361 as a duplicate.

On 6/19/19, 2:36 AM, Langer, Christoph wrote:

Hi Gary,

I think overall it would be better to fix the InputStream to be tolerant to

multiple calls to close, as Ralf pointed out. Maybe someone else on some
other place runs into this again because he/she relies on the correct
implementation of Closeable.

However, as a quick fix I can also imagine to do use a single resource like

this:

try (InputStreamReader isr = new

InputStreamReader(hvm.executeJCmd(line), "UTF-8")) {

Then we'd also have a single close call per instance.

But if you do that, you should at least open a bug to track fixing of the

InputStream implementation...

Best regards
Christoph


-Original Message-
From: serviceability-dev

On

Behalf Of gary.ad...@oracle.com
Sent: Dienstag, 18. Juni 2019 12:08
To: OpenJDK Serviceability
Subject: RFR: JDK-8224642: Test sun/tools/jcmd/TestJcmdSanity.java fails:
Bad file descriptor

The workaround below passed 1000 testruns on linux-x64-debug.

A more localized fix simply moves the stream reader out of the
try with resources, so only one close is applied to the underlying
socket. I'll run this test through 1000 testruns today.

Looking for a final review for this change.

diff --git a/src/jdk.jcmd/share/classes/sun/tools/jcmd/JCmd.java
b/src/jdk.jcmd/share/classes/sun/tools/jcmd/JCmd.java
--- a/src/jdk.jcmd/share/classes/sun/tools/jcmd/JCmd.java
+++ b/src/jdk.jcmd/share/classes/sun/tools/jcmd/JCmd.java
@@ -122,8 +122,8 @@
if (line.trim().equals("stop")) {
break;
}
-try (InputStream in = hvm.executeJCmd(line);
- InputStreamReader isr = new InputStreamReader(in,
"UTF-8")) {
+try (InputStream in = hvm.executeJCmd(line)) {
+InputStreamReader isr = new InputStreamReader(in, "UTF-8");
// read to EOF and just print output
char c[] = new char[256];
    int n;

On 6/17/19 3:23 PM, Gary Adams wrote:

https://bugs.openjdk.java.net/browse/JDK-8224642

I may have a handle on what is going wrong with the
TestJcmdSanity test and the bad file descriptor.

A change made in April 2019 placed the input stream and reader
within the same try with resources block. This has the effect of
calling the
SocketInputStream close method twice for each command processed.

https://bugs.openjdk.java.net/browse/JDK-8222491
http://hg.openjdk.java.net/jdk/jdk/rev/4224f26b2e7f

The last set of tests in the TestJcmdSanity test attempts to process ~100
VM.version commands in a loop. Since the closes are handled
when the objects are collected it may come at an inopportune time.

I'm testing the fix below to ensure a second close becomes a noop.
It may be better to revisit the earlier change that set up the double
close calls.

diff --git
a/src/jdk.attach/linux/classes/sun/tools/attach/VirtualMachineImpl.java


b/src/jdk.attach/linux/classes/sun/tools/attach/VirtualMachineImpl.java

---
a/src/jdk.attach/linux/classes/sun/tools/attach/VirtualMachineImpl.java
+++


b/src/jdk.attach/linux/classes/sun/tools/attach/VirtualMachineImpl.java

@@ -233,7 +233,7 @@
* InputStream for the socket connection to get target VM
*/
   private class SocketInputStream extends InputStream {
-int s;
+int s = -1;

   public SocketInputStream(int s) {
   this.s = s;
@@ -261,7 +261,10 @@
   }

   public void close() throws IOException {
+if (s != -1) {
   VirtualMachineImpl.close(s);
+s = -1;
+}
   }
   }


# HG changeset patch
# User gadams
# Date 1560954512 14400
#  Wed Jun 19 10:28:32 2019 -0400
# Node ID 40b15f2605afc70922041cf544aae3559ad53068
# Parent  e9da3a44a7edc0222e727b83bd758b9e43176cee
8224642: Test sun/tools/jcmd/TestJcmdSanity.java fails: Bad file descriptor
Reviewed-by: cjplummer, rschmelter, clanger

diff --git 
a/src/jdk.attach/linux/classes/sun/tools/attach/VirtualMachineImpl.java 
b/src/jdk.attach/linux/classes/sun/tools/attach/VirtualMachineImpl.java
--- a/src/jdk.attach/linux/classes/

Re: RFR: JDK-8224642: Test sun/tools/jcmd/TestJcmdSanity.java fails: Bad file descriptor

[Gary Adams]

I think everyone is in agreement now that preventing the double close
is the best way to handle this failure. If there are no further comments,
I'll push the attached patch on Thurs morning to the jdk/jdk repos.

I'll also close JDK-8223361 as a duplicate.

On 6/19/19, 2:36 AM, Langer, Christoph wrote:

Hi Gary,

I think overall it would be better to fix the InputStream to be tolerant to 
multiple calls to close, as Ralf pointed out. Maybe someone else on some other 
place runs into this again because he/she relies on the correct implementation 
of Closeable.

However, as a quick fix I can also imagine to do use a single resource like 
this:

try (InputStreamReader isr = new InputStreamReader(hvm.executeJCmd(line), 
"UTF-8")) {

Then we'd also have a single close call per instance.

But if you do that, you should at least open a bug to track fixing of the 
InputStream implementation...

Best regards
Christoph


-Original Message-
From: serviceability-dev  On
Behalf Of gary.ad...@oracle.com
Sent: Dienstag, 18. Juni 2019 12:08
To: OpenJDK Serviceability
Subject: RFR: JDK-8224642: Test sun/tools/jcmd/TestJcmdSanity.java fails:
Bad file descriptor

The workaround below passed 1000 testruns on linux-x64-debug.

A more localized fix simply moves the stream reader out of the
try with resources, so only one close is applied to the underlying
socket. I'll run this test through 1000 testruns today.

Looking for a final review for this change.

diff --git a/src/jdk.jcmd/share/classes/sun/tools/jcmd/JCmd.java
b/src/jdk.jcmd/share/classes/sun/tools/jcmd/JCmd.java
--- a/src/jdk.jcmd/share/classes/sun/tools/jcmd/JCmd.java
+++ b/src/jdk.jcmd/share/classes/sun/tools/jcmd/JCmd.java
@@ -122,8 +122,8 @@
   if (line.trim().equals("stop")) {
   break;
   }
-try (InputStream in = hvm.executeJCmd(line);
- InputStreamReader isr = new InputStreamReader(in,
"UTF-8")) {
+try (InputStream in = hvm.executeJCmd(line)) {
+InputStreamReader isr = new InputStreamReader(in, "UTF-8");
   // read to EOF and just print output
   char c[] = new char[256];
       int n;

On 6/17/19 3:23 PM, Gary Adams wrote:

https://bugs.openjdk.java.net/browse/JDK-8224642

I may have a handle on what is going wrong with the
TestJcmdSanity test and the bad file descriptor.

A change made in April 2019 placed the input stream and reader
within the same try with resources block. This has the effect of
calling the
SocketInputStream close method twice for each command processed.

https://bugs.openjdk.java.net/browse/JDK-8222491
   http://hg.openjdk.java.net/jdk/jdk/rev/4224f26b2e7f

The last set of tests in the TestJcmdSanity test attempts to process ~100
VM.version commands in a loop. Since the closes are handled
when the objects are collected it may come at an inopportune time.

I'm testing the fix below to ensure a second close becomes a noop.
It may be better to revisit the earlier change that set up the double
close calls.

diff --git
a/src/jdk.attach/linux/classes/sun/tools/attach/VirtualMachineImpl.java
b/src/jdk.attach/linux/classes/sun/tools/attach/VirtualMachineImpl.java
---
a/src/jdk.attach/linux/classes/sun/tools/attach/VirtualMachineImpl.java
+++
b/src/jdk.attach/linux/classes/sun/tools/attach/VirtualMachineImpl.java
@@ -233,7 +233,7 @@
   * InputStream for the socket connection to get target VM
   */
  private class SocketInputStream extends InputStream {
-int s;
+int s = -1;

  public SocketInputStream(int s) {
  this.s = s;
@@ -261,7 +261,10 @@
  }

  public void close() throws IOException {
+if (s != -1) {
  VirtualMachineImpl.close(s);
+s = -1;
+}
  }
  }


# HG changeset patch
# User gadams
# Date 1560945060 14400
#  Wed Jun 19 07:51:00 2019 -0400
# Node ID ff79eb44347a5aed75ddd8d536ffcf784384c126
# Parent  e0be41293b41975cd8e20c5d63ad0e176e12df3c
8224642: Test sun/tools/jcmd/TestJcmdSanity.java fails: Bad file descriptor
Reviewed-by: cjplummer, rschmelter, clanger

diff --git 
a/src/jdk.attach/linux/classes/sun/tools/attach/VirtualMachineImpl.java 
b/src/jdk.attach/linux/classes/sun/tools/attach/VirtualMachineImpl.java
--- a/src/jdk.attach/linux/classes/sun/tools/attach/VirtualMachineImpl.java
+++ b/src/jdk.attach/linux/classes/sun/tools/attach/VirtualMachineImpl.java
@@ -233,7 +233,7 @@
  * InputStream for the socket connection to get target VM
  */
 private class SocketInputStream extends InputStream {
-int s;
+int s = -1;
 
 public SocketInputStream(int s) {
 this.s = s;
@@ -261,7 +261,10 @@
 }
 
 public void close() throws IOException {
-VirtualMachineImpl.close(s);
+if (s != -1) {
+VirtualMachineImpl.close(s);
+s = -1;
+}
 }
 }
 

JDK-8224642: Test sun/tools/jcmd/TestJcmdSanity.java fails: Bad file descriptor

[Gary Adams]

https://bugs.openjdk.java.net/browse/JDK-8224642

I may have a handle on what is going wrong with the
TestJcmdSanity test and the bad file descriptor.

A change made in April 2019 placed the input stream and reader
within the same try with resources block. This has the effect of calling 
the

SocketInputStream close method twice for each command processed.

https://bugs.openjdk.java.net/browse/JDK-8222491
  http://hg.openjdk.java.net/jdk/jdk/rev/4224f26b2e7f

The last set of tests in the TestJcmdSanity test attempts to process ~100
VM.version commands in a loop. Since the closes are handled
when the objects are collected it may come at an inopportune time.

I'm testing the fix below to ensure a second close becomes a noop.
It may be better to revisit the earlier change that set up the double 
close calls.


diff --git 
a/src/jdk.attach/linux/classes/sun/tools/attach/VirtualMachineImpl.java 
b/src/jdk.attach/linux/classes/sun/tools/attach/VirtualMachineImpl.java

--- a/src/jdk.attach/linux/classes/sun/tools/attach/VirtualMachineImpl.java
+++ b/src/jdk.attach/linux/classes/sun/tools/attach/VirtualMachineImpl.java
@@ -233,7 +233,7 @@
  * InputStream for the socket connection to get target VM
  */
 private class SocketInputStream extends InputStream {
-int s;
+int s = -1;

 public SocketInputStream(int s) {
 this.s = s;
@@ -261,7 +261,10 @@
 }

 public void close() throws IOException {
+if (s != -1) {
 VirtualMachineImpl.close(s);
+s = -1;
+}
 }
 }

Re: [13] RFR: JDK-8225682: Reference to JNI spec on java.sun.com

[Gary Adams]

And the copyright.

Looks good to me.

On 6/14/19, 2:08 PM, Alex Menkov wrote:

Hi all,

Please review small fix for "Java Debug Wire Protocol" page.
The change fixes the link and typo ("Inteface" -> "Interface").

jira: https://bugs.openjdk.java.net/browse/JDK-8225682
webrev: 
http://cr.openjdk.java.net/~amenkov/docs/jdwp-protocol_jni_link/webrev/


--alex

Re: RFR: 8206074: nsk/jdi/EventRequestManager/createStepRequest/crstepreq001/TestDescription.java is timing out

[Gary Adams]

This fix looks good to me.

We've plugged a number of edge cases in the tests
where the timing of debuggee and debugger processes
was not guaranteed during shutdown of the test.

The only improvement we could add here is
to test if the vm is suspended, but an extra resume does
no harm to ensure the debuggee is running to receive the
quit message.

On 6/5/19, 10:36 PM, Daniil Titov wrote:

Please review a change that fixes the intermittent failure of the test.

The problem here that there is a chance that a single step event had been 
posted after the step request was created and before it was deleted.

The fix solves this by ensuring that vm.resume() is called at the end of the 
test.

Webrev: http://cr.openjdk.java.net/~dtitov/8206074/webrev.01
Bug: https://bugs.openjdk.java.net/browse/JDK-8206074

Thanks!
--Daniil

Re: RFR (XS): 8223718: Checks in check_slot_type_no_lvt() should be always executed

[Gary Adams]

+1

On 5/30/19, 12:39 PM, Alex Menkov wrote:

Looks good.

--alex

On 05/29/2019 19:32, serguei.spit...@oracle.com wrote:

Thanks, Gary!
I've fixed the copyright year in the test and split the updated lines.
Also found some inconsistent use of cached local variables and fixed it.

The webrev location is the same:
http://cr.openjdk.java.net/~sspitsyn/webrevs/2019/8223718-jvmti-getlocal.1/ 



Thanks,
Serguei


On 5/29/19 7:04 PM, gary.ad...@oracle.com wrote:

Be sure to check copyright dates and line lengths.

On 5/29/19 6:24 PM, serguei.spit...@oracle.com wrote:

Please, review fix for:
  https://bugs.openjdk.java.net/browse/JDK-8223718

Webrev:
http://cr.openjdk.java.net/~sspitsyn/webrevs/2019/8223718-jvmti-getlocal.1/ 




Summary:
  The JVMTI GetLocal() does not return the error code
  JVMTI_ERROR_INVALID_SLOT for the T_CONFLICT if the LVT is present.
  The fix is to always execute the check_slot_type_no_lvt().
  The test nsk/jvmti/unit/GetLocalVariable/getlocal003 is updated
  to expect the error code JVMTI_ERROR_INVALID_SLOT additionally to
  the JVMTI_ERROR_TYPE_MISMATCH in a couple of cases.

Testing:
  Successful execution of the nsk.jvmti tests (release/fastdebug).
  Mach5 run is in progress.

Thanks,
Serguei

Re: RFR: JDK-8218701: jdb tests failed with AGENT_ERROR_INVALID_THREAD

[Gary Adams]

I have not tracked down the specific root cause of this failure, yet.

It appears that the suspend is being attempted *before* the thread has been
recorded in *threadControl*.

Adding diagnostic messages is tricky because it changes the timing.
Here's a minimal probe to track threadControl addNode and clearThread
transactions. See attached log.txt.

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
@@ -491,7 +491,9 @@
 static void
 suspendWithInvokeEnabled(jbyte policy, jthread thread)
 {
+  /*if (threadControl_getInvokeRequest(thread) != NULL) { */
 invoker_enableInvokeRequests(thread);
+  /*} */

 if (policy == JDWP_SUSPEND_POLICY(ALL)) {
 (void)threadControl_suspendAll();
diff --git a/src/jdk.jdwp.agent/share/native/libjdwp/threadControl.c 
b/src/jdk.jdwp.agent/share/native/libjdwp/threadControl.c

--- a/src/jdk.jdwp.agent/share/native/libjdwp/threadControl.c
+++ b/src/jdk.jdwp.agent/share/native/libjdwp/threadControl.c
@@ -285,6 +285,7 @@
 static void
 addNode(ThreadList *list, ThreadNode *node)
 {
+  printf ("addNode %p \n", node->thread);
 node->next = NULL;
 node->prev = NULL;
 node->list = NULL;
@@ -362,6 +363,7 @@
 static void
 clearThread(JNIEnv *env, ThreadNode *node)
 {
+  printf("clearThread %p\n", node->thread);
 if (node->pendingStop != NULL) {
 tossGlobalRef(env, &(node->pendingStop));
 }
@@ -1646,6 +1648,8 @@
 node = findThread(, thread);
 if (node != NULL) {
  request = >currentInvoke;
+} else {
+  printf ("threadControl_getInvokeRequest %p\n", thread);
 }

 debugMonitorExit(threadLock);

The AGENT_ERROR_INVALID_THREAD is reported when invoker_enableInvokeRequest
does not find the thread. I added the print out in 
threadControl_getInvokeRequest

when the thread is not found.

The workaround bypasses the error and falls through to the threadControl 
CommonSuspend
path where runningThreads is complimented with an otherThreads mechanism 
to ensure
threads that are not between start and end events will be properly 
resumed later on.



On 5/23/19, 1:23 PM, Chris Plummer wrote:

Hi Gary,

So a JVMTI event came in on a valid thread, got processed by the Debug 
Agent and enqueued to be sent to the Debugger. However, before it was 
actually sent, the thread became invalid. Am I understanding this 
issue correctly?


thanks,

Chris

On 5/23/19 2:59 AM, gary.ad...@oracle.com wrote:
This proposed workaround ensures that the delay between a suspend 
request

passing through the jdwp command queue will not fail due to a no longer
running thread.

  Webrev: http://cr.openjdk.java.net/~gadams/8218701/webrev/
  Issue: https://bugs.openjdk.java.net/browse/JDK-8218701







launcher > Starting jdb launching local debuggee
Creating file for jdb stdout stream: .\\jdb.stdout
Creating file for jdb session: .\\jdb.session
Creating file for jdb stderr stream: .\\jdb.stderr
Setting first breakpoint
Sending command: stop in nsk.jdb.dump.dump002.dump002a.main
reply[0]: Deferring breakpoint nsk.jdb.dump.dump002.dump002a.main.
It will be set after the class is loaded.
reply[1]: > 
Starting debuggee class
Sending command: run 
reply[0]: run nsk.jdb.dump.dump002.dump002a
reply[1]: Set uncaught java.lang.Throwable
reply[2]: Set deferred uncaught java.lang.Throwable
reply[3]: > 
reply[4]: VM Started: addNode 00D4466E3270 
addNode 00D4466E3278 
addNode 00D4466E3280 
addNode 00D4466E3288 
addNode 00D4466E3290 
addNode 00D4466E3298 
addNode 00D4466E32A0 
addNode 00D4466E32A8 
addNode 00D4466E32C8 
addNode 00D4466E32D0 
addNode 00D4466E32D0 
Set deferred breakpoint nsk.jdb.dump.dump002.dump002a.main
reply[5]: 
reply[6]: Breakpoint hit: "thread=main", nsk.jdb.dump.dump002.dump002a.main(), 
line=37 bci=0
reply[7]: 37   System.exit(dump002.JCK_STATUS_BASE + 
_dump002a.runIt(args, System.out));
reply[8]: 
reply[9]: main[1] 
Test cases starts.
Sending command: stop in nsk.jdb.dump.dump002.dump002a.lastBreak
reply[0]: Set breakpoint nsk.jdb.dump.dump002.dump002a.lastBreak
reply[1]: main[1] 
Sending command: cont
reply[0]: > 
reply[1]: Breakpoint hit: "thread=main", 
nsk.jdb.dump.dump002.dump002a.lastBreak(), line=40 bci=0
reply[2]: 40static void lastBreak () {}
reply[3]: 
reply[4]: main[1] 
Sending command: dump nsk.jdb.dump.dump002.dump002a._dump002a
reply[0]:  nsk.jdb.dump.dump002.dump002a._dump002a = {
reply[1]: _dump002a: instance of nsk.jdb.dump.dump002.dump002a(id=1382)
reply[2]: iStatic: 0
reply[3]: iPrivate: 1
reply[4]: iProtect: 2
reply[5]: iPublic: 3
reply[6]: iFinal: 4
reply[7]: iTransient: 5
reply[8]: iVolatile: 6
reply[9]: iArray: instance of int[1] (id=1383)
reply[10]: sStatic: "zero"
reply[11]: sPrivate: "one"
reply[12]: sProtected: 

Fwd: Re: RFR: JDK-8223416: Exclude javax/management/monitor/DerivedGaugeMonitorTest.java until JDK-8042211 is fixed.

[Gary Adams]

 Original Message 
Subject: 	Re: RFR: JDK-8223416: Exclude 
javax/management/monitor/DerivedGaugeMonitorTest.java until JDK-8042211 
is fixed.

Date:   Tue, 7 May 2019 13:59:57 -0400
From:   Daniel D. Daugherty 
Reply-To:   daniel.daughe...@oracle.com
To: 	gary.ad...@oracle.com, Internal Serviceability 





Gary,

Thumbs up! But...

The test is open so problem listing the test should also be open.
The bug is closed because the description contains an internal URL.

Dan

On 5/7/19 1:56 PM, Gary Adams wrote:

 Trivial update to ProblemList test while underlying fix is developed.


 diff --git a/test/jdk/ProblemList.txt b/test/jdk/ProblemList.txt
 --- a/test/jdk/ProblemList.txt
 +++ b/test/jdk/ProblemList.txt
 @@ -566,6 +566,8 @@
   java/lang/management/ThreadMXBean/ThreadMXBeanStateTest.java 8081652
 generic-all
   java/lang/management/ThreadMXBean/AllThreadIds.java 8131745 generic-all

 +javax/management/monitor/DerivedGaugeMonitorTest.java 8042211
 generic-all
 +
   


   # jdk_io

Re: 8222667: vmTestbase/nsk/jdi/ThreadStartRequest/addThreadFilter/addthreadfilter002/TestDescription.java failed with "event IS NOT a breakpoint"

[Gary Adams]

Looks good to me.

BTW, how did you track down this failure.
Did you use some tracing option,
or did you add a diagnostic to dump the
stray event?


On 5/2/19, 12:48 PM, Daniil Titov wrote:

Hi Gary,

Please review a new version of the webrev that adds the comment you mentioned.

Regarding the reusable filters I think it makes sense to create them when we 
will find that they could be reused more than in one place and this test is a 
quite specific, it creates ThreadStartRequest and enables it before restricting 
it to the test thread only (by calling addThreadFilter()) since it is exactly 
what the test checks (that calling addThreadFilter() on enabled thread start 
request throws InvalidRequestStateException.

Webrev: http://cr.openjdk.java.net/~dtitov/8222667/webrev.02
Bug: https://bugs.openjdk.java.net/browse/JDK-8222667

Thanks!
--Danill

On 5/2/19, 4:56 AM, "Gary Adams"  wrote:

 It would be good to include a comment that the thread start is being
 allowed because of graal.

 Now that we have a series of graal specific alterations in the tests,
 it might be useful to provide some reusable filters. $.02

 On 5/1/19, 9:07 PM, Daniil Titov wrote:
 >  Please review the change that fixes the test that intermittently fails 
with Graal on.
 >
 >  The test tests  addThreadFilter() method for com.sun.jdi.ThreadStartRequest 
class. It starts a debuggee, creates ThreadStartRequest, enables it, and calls 
addThreadFilter(). The test also uses breakpoints to synchronize with the debuggee,  but the 
problem here is that while waiting for a breakpoint event, occasionally, when Graal is on, 
the event the test receives is a ThreadStartEvent for " HotSpotGraalManagement Bean 
Registration" thread, rather than a breakpoint event. The test doesn't expect it and 
fails.
 >
 >  The fix takes this into account and makes the test keep waiting for a 
breakpoint event instead of failing.
 >
 >  Webrev:http://cr.openjdk.java.net/~dtitov/8222667/webrev.01/
 >  Bug:https://bugs.openjdk.java.net/browse/JDK-8222667
 >
 >  Thanks!
 >  --Daniil
 >
 >

Re: RFR: 8222667: vmTestbase/nsk/jdi/ThreadStartRequest/addThreadFilter/addthreadfilter002/TestDescription.java failed with "event IS NOT a breakpoint"

[Gary Adams]

It would be good to include a comment that the thread start is being 
allowed because of graal.


Now that we have a series of graal specific alterations in the tests,
it might be useful to provide some reusable filters. $.02

On 5/1/19, 9:07 PM, Daniil Titov wrote:

Please review the change that fixes the test that intermittently fails with 
Graal on.

The test tests  addThreadFilter() method for com.sun.jdi.ThreadStartRequest class. It 
starts a debuggee, creates ThreadStartRequest, enables it, and calls addThreadFilter(). 
The test also uses breakpoints to synchronize with the debuggee,  but the problem here is 
that while waiting for a breakpoint event, occasionally, when Graal is on, the event the 
test receives is a ThreadStartEvent for " HotSpotGraalManagement Bean 
Registration" thread, rather than a breakpoint event. The test doesn't expect it and 
fails.

The fix takes this into account and makes the test keep waiting for a 
breakpoint event instead of failing.

Webrev:http://cr.openjdk.java.net/~dtitov/8222667/webrev.01/
Bug:https://bugs.openjdk.java.net/browse/JDK-8222667

Thanks!
--Daniil

Re: RFR 8223065: Add jcmd to get the listen address of the debugger

[Gary Adams]

In diagnosticCommand.cpp you'll want to use "res != 0"
on lines 1084 and 1086 to avoid compiler warnings
about  ambiguous conversions to boolean.

1082   // The result should be a byte array or null
1083   typeArrayOop res = (typeArrayOop) result.get_jobject();
1084   assert(!res || (TypeArrayKlass::cast(res->klass())->element_type() == T_BYTE), 
"Must be byte array");
1085
1086   if (res&&  (res->length()>  0)) {



I see a SEGV in print_debug_listen_address running
GetListenAddressTest with a linux-x64-debug build.

Can you triage the bug and target the fixVersion for 13.

Thanks

On 4/29/19, 9:19 AM, Schmelter, Ralf wrote:

Please review the patch which adds a jcmd to get the actual address the 
debugging backend is listening on.

The this value was stored in the agent property sun.jdwp.listenerAddress and 
currently only used by the ProcessAttachingConnector.

Additionally, the listen address is now displayed by the 
VM.start_java_debugging command, if a new session was started.

webrev: http://cr.openjdk.java.net/~rschmelter/webrevs/8223065/webrev.0/
bugreport: https://bugs.openjdk.java.net/browse/JDK-8223065

Best regards,
Ralf

Re: RFR (S) 8222529: sun.jdwp.listenerAddress agent property uses wrong encoding

[Gary Adams]

It'd be good to have someone on build-dev review this change.

On 4/26/19, 3:36 AM, Langer, Christoph wrote:

Hi Alex,

for other platforms (toolchains), except Windows, -ljava is part of 
BASIC_JDKLIB_LIBS ->  $(JDKLIB_LIBS), see here: 
http://hg.openjdk.java.net/jdk/jdk/file/9ebb614d293d/make/autoconf/libraries.m4#l114

Although I don't know the reasoning for that, the patch seems correct to me.

I guess it can be pushed then?

Best regards
Christoph


-Original Message-
From: serviceability-dev  On
Behalf Of Alex Menkov
Sent: Freitag, 26. April 2019 03:10
To: serguei.spit...@oracle.com; Schmelter, Ralf;
serviceability-dev@openjdk.java.net
Subject: Re: RFR (S) 8222529: sun.jdwp.listenerAddress agent property uses
wrong encoding

Hi Ralf,

You added libjava for Windows, but not for other platforms.
Doesn't it need
LIBS_unix := -ljava
?

--alex

On 04/25/2019 17:59, serguei.spit...@oracle.com wrote:

Hi Ralf,

The fix looks good to me.
We agreed that Alex will also look at this.

Thanks,
Serguei

On 4/17/19 1:30 AM, Schmelter, Ralf wrote:

Can you please review this change, which ensures the
sun.jdwp.listenerAddress property value is created using the platform
encoding.

webrev:

http://cr.openjdk.java.net/~rschmelter/webrevs/8222529/webrev.0/

bugreport: https://bugs.openjdk.java.net/browse/JDK-8222529

Best regards,
Ralf

Re: RFR (trivial): 8222970: Update ProblemList for vmTestbase/nsk/jdb/eval/eval001/eval001.java

[Gary Adams]

Looks OK to me.

On 4/25/19, 4:36 AM, David Holmes wrote:

Bug: https://bugs.openjdk.java.net/browse/JDK-8222970

vmTestbase/nsk/jdb/eval/eval001/eval001.java is ProblemListed under 
JDK-8221503 which is now closed as a duplicate of JDK-8212117. So 
ProblemList.txt needs updating.


Thanks,
David
--

diff -r b43cc3b9ef40 test/hotspot/jtreg/ProblemList.txt
--- a/test/hotspot/jtreg/ProblemList.txt
+++ b/test/hotspot/jtreg/ProblemList.txt
@@ -159,7 +159,7 @@

vmTestbase/nsk/jdi/VirtualMachine/redefineClasses/redefineclasses021/TestDescription.java 
8065773 generic-all


vmTestbase/nsk/jdi/VirtualMachine/redefineClasses/redefineclasses023/TestDescription.java 
8065773 generic-all


-vmTestbase/nsk/jdb/eval/eval001/eval001.java 8221503 generic-all
+vmTestbase/nsk/jdb/eval/eval001/eval001.java 8212117 generic-all

 vmTestbase/metaspace/gc/firstGC_10m/TestDescription.java 8208250 
generic-all
 vmTestbase/metaspace/gc/firstGC_50m/TestDescription.java 8208250 
generic-all

Re: RFR: JDK-8222741: jdi/EventQueue/remove/remove004 fails due to VMDisconnectedException

[Gary Adams]

From the CR :


--> debugger: -t2: method 'run' exit

Thead2 from the debugger process has completed. remove004.java:531

debugee.stderr> **> debuggee: after: Thread.sleep

Debuggee process has completed the sleep operation.  remove004a.java:113

--> debugger: breakpointForCommunication

Debugger main thread has passed the breakpoint event check.
  remove004.java:315 breakpointForCommunication:479
  remove004a.java:120 methodForCommunication

Since  VMDisconnectedException is a RunTimeException,
it may have been thrown during the breakpointForCommunication
processing or in the debuggeeClass.getValue field lookup.

# ERROR: ##> debugger: ERROR: VMDisconnectedException : 
com.sun.jdi.VMDisconnectedException

The following stacktrace is for failure analysis.
nsk.share.TestFailure: ##> debugger: ERROR: VMDisconnectedException : 
com.sun.jdi.VMDisconnectedException

at nsk.share.Log.logExceptionForFailureAnalysis(Log.java:428)
at nsk.share.Log.complain(Log.java:399)
at nsk.jdi.EventQueue.remove.remove004.log3(remove004.java:120)
at nsk.jdi.EventQueue.remove.remove004.runTest(remove004.java:262)
at nsk.jdi.EventQueue.remove.remove004.runThis(remove004.java:195)
at nsk.jdi.EventQueue.remove.remove004.run(remove004.java:101)

The VMDisconnectedException is caught in runTest:262 which
was processing the testRun:252 steps where the proposed change
would be placed. If the failure was detected in the final steps
confirming the VMDeathEvent, then we would have seen the
log message at remove004.java:254.

On 4/19/19, 1:58 PM, Chris Plummer wrote:

Hi Gary,

Can you add the backtrace of the exception to the CR. It will make it 
a bit easier to understand when/why this bug is happening.


If I understand correctly, the second time through the loop the 
debuggee should be at a breakpoint, but we do the vm.resume() before 
we check for the breakpoint event, and this sometimes leads to the 
debuggee exiting before the call to eventIterator.nextEvent(), and 
this is where the exception occurs.


Also, I assume with the old implementation that the vm.resume() done 
at lines 321 or 329 actually is not doing anything on the 2nd and 
subsequent iterations since the debuggee is already resumed by the 
resume() at the top of the loop. With your fix, they now are providing 
a needed resume().


If my understanding is correct, then your fix looks good.

thanks,

Chris

On 4/19/19 10:06 AM, Gary Adams wrote:

Prevent the debuggee from exiting too quickly by
holding it at the breakpoint until the end of processing
flag has been read by the main thread from the debugger process.

  Webrev: http://cr.openjdk.java.net/~gadams/8222741/webrev/
  Issue: https://bugs.openjdk.java.net/browse/JDK-8222741

RFR: JDK-8222741: jdi/EventQueue/remove/remove004 fails due to VMDisconnectedException

[Gary Adams]

Prevent the debuggee from exiting too quickly by
holding it at the breakpoint until the end of processing
flag has been read by the main thread from the debugger process.

  Webrev: http://cr.openjdk.java.net/~gadams/8222741/webrev/
  Issue: https://bugs.openjdk.java.net/browse/JDK-8222741

Re: JDK-8177064: jcmd help command should not require the process identification

[Gary Adams]

On 4/11/19, 8:16 PM, David Holmes wrote:

Hi Gary,

On 12/04/2019 4:24 am, Gary Adams wrote:

Two years ago a request was made to allow

jcmd help

to be interpretted as a request for help from the current jcmd process
rather than requiring a separate target process to be involved.

Some attempts were made to close the issue, because the
command was not documented to work that way.
It was also pointed out that the help from the running jcmd
might not match the cmds available from a subsequent
request to a different process possibly running a different version
of the vm.

As proof of concept exercise this webrev shows a minimal set of
changes that could support

   jcmd self help

Before fleshing out additional changes in documentation and testing,
I'd like to know if this is still a worth while enhancement. Also,


Given we allow a pid of 0 to mean all Java processes I can see scope 
for a special value that means "the current Java process", but using a 
text word like "self" seems awkward at best. Plus within a shell you 
can use: 
jcmd $$ help

$$ is the pid for the shell where the command is running,
not the jcmd pid.

$ jcmd $$ help
17076:
com.sun.tools.attach.AttachNotSupportedException: Unable to open socket 
file: target process not responding or HotSpot VM not loaded

at sun.tools.attach.BsdVirtualMachine.(BsdVirtualMachine.java:90)
at 
sun.tools.attach.BsdAttachProvider.attachVirtualMachine(BsdAttachProvider.java:63)

at com.sun.tools.attach.VirtualMachine.attach(VirtualMachine.java:208)
at sun.tools.jcmd.JCmd.executeCommandForPid(JCmd.java:147)
at sun.tools.jcmd.JCmd.main(JCmd.java:131)



to act on the jcmd VM itself, so there doesn't need to be special 
treatment in that sense. Perhaps some tweaks to the "jcmd -h" text.

If we did allow the self attach, we would need to update the help text.
I initially worked with -1 as another special value, but that looks a lot
like -l the list argument.



I'd like to know more about the history of the checks preventing self 
attach.


This may give you a start: 
https://bugs.openjdk.java.net/browse/JDK-8177154

I understand the dangers of agent loading and the arguments against
command line options and default settings that took place in jdk9
release.

I'm looking for something older that explains the rationale about
not allowing attach to the current process.


Cheers,
David


   Webrev: http://cr.openjdk.java.net/~gadams/8177064/webrev/index.html
   Issue: https://bugs.openjdk.java.net/browse/JDK-8177064

JDK-8177064: jcmd help command should not require the process identification

[Gary Adams]

Two years ago a request was made to allow

   jcmd help

to be interpretted as a request for help from the current jcmd process
rather than requiring a separate target process to be involved.

Some attempts were made to close the issue, because the
command was not documented to work that way.
It was also pointed out that the help from the running jcmd
might not match the cmds available from a subsequent
request to a different process possibly running a different version
of the vm.

As proof of concept exercise this webrev shows a minimal set of
changes that could support

  jcmd self help

Before fleshing out additional changes in documentation and testing,
I'd like to know if this is still a worth while enhancement. Also,
I'd like to know more about the history of the checks preventing self 
attach.


  Webrev: http://cr.openjdk.java.net/~gadams/8177064/webrev/index.html
  Issue: https://bugs.openjdk.java.net/browse/JDK-8177064

Re: RFR: JDK-8203364: Some serviceability/sa/ tests intermittently fail with java.io.IOException: LingeredApp terminated with non-zero exit code 3

[Gary Adams]

I need a second reviewer for this minor update.

On 4/2/19, 2:18 PM, Chris Plummer wrote:

On 4/2/19 11:11 AM, Gary Adams wrote:

The test exits with 3 for the general Exception.
I added 4 just to distinguish the return.
I see 7 used earlier when the lockfile is not initialized.

I could add a message, if you think it is useful.

Yes, I think it would be.


The exit code is printed in stopApp(). line 377.
Ok. I was just wondering if specific error codes were being checked 
for. It looks like that's not the case.


thanks,

Chris


On 4/2/19, 1:58 PM, Chris Plummer wrote:

Hi Gary,

I see various System.exit() calls with varying exit status. Is there 
some place that documents them? What is 4?


Any reason not to print the exception before doing the System.exit(4)?

thanks,

Chris

On 4/2/19 4:50 AM, Gary Adams wrote:
This proposed change allows a wider range of IOExceptions to be 
observed
when a LingeredApp is being shutdown. It may not be just a 
NoSuchFileException.
Botton line - if the lock file doesn't exist at the time the 
IOException

is observed then the main application is terminating the test sequence
and LingeredApp should end normally.

  Webrev: http://cr.openjdk.java.net/~gadams/8203364/webrev/
  Issue: https://bugs.openjdk.java.net/browse/JDK-8203364

Re: RFR: JDK-8203364: Some serviceability/sa/ tests intermittently fail with java.io.IOException: LingeredApp terminated with non-zero exit code 3

[Gary Adams]

The test exits with 3 for the general Exception.
I added 4 just to distinguish the return.
I see 7 used earlier when the lockfile is not initialized.

I could add a message, if you think it is useful.

The exit code is printed in stopApp(). line 377.

On 4/2/19, 1:58 PM, Chris Plummer wrote:

Hi Gary,

I see various System.exit() calls with varying exit status. Is there 
some place that documents them? What is 4?


Any reason not to print the exception before doing the System.exit(4)?

thanks,

Chris

On 4/2/19 4:50 AM, Gary Adams wrote:

This proposed change allows a wider range of IOExceptions to be observed
when a LingeredApp is being shutdown. It may not be just a 
NoSuchFileException.

Botton line - if the lock file doesn't exist at the time the IOException
is observed then the main application is terminating the test sequence
and LingeredApp should end normally.

  Webrev: http://cr.openjdk.java.net/~gadams/8203364/webrev/
  Issue: https://bugs.openjdk.java.net/browse/JDK-8203364

RFR: JDK-8203364: Some serviceability/sa/ tests intermittently fail with java.io.IOException: LingeredApp terminated with non-zero exit code 3

[Gary Adams]

This proposed change allows a wider range of IOExceptions to be observed
when a LingeredApp is being shutdown. It may not be just a 
NoSuchFileException.

Botton line - if the lock file doesn't exist at the time the IOException
is observed then the main application is terminating the test sequence
and LingeredApp should end normally.

  Webrev: http://cr.openjdk.java.net/~gadams/8203364/webrev/
  Issue: https://bugs.openjdk.java.net/browse/JDK-8203364

Re: LingeredApp termination sequence

[Gary Adams]

The problem I was trying to understand a little better ...

I was observing an IOException, but it was not a NoSuchFileException.
Changing to catch just IOException runs cleanly, but may obscure
some other cause of the IOException.

Testing a variant today that exits cleanly as long as the lock file was 
removed.


diff --git a/test/lib/jdk/test/lib/apps/LingeredApp.java 
b/test/lib/jdk/test/lib/apps/LingeredApp.java

--- a/test/lib/jdk/test/lib/apps/LingeredApp.java
+++ b/test/lib/jdk/test/lib/apps/LingeredApp.java
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2015, 2018, Oracle and/or its affiliates. All rights 
reserved.
+ * Copyright (c) 2015, 2019, 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
@@ -492,18 +492,21 @@
 }

 String theLockFileName = args[0];
+Path path = Paths.get(theLockFileName);

 try {
-Path path = Paths.get(theLockFileName);
-
 while (Files.exists(path)) {
 // Touch the lock to indicate our readiness
 setLastModified(theLockFileName, epoch());
 Thread.sleep(spinDelay);
 }
-} catch (NoSuchFileException ex) {
+} catch (IOException ex) {
 // Lock deleted while we are setting last modified time.
-// Ignore error and lets the app exits
+// Ignore the error and let the app exit.
+if (Files.exists(path)) {
+// If the lock file was not removed, return an error.
+System.exit(4);
+}
 } catch (Exception ex) {
 System.err.println("LingeredApp ERROR: " + ex);
 // Leave exit_code = 1 to Java launcher


During the latest set of test runs I saw a variety of RMI binding errors.
These could be issues with the test infrastructure systems.
Or could be collisions from the test harness.

[Jstatd-Thread] Could not bind /JStatRemoteHost to RMI Registry
[Jstatd-Thread] java.rmi.server.ExportException: Port already in use: 
1099; nested exception is:

[Jstatd-Thread] java.net.BindException: Address already in use: NET_Bind
[Jstatd-Thread] at 
java.rmi/sun.rmi.transport.tcp.TCPTransport.listen(TCPTransport.java:335)



[Jstatd-Thread] Could not bind //:49153/JStatRemoteHost to RMI Registry
[Jstatd-Thread] java.rmi.ConnectIOException: error during JRMP 
connection establishment; nested exception is:

[Jstatd-Thread] java.net.SocketTimeoutException: Read timed out


[Jstatd-Thread] Could not bind //:33712/JStatRemoteHost to RMI Registry
[Jstatd-Thread] java.rmi.server.ExportException: Port already in use: 
33712; nested exception is:
[Jstatd-Thread] java.net.BindException: Address already in use (Bind 
failed)



[Jstatd-Thread] Could not bind //:58765/TestJstatdServer to RMI Registry
[Jstatd-Thread] java.rmi.ConnectIOException: Exception creating 
connection to: 10.133.156.143; nested exception is:
[Jstatd-Thread] java.net.NoRouteToHostException: Cannot assign 
requested address



On 3/30/19, 9:46 AM, Dmitry Samersoff wrote:

Hello Gary,

The LingeredApp was designed to provide precise control over the life
cycle of a subordinate process.

This means that if the app exits unexpectedly, we should have detailed
information why it happens.

IOException may occur when the app tries to touch lockfile (e.g. due to
a network filesystem error), so it might be better to catch IOException,
then check if it is an instance of NoSuchFileException or the lockfile
was deleted by driver.

-Dmitry

On 29.03.2019 20:12, Gary Adams wrote:

While running some bulk testing on jdk/sun/tools I've
come across some errors reported from the termination sequence
for LingeredApp debuggee process.

The main process creates a locking file, which the LingeredApp
sits in a loop updating the file modification date. When the main
test completes it stops the LingeredApp by removing the file.

The clean exit results when LingeredApp gets a NoSuchFileException.
I see a number of bugs filed that are getting a raw IOException
on the LingeredApp termination.

I'm going to attempt to get more specific information about the
IOException.

diff --git a/test/lib/jdk/test/lib/apps/LingeredApp.java
b/test/lib/jdk/test/lib/apps/LingeredApp.java
--- a/test/lib/jdk/test/lib/apps/LingeredApp.java
+++ b/test/lib/jdk/test/lib/apps/LingeredApp.java
@@ -504,6 +504,9 @@
  } catch (NoSuchFileException ex) {
  // Lock deleted while we are setting last modified time.
  // Ignore error and lets the app exits
+} catch (IOException ioe) {
+ioe.printStackTrace();
+System.exit(3);
  } catch (Exception ex) {
  System.err.println("LingeredApp ERROR: " + ex);
  // Leave exit_code = 1 to Java launcher

Since the error is being detected as the test is 

RFR: JDK-8221694: jstatLineCounts1 needs to be NaN resilient

[Gary Adams]

With additional testing of jdk/sun/tools The problem seen previously with
lineCounts3 and lineCounts4 has begun to appear in lineCounts1. To complete
the set this changset will also invlude the same fix for lineCounts2.
e.g. allow floating point or NaN dash ("-") in the metaspace column
 ([0-9]+\.[0-9]+|-)

  Issue: https://bugs.openjdk.java.net/browse/JDK-8221694



diff --git a/test/jdk/sun/tools/jstat/lineCounts1.awk 
b/test/jdk/sun/tools/jstat/lineCounts1.awk

--- a/test/jdk/sun/tools/jstat/lineCounts1.awk
+++ b/test/jdk/sun/tools/jstat/lineCounts1.awk
@@ -18,7 +18,7 @@
 headerlines++;
 }

-/^[ ]*[0-9]+\.[0-9]+[ ]*[0-9]+\.[0-9]+[ ]*[0-9]+\.[0-9]+[ 
]*[0-9]+\.[0-9]+[ ]*[0-9]+\.[0-9]+[ ]*([0-9]+\.[0-9]+)|-[ ]*[0-9]+[ 
]*[0-9]+\.[0-9]+[ ]*[0-9]+[ ]*[0-9]+\.[0-9]+[ ]*[0-9]+[ 
]*[0-9]+\.[0-9]+[ ]*[0-9]+\.[0-9]+$/{
+/^[ ]*[0-9]+\.[0-9]+[ ]*[0-9]+\.[0-9]+[ ]*[0-9]+\.[0-9]+[ 
]*[0-9]+\.[0-9]+[ ]*([0-9]+\.[0-9]+|-)[ ]*([0-9]+\.[0-9]+|-)[ ]*[0-9]+[ 
]*[0-9]+\.[0-9]+[ ]*[0-9]+[ ]*[0-9]+\.[0-9]+[ ]*[0-9]+[ 
]*[0-9]+\.[0-9]+[ ]*[0-9]+\.[0-9]+$/{

 datalines++;
 }

diff --git a/test/jdk/sun/tools/jstat/lineCounts2.awk 
b/test/jdk/sun/tools/jstat/lineCounts2.awk

--- a/test/jdk/sun/tools/jstat/lineCounts2.awk
+++ b/test/jdk/sun/tools/jstat/lineCounts2.awk
@@ -14,7 +14,7 @@
 headerlines++;
 }

-/^[ ]*[0-9]+\.[0-9]+[ ]*[0-9]+\.[0-9]+[ ]*[0-9]+\.[0-9]+[ 
]*[0-9]+\.[0-9]+[ ]*[0-9]+\.[0-9]+[ ]*([0-9]+\.[0-9]+)|-[ ]*[0-9]+[ 
]*[0-9]+\.[0-9]+[ ]*[0-9]+[ ]*[0-9]+\.[0-9]+[ ]*[0-9]+[ 
]*[0-9]+\.[0-9]+[ ]*[0-9]+\.[0-9]+$/{
+/^[ ]*[0-9]+\.[0-9]+[ ]*[0-9]+\.[0-9]+[ ]*[0-9]+\.[0-9]+[ 
]*[0-9]+\.[0-9]+[ ]*([0-9]+\.[0-9]+|-)[ ]*([0-9]+\.[0-9]+|-)[ ]*[0-9]+[ 
]*[0-9]+\.[0-9]+[ ]*[0-9]+[ ]*[0-9]+\.[0-9]+[ ]*[0-9]+[ 
]*[0-9]+\.[0-9]+[ ]*[0-9]+\.[0-9]+$/{

 datalines++;
 }

LingeredApp termination sequence

[Gary Adams]

While running some bulk testing on jdk/sun/tools I've
come across some errors reported from the termination sequence
for LingeredApp debuggee process.

The main process creates a locking file, which the LingeredApp
sits in a loop updating the file modification date. When the main
test completes it stops the LingeredApp by removing the file.

The clean exit results when LingeredApp gets a NoSuchFileException.
I see a number of bugs filed that are getting a raw IOException
on the LingeredApp termination.

I'm going to attempt to get more specific information about the IOException.

diff --git a/test/lib/jdk/test/lib/apps/LingeredApp.java 
b/test/lib/jdk/test/lib/apps/LingeredApp.java

--- a/test/lib/jdk/test/lib/apps/LingeredApp.java
+++ b/test/lib/jdk/test/lib/apps/LingeredApp.java
@@ -504,6 +504,9 @@
 } catch (NoSuchFileException ex) {
 // Lock deleted while we are setting last modified time.
 // Ignore error and lets the app exits
+} catch (IOException ioe) {
+ioe.printStackTrace();
+System.exit(3);
 } catch (Exception ex) {
 System.err.println("LingeredApp ERROR: " + ex);
 // Leave exit_code = 1 to Java launcher

Since the error is being detected as the test is terminating,
I think it would be possible to simply replace the NoSuchFileException,
and consider an IOException as the trigger for the clean exit sequence.

Anyone have experience with this corner of the swamp?

Re: RFR: JDK-8184770: JDWP support for IPv6

[Gary Adams]

Is there any documentation that needs to be updated along with the impl 
changes?


Would it make sense to support the preference properties?
  java.net.preferIPv4Stack
  java.net.preferIPv6Addresses

Will the previous jdwp tests run with IPv6 or just the new additions?

Should probably have reviewers with networking expertise. core-libs(?)

Do we know if IPv6 is enabled in our test infrastructure?

A stray "printf" statement in the initial webrev. socketTransport.c

On 3/27/19, 7:04 PM, Alex Menkov wrote:

Hi all,

Please review the fix for
https://bugs.openjdk.java.net/browse/JDK-8184770
webrev:
http://cr.openjdk.java.net/~amenkov/IPv6/webrev.00/

Main changes are in socketTransport.c - the code is updated to support 
both IPv4 and IPv6.

Some details to simplify reviewing:
- listening:
  - if IP address is specified (like 127.0.0.1 or ::1), connector 
listens only on this address;
  - for backward compatibility if no address (or "localhost") is 
specified, IPv4 is used (if supported by the host);
  - if "*" is specified (means "listen on all local interfaces"), dual 
mode socket is used to listen on both IPv6 and IPv4 addresses;
  - AllowedPeerInfo structure (for "allow" option) is updated to use 
IPv6 address/mask, support for IPv4 is implemented by using "mapped" 
IPv4 addresses;
- attaching: agent resolves and tries to connect to all (IPv4 and 
IPv6) addresses, IPv4 are tried first;


SocketListeningConnector.java/SocketTransportService.java are updated 
to support IPv6 addresses (the addresses may contain colons);


new JdwpAttachTest.java/JdwpListenTest.java test that listening and 
attaching works for all available addresses (Ipv4 and IPv6)


BasicJDWPConnectionTest.java was renamed to JdwpAllowTest.java (as it 
tests "allow" functionality), tests for mask (prefix length) 
functionality are added (for both IPv4 and IPv6);


--alex

Re: RFR: 8218727: vmTestbase/nsk/jvmti/scenarios/events/EM04/em04t001/TestDescription.java crash in native library

[Gary Adams]

Looks OK to me.

On 3/27/19, 3:59 AM, serguei.spit...@oracle.com wrote:

Hi Daniil,

The fix looks good.

Thanks,
Serguei


On 3/26/19 20:32, Daniil Titov wrote:

Please review the change that fixes the test when running with Graal on.

The problem here is the lack of synchronization when accessing the 
internal data structure (the list of events) in the agent procedure 
and in the callbacks the test enables. That results in the agent proc 
starts removing elements from the list of the events while the 
callback handlers are still enabled, assuming that no new events 
could be generated,  but in case of Graal it is not true. This causes 
to the crash when the callback iterates over the list of events and 
tries to access the element that was just removed by the agent 
procedure.


Webrev: http://cr.openjdk.java.net/~dtitov/8218727/webrev.01/
Bug: https://bugs.openjdk.java.net/browse/JDK-8218727

Thanks!

--Daniil

JDK-8207347: HeapDumpAllTest.java fails intermittently

[Gary Adams]

I'll try one more time to reproduce the failure running the
serviceability/dcmd tests.

If there are no failures, I plan to close the issue
similar to what was done for
JDK-8217220  
runtime/NMT/MallocStressTest.java: target process doesn't respond

Re: RFR: JDK-8218128: vmTestbase/nsk/jvmti/ResourceExhausted/resexhausted003 and 004 use wrong path to test classes

[Gary Adams]

During testing I found that there is still good reasons to leave 003 and 
004
tests on the ProblemList. I believe there is still work to be done to 
ensure

JDK-7013634 and JDK-6606767 are covered.

This pass attempts to solve the primary issue in this bug which is 
improper paths on the
test command lines. Cleanup is limited to the 003 and 004 tests. The 
tests will

remain on the ProblemList.

  Issue: https://bugs.openjdk.java.net/browse/JDK-8218128
  Webrev: http://cr.openjdk.java.net/~gadams/8218128/webrev.01/index.html

On 3/8/19, 3:39 PM, Chris Plummer wrote:
Ok, I didn't realize the @ignore was removed also. I guess this is 
what "exclude" was converted into. So the question now is should the 
@ignore (and the exclude comment) remain. The @ignore bug 
(JDK-7013634) was closed as CNR. I'm actually the one that closed it a 
while back, and that may have been a mistake since I didn't realize 
that some of the tests had been completely excluded from testing, even 
quarantine testing. After reading through some of the comments in 
JDK-7013634, I'm not trusting these tests to run reliably. That's 
often the case with tests that try to exhaust resources.


Chris

On 3/8/19 9:41 AM, Gary Adams wrote:

On 3/8/19, 12:10 PM, Chris Plummer wrote:
You can remove the quarantine and exclude keywords. I think that's 
appropriate if the test is off the problemlist and working. It was 
just nonconcurrent removal that I was against.

Done


As for the resexhausted001 failure you are still seeing, how could 
jtreg exclude it if it was not on the problemlist. I didn't think 
there was any other mechanism. I don't believe jtreg looks at the 
tonga keywords.


I believe the @ignore is a jtreg keyword.
With the -ignore command line flag jtreg can be directed to
quietly ignore a test, or force an error, or attempt to run the test
even though the @ignore directive is there.



thanks,

Chris

On 3/8/19 5:07 AM, Gary Adams wrote:

I'll revert the comments that were left in during the tonga conversion.

I did come across an interesting test failure in resexhausted001
which had an
  @ignore 7013634

JDK-7013634: jvmti resexhausted001 can timeout or fail due to 
excessive thread creation


The test was closed because it was not reproducible.
Even though the test was not on the ProblemList, I believe
jtreg was excluding the test from running.

The original problem reported an "out of swap" condition.

The current failure reports:
--System.out:(3/217)--
Creating threads...
Timeout refired 480 times
[730.871s][warning][os,thread] Failed to start thread - _beginthreadex failed 
(EACCES) for attributes: stacksize: default, flags: CREATE_SUSPENDED 
STACK_SIZE_PARAM_IS.


On 3/7/19, 4:02 PM, Chris Plummer wrote:

Hi Gary,

Why did you remove the "nonconcurrent" keyword. I know these are 
just comments for reference that were added when the test was 
ported from tonga, but as a comment it is still applicable. The 
test should not be run concurrent with others (which you have also 
fixed with the addition of the "exclusiveAccess.dirs=.").


Otherwise changes look good.

thanks,

Chris

On 3/7/19 10:57 AM, Gary Adams wrote:

This proposed fix will restore the ResourceExhausted tests.

Test 3 and 4 were on the ProblemList because of the potential
path issues in finding the correct classes. This change searches the
test.class.path for the appropriate vmTestbase classes rather 
than using

incorrect settings on the command line.

Some clean up has been done to remove quarantine keyword
and @ignore directives. Should additional clean up be done to remove
bug numbers, etc.?

TEST.PROPERTIES were added so test 3 so it is consistent with the 
other tests

in the group.

  Issue: https://bugs.openjdk.java.net/browse/JDK-8218128
  Webrev: 
http://cr.openjdk.java.net/~gadams/8218128/webrev.00/index.html


Local testing has been successful on a linux-x64-debug build.
Testing on mach5 for other platforms next.

Re: RFR: JDK-8203026: java.rmi.NoSuchObjectException: no such object in table

[Gary Adams]

Here's an updated webrev that includes the change to bind to the stub.
The jstatd tests continue to pass after this change.

 Webrev: http://cr.openjdk.java.net/~gadams/8203026/webrev.01/

It would be good to have a second reviewer from the serviceability team.

Longer term it would be good to replace the rmi dependency, which does not
appear to be central to the functionality provided here.

On 3/22/19, 9:56 AM, Roger Riggs wrote:

Hi Gary,

Holding a static reference to the implementation solves the problem.

But I noticed that the object that is bound in the registry is the 
RemoteHostImpl

and it should be the RemoteHost stub.

Line 145: should be:

bind(name.toString(), stub);

That is likely to solve the problem as well, because the distributed 
garbage
collector will correctly cause a hard reference to be retained to the 
implementation.


Roger


On 03/22/2019 04:05 AM, gary.ad...@oracle.com wrote:

Here's a proposed fix for the intermittent jstatd test failure.
By moving the exported object from a local method variable to a
static class variable a strong reference is held so the object
will not be garbage collected prematurely.

  Webrev: http://cr.openjdk.java.net/~gadams/8203026/webrev.00/
  Issue: https://bugs.openjdk.java.net/browse/JDK-8203026

The test is currently filed as a core-libs/java.rmi, but it probably
should be core-svc/tools with label=jstatd. It is the callers
responsibility to ensure the strong reference to prevent
the premature garbage collection, so this is a test failure
rather than a need to change the underlying rmi behavior.

Re: RFR: JDK-8220295: sun/tools/jps/TestJps.java still timing out

[Gary Adams]

Agreed.

Unless a test is specifically testing the functionality of 
VirtualMachine.list()

it should not be used for locating a pid, which is easily provided
from the ProcessBuilder after the process is started.

Reducing the environmental interference of scanning all running java 
processes

is our best bet at this point for these intermittent timeouts.

I do plan to follow up to find the root cause of these hangs,
but at this time we don't have a concrete problem identified.
We just know which swamp to look for the beast.

On 3/24/19, 9:09 PM, David Holmes wrote:
Just an aside, but it seems that other tests that happen to use this 
functionality can also run into these transient timeout issues and 
likely for the same underlying reasons. See for example:


JDK-8217347: [TESTBUG] runtime/appcds/jvmti/InstrumentationTest.java 
timed out


So it would be good to get to the bottom of the underlying problem here.

Cheers,
David

On 23/03/2019 1:00 pm, Chris Plummer wrote:

Ok. No need to write the stress test now, just file the CR.

thanks,

Chris

On 3/22/19 4:07 PM, gary.ad...@oracle.com wrote:

Yes, there will be more follow up after the patch is pushed.

I started a thread on jtreg-dev to discuss the current implementation
and possibly an enhancement to meet the need of the vmTestbase
tests that may have the wrong expectation of the current feature.
e.g. the previous test harness had a nonconcurrent option.

Filing a bug against jtreg will be cleaner after a bit more 
discussion there.


You had also suggested creating a stress test to see if we can
capture the jps hang. That may take a bit more time to consider
and is not as urgent as some other work items in progress.

On 3/22/19 3:32 PM, Chris Plummer wrote:

Hi Gary,

The changes look fine, but are you going to file the CRs I 
suggested (one for jtreg exclusiveAccess.dirs help text and one to 
further look into this issue)?


thanks,

Chris

On 3/22/19 1:38 AM, gary.ad...@oracle.com wrote:

This proposed change will configure the sun/tools/j* and
com/sun/tools/attach tests to be labeled as jtreg 
exclusiveAccess.dirs

so the java attaching tests are not run concurrently.

There is only annecdotal observations that multiple
tools are running when these tests are timing out.
1000s of test runs have succeeded with this change in place.

More investigation can be done to find the root cause
of these failures, but for now it's best to make the tests
run more reliably without timing out.

  Issues:
https://bugs.openjdk.java.net/browse/JDK-8220242
https://bugs.openjdk.java.net/browse/JDK-8220295

  Webrev:
http://cr.openjdk.java.net/~gadams/8220295/webrev/

Re: RFR: JDK-8220295: sun/tools/jps/TestJps.java still timing out

[Gary Adams]

I've been talking with Roger about the RMI issue
I plan to cross post the RFR to serviceability-dev and core-libs.
The fix needs reviews from both groups.

That being said, the responsibility for keeping the strong reference is
specified to be the callers responsibility. It's part of the distributed
garbage collection foundation.

I'll start cleaning up the jstat RFRs tomorrow. The good news is all the 
tests

are passing with the fixes discussed in this thread.

On 3/21/19 6:53 PM, Chris Plummer wrote:

Hi Gary,

I admittedly know next to nothing about how jstat and jstatd interact, 
and also know little about RMI, but after reading the bug I can't help 
but think that your fix might just be hiding a bug else (like in RMI). 
For example, should UnicastRemoteObject.exportObject() be keeping the 
remoteHost object live?


thanks,

Chris

On 3/21/19 11:47 AM, Gary Adams wrote:
The jstatd failures look like the server remote endpoint was GC'd 
after it was bound.
A strong reference should prevent the NoSuchObjectException 
(JDK-8203026).
Was seeing 1 failure every 50 testruns. Currently through 400 test 
runs with

no failures with this proposed fix.

diff --git 
a/src/jdk.jstatd/share/classes/sun/tools/jstatd/Jstatd.java 
b/src/jdk.jstatd/share/classes/sun/tools/jstatd/Jstatd.java

--- a/src/jdk.jstatd/share/classes/sun/tools/jstatd/Jstatd.java
+++ b/src/jdk.jstatd/share/classes/sun/tools/jstatd/Jstatd.java
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2004, 2018, Oracle and/or its affiliates. All 
rights reserved.
+ * Copyright (c) 2004, 2019, 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
@@ -45,6 +45,7 @@
 private static Registry registry;
 private static int port = -1;
 private static boolean startRegistry = true;
+    private static RemoteHostImpl remoteHost;

 private static void printUsage() {
 System.err.println("usage: jstatd [-nr] [-p port] [-n 
rminame]\n" +

@@ -138,7 +139,7 @@
 try {
 // use 1.5.0 dynamically generated subs.
System.setProperty("java.rmi.server.ignoreSubClasses", "true");
-    RemoteHostImpl remoteHost = new RemoteHostImpl();
+    remoteHost = new RemoteHostImpl();
 RemoteHost stub = (RemoteHost) 
UnicastRemoteObject.exportObject(

 remoteHost, 0);
 bind(name.toString(), remoteHost);

diff --git 
a/src/jdk.jstatd/share/classes/sun/tools/jstatd/RemoteHostImpl.java 
b/src/jdk.jstatd/share/classes/sun/tools/jstatd/RemoteHostImpl.java

--- a/src/jdk.jstatd/share/classes/sun/tools/jstatd/RemoteHostImpl.java
+++ b/src/jdk.jstatd/share/classes/sun/tools/jstatd/RemoteHostImpl.java
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2004, Oracle and/or its affiliates. All rights 
reserved.
+ * Copyright (c) 2004, 2019, 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
@@ -50,6 +50,7 @@

 private MonitoredHost monitoredHost;
 private Set activeVms;
+    private static RemoteVmImpl rvm;

 public RemoteHostImpl() throws MonitorException {
 try {
@@ -76,7 +77,7 @@
 try {
 VmIdentifier vmid = new VmIdentifier(vmidStr);
 MonitoredVm mvm = monitoredHost.getMonitoredVm(vmid);
-    RemoteVmImpl rvm = new 
RemoteVmImpl((BufferedMonitoredVm)mvm);

+    rvm = new RemoteVmImpl((BufferedMonitoredVm)mvm);
 stub = (RemoteVm) UnicastRemoteObject.exportObject(rvm, 0);
 }
 catch (URISyntaxException e) {


On 3/20/19, 8:26 AM, Gary Adams wrote:

I think I have found the source of the dash.
 sun/tools/jstat/RowClosure.java
sets the NaN value to "-".

That should be fine.

It just means in the jstat output tests need to account for the 
possibility.


...

On 3/19/19, 6:07 PM, Chris Plummer wrote:
The presence of the '-' sounds familiar. I recall reading about it 
in a review. I know I did a review for these tests that impacted 
the output, but I don't see it introducing a '-' anywhere. See 
JDK-8202466. However, that bug references JDK-8199519, which caused 
problems by changing a value in the output from a '-' to a 0:


--- 
a/test/hotspot/jtreg/serviceability/tmtools/jstat/utils/JstatGcCauseResults.java 
Wed Apr 25 17:50:32 2018 -0400
+++ 
b/test/hotspot/jtreg/serviceability/tmtools/jstat/utils/JstatGcCauseResults.java 
Thu Apr 26 09:45:47 2018 +0900

@@ -74,10 +74,18 @@
assertThat(GCT >= 0, "Incorrect time value for GCT");
assertThat(GCT >= YGCT, "GCT < YGCT (total garbage collection time 
< young generation garbage collection time)");


- int CGC = getIntValue("CGC");
- float CGCT = getFloatValue("CGCT");
- assertThat(CGCT >= 0, "I

Re: RFR: JDK-8220295: sun/tools/jps/TestJps.java still timing out

[Gary Adams]

The jstatd failures look like the server remote endpoint was GC'd after 
it was bound.

A strong reference should prevent the NoSuchObjectException (JDK-8203026).
Was seeing 1 failure every 50 testruns. Currently through 400 test runs 
with

no failures with this proposed fix.

diff --git a/src/jdk.jstatd/share/classes/sun/tools/jstatd/Jstatd.java 
b/src/jdk.jstatd/share/classes/sun/tools/jstatd/Jstatd.java

--- a/src/jdk.jstatd/share/classes/sun/tools/jstatd/Jstatd.java
+++ b/src/jdk.jstatd/share/classes/sun/tools/jstatd/Jstatd.java
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2004, 2018, Oracle and/or its affiliates. All rights 
reserved.
+ * Copyright (c) 2004, 2019, 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
@@ -45,6 +45,7 @@
 private static Registry registry;
 private static int port = -1;
 private static boolean startRegistry = true;
+    private static RemoteHostImpl remoteHost;

 private static void printUsage() {
 System.err.println("usage: jstatd [-nr] [-p port] [-n 
rminame]\n" +

@@ -138,7 +139,7 @@
 try {
 // use 1.5.0 dynamically generated subs.
System.setProperty("java.rmi.server.ignoreSubClasses", "true");
-    RemoteHostImpl remoteHost = new RemoteHostImpl();
+    remoteHost = new RemoteHostImpl();
 RemoteHost stub = (RemoteHost) 
UnicastRemoteObject.exportObject(

 remoteHost, 0);
 bind(name.toString(), remoteHost);

diff --git 
a/src/jdk.jstatd/share/classes/sun/tools/jstatd/RemoteHostImpl.java 
b/src/jdk.jstatd/share/classes/sun/tools/jstatd/RemoteHostImpl.java

--- a/src/jdk.jstatd/share/classes/sun/tools/jstatd/RemoteHostImpl.java
+++ b/src/jdk.jstatd/share/classes/sun/tools/jstatd/RemoteHostImpl.java
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2004, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2004, 2019, 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
@@ -50,6 +50,7 @@

 private MonitoredHost monitoredHost;
 private Set activeVms;
+    private static RemoteVmImpl rvm;

 public RemoteHostImpl() throws MonitorException {
 try {
@@ -76,7 +77,7 @@
 try {
 VmIdentifier vmid = new VmIdentifier(vmidStr);
 MonitoredVm mvm = monitoredHost.getMonitoredVm(vmid);
-    RemoteVmImpl rvm = new RemoteVmImpl((BufferedMonitoredVm)mvm);
+    rvm = new RemoteVmImpl((BufferedMonitoredVm)mvm);
 stub = (RemoteVm) UnicastRemoteObject.exportObject(rvm, 0);
 }
 catch (URISyntaxException e) {


On 3/20/19, 8:26 AM, Gary Adams wrote:

I think I have found the source of the dash.
 sun/tools/jstat/RowClosure.java
sets the NaN value to "-".

That should be fine.

It just means in the jstat output tests need to account for the 
possibility.


...

On 3/19/19, 6:07 PM, Chris Plummer wrote:
The presence of the '-' sounds familiar. I recall reading about it in 
a review. I know I did a review for these tests that impacted the 
output, but I don't see it introducing a '-' anywhere. See 
JDK-8202466. However, that bug references JDK-8199519, which caused 
problems by changing a value in the output from a '-' to a 0:


--- 
a/test/hotspot/jtreg/serviceability/tmtools/jstat/utils/JstatGcCauseResults.java 
Wed Apr 25 17:50:32 2018 -0400
+++ 
b/test/hotspot/jtreg/serviceability/tmtools/jstat/utils/JstatGcCauseResults.java 
Thu Apr 26 09:45:47 2018 +0900

@@ -74,10 +74,18 @@
assertThat(GCT >= 0, "Incorrect time value for GCT");
assertThat(GCT >= YGCT, "GCT < YGCT (total garbage collection time < 
young generation garbage collection time)");


- int CGC = getIntValue("CGC");
- float CGCT = getFloatValue("CGCT");
- assertThat(CGCT >= 0, "Incorrect time value for CGCT");
+ int CGC = 0;
+ float CGCT = 0.0f;
+ try {
+ CGC = getIntValue("CGC");
+ } catch (NumberFormatException e) {
+ if (!e.getMessage().equals("Unparseable number: \"-\"")) {
+ throw e;
+ }
+ }
if (CGC > 0) {
+ CGCT = getFloatValue("CGCT");
+ assertThat(CGCT >= 0, "Incorrect time value for CGCT");
assertThat(CGCT > 0, "Number of concurrent GC events is " + CGC + ", 
but CGCT is 0");

}
I'm not sure if any of this is related to what you are seeing. Chris

On 3/19/19 11:39 AM, Gary Adams wrote:

A quick follow up on the jstat test failures.

On the failed runs the output looks like this :

--messages:(3/127)--
command: shell jstatLineCounts4.sh
reason: User specified action: run shell jstatLineCounts4.sh
elapsed time (seconds): 7.496
--System.out:(13/1261)--

Re: RFR: JDK-8220295: sun/tools/jps/TestJps.java still timing out

[Gary Adams]

I think I have found the source of the dash.
 sun/tools/jstat/RowClosure.java
sets the NaN value to "-".

That should be fine.

It just means in the jstat output tests need to account for the possibility.

...

On 3/19/19, 6:07 PM, Chris Plummer wrote:
The presence of the '-' sounds familiar. I recall reading about it in 
a review. I know I did a review for these tests that impacted the 
output, but I don't see it introducing a '-' anywhere. See 
JDK-8202466. However, that bug references JDK-8199519, which caused 
problems by changing a value in the output from a '-' to a 0:


--- 
a/test/hotspot/jtreg/serviceability/tmtools/jstat/utils/JstatGcCauseResults.java
Wed Apr 25 17:50:32 2018 -0400
+++ 
b/test/hotspot/jtreg/serviceability/tmtools/jstat/utils/JstatGcCauseResults.java
Thu Apr 26 09:45:47 2018 +0900
@@ -74,10 +74,18 @@
  assertThat(GCT>= 0, "Incorrect time value for GCT");
  assertThat(GCT>= YGCT, "GCT<  YGCT (total garbage collection time<  young 
generation garbage collection time)");
  
-int CGC = getIntValue("CGC");

-float CGCT = getFloatValue("CGCT");
-assertThat(CGCT>= 0, "Incorrect time value for CGCT");
+int CGC = 0;
+float CGCT = 0.0f;
+try {
+CGC = getIntValue("CGC");
+} catch (NumberFormatException e) {
+if (!e.getMessage().equals("Unparseable number: \"-\"")) {
+throw e;
+}
+}
  if (CGC>  0) {
+CGCT = getFloatValue("CGCT");
+assertThat(CGCT>= 0, "Incorrect time value for CGCT");
  assertThat(CGCT>  0, "Number of concurrent GC events is " + CGC + ", 
but CGCT is 0");
  }

I'm not sure if any of this is related to what you are seeing.

Chris

On 3/19/19 11:39 AM, Gary Adams wrote:

A quick follow up on the jstat test failures.

On the failed runs the output looks like this :

--messages:(3/127)--
command: shell jstatLineCounts4.sh
reason: User specified action: run shell jstatLineCounts4.sh
elapsed time (seconds): 7.496
--System.out:(13/1261)--
   S0 S1 E  O  M CCSYGC YGCTFGCFGCTCGC  
  CGCT GCT
   0.00   0.00   0.00   0.00  -  -  00.000 00.000 0 
   0.0000.000
   0.00   0.00   0.00   0.00  -  -  00.000 00.000 0 
   0.0000.000
   0.00   0.00   0.00   0.00  -  -  00.000 00.000 0 
   0.0000.000
   0.00   0.00   0.00   0.00  -  -  00.000 00.000 0 
   0.0000.000
   0.00   0.00   0.00   0.00  -  -  00.000 00.000 0 
   0.0000.000
   0.00   0.00   0.00   0.00  -  -  00.000 00.000 0 
   0.0000.000
   0.00   0.00   0.00   0.00  -  28.19  10.571 00.000 0 
   0.0000.571
   0.00 100.00   0.00  14.85  31.29  28.19  10.571 00.000 0 
   0.0000.571
   0.00 100.00   0.00  14.85  31.29  28.19  10.571 00.000 0 
   0.0000.571
   0.00 100.00   0.00  14.85  31.29  28.19  10.571 00.000 0 
   0.0000.571
   S0 S1 E  O  M CCSYGC YGCTFGCFGCTCGC  
  CGCT GCT
   0.00 100.00   0.00  14.85  31.29  28.19  10.571 00.000 0 
   0.0000.571

The awk scripts used to check the output are not aware a dash '-' is 
allowed the metaspace column.

Here is a quick stab to allow the dashes.
Is anyone aware of recent changes in the gcutil output?

diff --git a/test/jdk/sun/tools/jstat/lineCounts3.awk 
b/test/jdk/sun/tools/jstat/lineCounts3.awk

--- a/test/jdk/sun/tools/jstat/lineCounts3.awk
+++ b/test/jdk/sun/tools/jstat/lineCounts3.awk
@@ -23,7 +23,7 @@
 headerlines++;
 }

-/^[ ]*[0-9]+\.[0-9]+[ ]*[0-9]+\.[0-9]+[ ]*[0-9]+\.[0-9]+[ 
]*[0-9]+\.[0-9]+[ ]*[0-9]+\.[0-9]+[ ]*([0-9]+\.[0-9]+)|-[ ]*[0-9]+[ 
]*[0-9]+\.[0-9]+[ ]*[0-9]+[ ]*[0-9]+\.[0-9]+[ ]*[0-9]+[ 
]*[0-9]+\.[0-9]+[ ]*[0-9]+\.[0-9]+$/{
+/^[ ]*[0-9]+\.[0-9]+[ ]*[0-9]+\.[0-9]+[ ]*[0-9]+\.[0-9]+[ 
]*[0-9]+\.[0-9]+[ ]*([0-9]+\.[0-9]+)|-[ ]*([0-9]+\.[0-9]+)|-[ 
]*[0-9]+[ ]*[0-9]+\.[0-9]+[ ]*[0-9]+[ ]*[0-9]+\.[0-9]+[ ]*[0-9]+[ 
]*[0-9]+\.[0-9]+[ ]*[0-9]+\.[0-9]+$/{

 datalines++;
 }

diff --git a/test/jdk/sun/tools/jstat/lineCounts4.awk 
b/test/jdk/sun/tools/jstat/lineCounts4.awk

--- a/test/jdk/sun/tools/jstat/lineCounts4.awk
+++ b/test/jdk/sun/tools/jstat/lineCounts4.awk
@@ -26,7 +26,7 @@
 headerlines++;
 }

-/^[ ]*[0-9]+\.[0-9]+[ ]*[0-9]+\.[0-9]+[ ]*[0-9]+\.[0-9]+[ 
]*[0-9]+\.[0-9]+[ ]*[0-9]+\.[0-9]+[ ]*([0-9]+\.[0-9]+)|-[ ]*[0-9]+[ 
]*[0-9]+\.[0-9]+[ ]*[0-9]+[ ]*[0-9]+\.[0-9]+[ ]*[0-9]+[ 
]*[0-9]+\.[0-9]+[ ]*[0-9]+\.[0-9]+$/{
+/^[ ]*[0-9]+\.[0-9]+[ ]*[0-9]+

Re: RFR: JDK-8220295: sun/tools/jps/TestJps.java still timing out

[Gary Adams]

A quick follow up on the jstat test failures.

On the failed runs the output looks like this :

--messages:(3/127)--
command: shell jstatLineCounts4.sh
reason: User specified action: run shell jstatLineCounts4.sh
elapsed time (seconds): 7.496
--System.out:(13/1261)--
  S0 S1 E  O  M CCSYGC YGCTFGCFGCTCGC   
 CGCT GCT
  0.00   0.00   0.00   0.00  -  -  00.000 00.000 0  
  0.0000.000
  0.00   0.00   0.00   0.00  -  -  00.000 00.000 0  
  0.0000.000
  0.00   0.00   0.00   0.00  -  -  00.000 00.000 0  
  0.0000.000
  0.00   0.00   0.00   0.00  -  -  00.000 00.000 0  
  0.0000.000
  0.00   0.00   0.00   0.00  -  -  00.000 00.000 0  
  0.0000.000
  0.00   0.00   0.00   0.00  -  -  00.000 00.000 0  
  0.0000.000
  0.00   0.00   0.00   0.00  -  28.19  10.571 00.000 0  
  0.0000.571
  0.00 100.00   0.00  14.85  31.29  28.19  10.571 00.000 0  
  0.0000.571
  0.00 100.00   0.00  14.85  31.29  28.19  10.571 00.000 0  
  0.0000.571
  0.00 100.00   0.00  14.85  31.29  28.19  10.571 00.000 0  
  0.0000.571
  S0 S1 E  O  M CCSYGC YGCTFGCFGCTCGC   
 CGCT GCT
  0.00 100.00   0.00  14.85  31.29  28.19  10.571 00.000 0  
  0.0000.571


The awk scripts used to check the output are not aware a dash '-' is 
allowed the metaspace column.

Here is a quick stab to allow the dashes.
Is anyone aware of recent changes in the gcutil output?

diff --git a/test/jdk/sun/tools/jstat/lineCounts3.awk 
b/test/jdk/sun/tools/jstat/lineCounts3.awk

--- a/test/jdk/sun/tools/jstat/lineCounts3.awk
+++ b/test/jdk/sun/tools/jstat/lineCounts3.awk
@@ -23,7 +23,7 @@
 headerlines++;
 }

-/^[ ]*[0-9]+\.[0-9]+[ ]*[0-9]+\.[0-9]+[ ]*[0-9]+\.[0-9]+[ 
]*[0-9]+\.[0-9]+[ ]*[0-9]+\.[0-9]+[ ]*([0-9]+\.[0-9]+)|-[ ]*[0-9]+[ 
]*[0-9]+\.[0-9]+[ ]*[0-9]+[ ]*[0-9]+\.[0-9]+[ ]*[0-9]+[ 
]*[0-9]+\.[0-9]+[ ]*[0-9]+\.[0-9]+$/{
+/^[ ]*[0-9]+\.[0-9]+[ ]*[0-9]+\.[0-9]+[ ]*[0-9]+\.[0-9]+[ 
]*[0-9]+\.[0-9]+[ ]*([0-9]+\.[0-9]+)|-[ ]*([0-9]+\.[0-9]+)|-[ ]*[0-9]+[ 
]*[0-9]+\.[0-9]+[ ]*[0-9]+[ ]*[0-9]+\.[0-9]+[ ]*[0-9]+[ 
]*[0-9]+\.[0-9]+[ ]*[0-9]+\.[0-9]+$/{

 datalines++;
 }

diff --git a/test/jdk/sun/tools/jstat/lineCounts4.awk 
b/test/jdk/sun/tools/jstat/lineCounts4.awk

--- a/test/jdk/sun/tools/jstat/lineCounts4.awk
+++ b/test/jdk/sun/tools/jstat/lineCounts4.awk
@@ -26,7 +26,7 @@
 headerlines++;
 }

-/^[ ]*[0-9]+\.[0-9]+[ ]*[0-9]+\.[0-9]+[ ]*[0-9]+\.[0-9]+[ 
]*[0-9]+\.[0-9]+[ ]*[0-9]+\.[0-9]+[ ]*([0-9]+\.[0-9]+)|-[ ]*[0-9]+[ 
]*[0-9]+\.[0-9]+[ ]*[0-9]+[ ]*[0-9]+\.[0-9]+[ ]*[0-9]+[ 
]*[0-9]+\.[0-9]+[ ]*[0-9]+\.[0-9]+$/{
+/^[ ]*[0-9]+\.[0-9]+[ ]*[0-9]+\.[0-9]+[ ]*[0-9]+\.[0-9]+[ 
]*[0-9]+\.[0-9]+[ ]*([0-9]+\.[0-9]+)|-[ ]*([0-9]+\.[0-9]+)|-[ ]*[0-9]+[ 
]*[0-9]+\.[0-9]+[ ]*[0-9]+[ ]*[0-9]+\.[0-9]+[ ]*[0-9]+[ 
]*[0-9]+\.[0-9]+[ ]*[0-9]+\.[0-9]+$/{

 if (headerlines == 2) {
 datalines2++;
 }


On 3/19/19, 8:22 AM, Gary Adams wrote:

After 1000 testruns on {solaris,linux,windows,macosx} debug builds
running
  test/jdk/sun/tools
  test/jdk/com/sun/tools/attach

with this change, no failures were observed in TestJps or TempDirTest.

diff --git a/test/jdk/TEST.ROOT b/test/jdk/TEST.ROOT
--- a/test/jdk/TEST.ROOT
+++ b/test/jdk/TEST.ROOT
@@ -22,7 +22,11 @@
 javax/management sun/awt sun/java2d javax/xml/jaxp/testng/validation 
java/lang/ProcessHandle


 # Tests that cannot run concurrently
-exclusiveAccess.dirs=java/rmi/Naming java/util/prefs 
sun/management/jmxremote sun/tools/jstatd sun/security/mscapi 
java/util/stream java/util/Arrays/largeMemory java/util/BitSet/stream 
javax/rmi
+exclusiveAccess.dirs=java/rmi/Naming java/util/prefs 
sun/management/jmxremote \
+sun/tools/jstatd sun/tools/jcmd sun/tools/jhsdb 
sun/tools/jhsdb/heapconfig \
+sun/tools/jinfo sun/tools/jmap sun/tools/jps sun/tools/jstack 
sun/tools/jstat \
+com/sun/tools/attach sun/security/mscapi java/util/stream 
java/util/Arrays/largeMemory \

+java/util/BitSet/stream javax/rmi
 # Group definitions
 groups=TEST.groups

Failures were observed in the following tests.
sun/tools/jstat/jstatLineCounts4.sh,[],[solaris-sparcv9-debug],[ExitCode: 
1],[bug4990825 shell], [jstatLineCounts4.sh],[37605],[] 
sun/tools/jstat/jstatLineCounts4.sh,[],[solaris-sparcv9-debug],[ExitCode: 
1],[bug4990825 shell], [jstatLineCounts4.sh],[37605],[]   
sun/tools/jstat/jstatLineCounts3.sh
sun/tools/jstat/jstatLineCounts4.sh,[],[solaris-sparcv9-debug],[ExitCode: 
1],[bug4990825 shell], [jstatLineCounts4.sh],[37605],[] 
sun/tools/jstat/jstatLineCounts4.sh,[],[solaris-sparcv9-debug],[ExitCode: 
1],[bug4990825 shell], [jstatLineCounts4.sh],[37605

RFR: JDK-8220295: sun/tools/jps/TestJps.java still timing out

[Gary Adams]

After 1000 testruns on {solaris,linux,windows,macosx} debug builds
running
  test/jdk/sun/tools
  test/jdk/com/sun/tools/attach

with this change, no failures were observed in TestJps or TempDirTest.

diff --git a/test/jdk/TEST.ROOT b/test/jdk/TEST.ROOT
--- a/test/jdk/TEST.ROOT
+++ b/test/jdk/TEST.ROOT
@@ -22,7 +22,11 @@
 javax/management sun/awt sun/java2d javax/xml/jaxp/testng/validation 
java/lang/ProcessHandle


 # Tests that cannot run concurrently
-exclusiveAccess.dirs=java/rmi/Naming java/util/prefs 
sun/management/jmxremote sun/tools/jstatd sun/security/mscapi 
java/util/stream java/util/Arrays/largeMemory java/util/BitSet/stream 
javax/rmi
+exclusiveAccess.dirs=java/rmi/Naming java/util/prefs 
sun/management/jmxremote \
+sun/tools/jstatd sun/tools/jcmd sun/tools/jhsdb 
sun/tools/jhsdb/heapconfig \
+sun/tools/jinfo sun/tools/jmap sun/tools/jps sun/tools/jstack 
sun/tools/jstat \
+com/sun/tools/attach sun/security/mscapi java/util/stream 
java/util/Arrays/largeMemory \

+java/util/BitSet/stream javax/rmi
 # Group definitions
 groups=TEST.groups

Failures were observed in the following tests.
sun/tools/jstat/jstatLineCounts4.sh,[],[solaris-sparcv9-debug],[ExitCode: 1],[bug4990825 
shell], [jstatLineCounts4.sh],[37605],[] 
sun/tools/jstat/jstatLineCounts4.sh,[],[solaris-sparcv9-debug],[ExitCode: 1],[bug4990825 
shell], [jstatLineCounts4.sh],[37605],[]   
sun/tools/jstat/jstatLineCounts3.sh
sun/tools/jstat/jstatLineCounts4.sh,[],[solaris-sparcv9-debug],[ExitCode: 1],[bug4990825 
shell], [jstatLineCounts4.sh],[37605],[] 
sun/tools/jstat/jstatLineCounts4.sh,[],[solaris-sparcv9-debug],[ExitCode: 1],[bug4990825 
shell], [jstatLineCounts4.sh],[37605],[]   
sun/tools/jstat/jstatLineCounts4.sh

  sun/tools/jstatd/TestJstatdDefaults.java
  sun/tools/jstatd/TestJstatdServer.java
  sun/tools/jstatd/TestJstatdPort.java
  sun/tools/jstatd/TestJstatdExternalRegistry.java

I'll investigate those failures some more, but I don't think they are
related to the change to exclusiveAccess.dirs.

Issues:
  https://bugs.openjdk.java.net/browse/JDK-8220295
  https://bugs.openjdk.java.net/browse/JDK-8220242

At this point just looking for feedback, if this is a reasonable direction
to deal with the intermittent timeouts with theses tests to reduce the
concurrency with other attaching tests.

RFR: JDK-8220678: unquarantine nsk/jdi/ThreadReference/setEnabled/setenabled003

[Gary Adams]

This trivial changeset will restore setenabled003 testing to jdk 13
where the test does not appear to be failing.

The original bug JDK-8066993 will be left open in case sustaining
team needs to apply changes in older releases.

diff --git a/test/hotspot/jtreg/ProblemList.txt 
b/test/hotspot/jtreg/ProblemList.txt

--- a/test/hotspot/jtreg/ProblemList.txt
+++ b/test/hotspot/jtreg/ProblemList.txt
@@ -158,7 +158,6 @@
 
vmTestbase/nsk/monitoring/ThreadMXBean/ThreadInfo/Deadlock/JavaDeadlock001/TestDescription.java
 8060733 generic-all

 vmTestbase/nsk/jdi/ThreadReference/stop/stop001/TestDescription.java 
7034630 generic-all
-vmTestbase/nsk/jdi/EventRequest/setEnabled/setenabled003/TestDescription.java 
8066993 generic-all

 
vmTestbase/nsk/jdi/VirtualMachine/redefineClasses/redefineclasses021/TestDescription.java
 8065773 generic-all
 
vmTestbase/nsk/jdi/VirtualMachine/redefineClasses/redefineclasses023/TestDescription.java
 8065773 generic-all

Re: RFR: JDK-8218166: [Graal] com/sun/jdi/SimulResumerTest.java failure

[Gary Adams]

One last set of diffs ...
  - added comments on the ignored exceptions
  - commented out excessive diagnostic print out
 (this will remove the jtreg truncated output)

Ok to use dan, dean and jc as reveiwers?

diff --git a/test/jdk/com/sun/jdi/SimulResumerTest.java 
b/test/jdk/com/sun/jdi/SimulResumerTest.java

--- a/test/jdk/com/sun/jdi/SimulResumerTest.java
+++ b/test/jdk/com/sun/jdi/SimulResumerTest.java
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2008, 2015, Oracle and/or its affiliates. All rights 
reserved.
+ * Copyright (c) 2008, 2019, 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
@@ -210,7 +210,9 @@
 }

 } catch (IncompatibleThreadStateException ee) {
-// ignore
+// ignore checks if thread was not suspended
+} catch (ObjectCollectedException ee) {
+// ignore checks if thread was collected
 } catch (VMDisconnectedException ee) {
 // This is how we stop.  The debuggee runs to completion
 // and we get this exception.
@@ -249,7 +251,7 @@
 public void run() {
 while (true) {
 iters++;
-System.out.println("bkpts = " + bkpts + ", 
iters = " + iters);
+// System.out.println("bkpts = " + bkpts + ", 
iters = " + iters);

 try {
 Thread.sleep(waitTime);
 check(debuggeeThread1);



On 3/7/19, 8:19 AM, Gary Adams wrote:

While trying to reproduce the timeout reported in
  JDK-8000669: com/sun/jdi/SimulResumerTest.java times out

I was unable to reproduce the timeout failure, but I did occasionally
see the ObjectCollectedException. The output from the test is very 
verbose
and may be the source of the occasional timeout. I'd like to close 
JDK-8000669
as cannot reproduce and if it shows up again look into limiting the 
amount

of non-essential output from the test.

This is a racy test to begin with and it already is ignoring exceptions
due to unexpected thread states. Adding the ignore for 
ObjectCollectedException

allows the test to complete without errors.

The graal label was recently removed. We should also remove it from 
the summary.


Proposed changeset:


diff --git a/test/jdk/com/sun/jdi/SimulResumerTest.java 
b/test/jdk/com/sun/jdi/SimulResumerTest.java

--- a/test/jdk/com/sun/jdi/SimulResumerTest.java
+++ b/test/jdk/com/sun/jdi/SimulResumerTest.java
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2008, 2015, Oracle and/or its affiliates. All rights 
reserved.
+ * Copyright (c) 2008, 2019, 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
@@ -211,6 +211,8 @@

 } catch (IncompatibleThreadStateException ee) {
 // ignore
+} catch (ObjectCollectedException ee) {
+// ignore
 } catch (VMDisconnectedException ee) {
 // This is how we stop.  The debuggee runs to completion
 // and we get this exception.

Re: JDK-8218166: [Graal] com/sun/jdi/SimulResumerTest.java failure

[Gary Adams]

There is a caution in the disableCollection() docs that it should be used
sparingly, because the behavior between debug and non-debug runs
could differ. I actually prefer leaving the test in it's racy configuration.
The collection of the object is not central to what the test is actually
trying to observe. Ignoring the exception if it does occur allows the
test to complete the work it intends to cover.

On 3/12/19, 2:22 PM, Daniil Titov wrote:

Hi Gary,

The fix looks good. However, since there are only two references 
(debuggeeThread1 and debuggeeThread2) the test checks, an alternative to 
catching and ignoring ObjectCollectedException  could be just calling 
disableCollection() on these two thread references when a breakpoint handle is 
called (lines 130 and 135).

Thanks!

Best regards,
Daniil



On 3/12/19, 11:05 AM, "serviceability-dev on behalf of Gary 
Adams"  wrote:

 Still need 2 more reviewers ...

 On 3/7/19, 9:45 AM, Daniel D. Daugherty wrote:
 >  Gary,
 >
 >  Since the 'graal' label was recently removed, I also removed "[Graal]"
 >  from the bug synopsis. Please make sure you update your commit mesg.
 >
     >
 >  On 3/7/19 8:19 AM, Gary Adams wrote:
 >>  While trying to reproduce the timeout reported in
 >>JDK-8000669: com/sun/jdi/SimulResumerTest.java times out
 >>
 >>  I was unable to reproduce the timeout failure, but I did occasionally
 >>  see the ObjectCollectedException. The output from the test is very
 >>  verbose
 >>  and may be the source of the occasional timeout. I'd like to close
 >>  JDK-8000669
 >>  as cannot reproduce and if it shows up again look into limiting the
 >>  amount
 >>  of non-essential output from the test.
 >>
 >>  This is a racy test to begin with and it already is ignoring exceptions
 >>  due to unexpected thread states. Adding the ignore for
 >>  ObjectCollectedException
 >>  allows the test to complete without errors.
 >>
 >>  The graal label was recently removed. We should also remove it from
 >>  the summary.
 >>
 >>  Proposed changeset:
 >>
 >>
 >>  diff --git a/test/jdk/com/sun/jdi/SimulResumerTest.java
 >>  b/test/jdk/com/sun/jdi/SimulResumerTest.java
 >>  --- a/test/jdk/com/sun/jdi/SimulResumerTest.java
 >>  +++ b/test/jdk/com/sun/jdi/SimulResumerTest.java
 >>  @@ -1,5 +1,5 @@
 >>   /*
 >>  - * Copyright (c) 2008, 2015, Oracle and/or its affiliates. All
 >>  rights reserved.
 >>  + * Copyright (c) 2008, 2019, 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
 >>  @@ -211,6 +211,8 @@
 >>
 >>   } catch (IncompatibleThreadStateException ee) {
 >>   // ignore
 >>  +} catch (ObjectCollectedException ee) {
 >>  +// ignore
 >>   } catch (VMDisconnectedException ee) {
 >>   // This is how we stop.  The debuggee runs to
 >>  completion
 >>   // and we get this exception.
 >
 >  There should be some sort of comment explaining why it is okay to ignore
 >  the ObjectCollectedException. When the IncompatibleThreadStateException
 >  was ignored, there should have been a comment added for that also.
 >
 >  Dan
 >

Re: RFR: JDK-8218166: [Graal] com/sun/jdi/SimulResumerTest.java failure

[Gary Adams]

Still need 2 more reviewers ...

On 3/7/19, 9:45 AM, Daniel D. Daugherty wrote:

Gary,

Since the 'graal' label was recently removed, I also removed "[Graal]"
from the bug synopsis. Please make sure you update your commit mesg.


On 3/7/19 8:19 AM, Gary Adams wrote:

While trying to reproduce the timeout reported in
  JDK-8000669: com/sun/jdi/SimulResumerTest.java times out

I was unable to reproduce the timeout failure, but I did occasionally
see the ObjectCollectedException. The output from the test is very 
verbose
and may be the source of the occasional timeout. I'd like to close 
JDK-8000669
as cannot reproduce and if it shows up again look into limiting the 
amount

of non-essential output from the test.

This is a racy test to begin with and it already is ignoring exceptions
due to unexpected thread states. Adding the ignore for 
ObjectCollectedException

allows the test to complete without errors.

The graal label was recently removed. We should also remove it from 
the summary.


Proposed changeset:


diff --git a/test/jdk/com/sun/jdi/SimulResumerTest.java 
b/test/jdk/com/sun/jdi/SimulResumerTest.java

--- a/test/jdk/com/sun/jdi/SimulResumerTest.java
+++ b/test/jdk/com/sun/jdi/SimulResumerTest.java
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2008, 2015, Oracle and/or its affiliates. All 
rights reserved.
+ * Copyright (c) 2008, 2019, 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
@@ -211,6 +211,8 @@

 } catch (IncompatibleThreadStateException ee) {
 // ignore
+} catch (ObjectCollectedException ee) {
+// ignore
 } catch (VMDisconnectedException ee) {
 // This is how we stop.  The debuggee runs to 
completion

 // and we get this exception.


There should be some sort of comment explaining why it is okay to ignore
the ObjectCollectedException. When the IncompatibleThreadStateException
was ignored, there should have been a comment added for that also.

Dan

RFR: JDK-8220257: fix headings in java.instrument

[Gary Adams]

Need one reviewer for this trivial correction .

After pushing the GPL header update, the h3 to h2 tags will be updated.
The copyright will be changed with the headings, so I'll push that 
change first.
  JDK-8220474: Incorrect GPL header in 
src/java.instrument/share/classes/java/lang/instrument/package-info.java


diff --git 
a/src/java.instrument/share/classes/java/lang/instrument/package-info.java 
b/src/java.instrument/share/classes/java/lang/instrument/package-info.java
--- 
a/src/java.instrument/share/classes/java/lang/instrument/package-info.java
+++ 
b/src/java.instrument/share/classes/java/lang/instrument/package-info.java

@@ -1,11 +1,11 @@
 /*
- * Copyright (c) 2003, 2017, Oracle and/or its affiliates. All rights 
reserved.
+ * Copyright (c) 2003, 2019, 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.  Oracle designates this
- * particular file as subject to the Classpath exception as provided
+ * particular file as subject to the "Classpath" exception as provided
  * by Oracle in the LICENSE file that accompanied this code.
  *
  * This code is distributed in the hope that it will be useful, but 
WITHOUT

@@ -52,7 +52,7 @@
  *  Each of these ways to start an agent is described below.
  *
  *
- * Starting an Agent from the Command-Line Interface
+ * Starting an Agent from the Command-Line Interface
  *
  *  Where an implementation provides a means to start agents from the
  * command-line interface, an agent is started by adding the following 
option

@@ -113,7 +113,7 @@
  * threads, is legal from {@code premain}.
  *
  *
- * Starting an Agent After VM Startup
+ * Starting an Agent After VM Startup
  *
  *  An implementation may provide a mechanism to start agents 
sometime after

  * the the VM has started. The details as to how this is initiated are
@@ -164,7 +164,7 @@
  * by the JVM for troubleshooting purposes).
  *
  *
- * Including an Agent in an Executable JAR file
+ * Including an Agent in an Executable JAR file
  *
  *  The JAR File Specification defines manifest attributes for 
standalone

  * applications that are packaged as executable JAR files. If an
@@ -194,8 +194,8 @@
  * an uncaught exception or error, the JVM will abort.
  *
  *
- *  Loading agent classes and the modules/classes available to the 
agent

- * class 
+ *  Loading agent classes and the modules/classes available to the 
agent

+ * class 
  *
  *  Classes loaded from the agent JAR file are loaded by the
  * {@linkplain ClassLoader#getSystemClassLoader() system class loader} 
and are

@@ -242,7 +242,7 @@
  * In other words, a custom system class loader must support the 
mechanism to

  * add an agent JAR file to the system class loader search.
  *
- * Manifest Attributes
+ * Manifest Attributes
  *
  *  The following manifest attributes are defined for an agent JAR 
file:

  *
@@ -311,7 +311,7 @@
  * ignored).
  *
  *
- * Instrumenting code in modules
+ * Instrumenting code in modules
  *
  *  As an aid to agents that deploy supporting classes on the 
search path of
  * the bootstrap class loader, or the search path of the class loader 
that loads

@@ -323,4 +323,4 @@
  * @revised 9
  */

RFR: JDK-8220474: Incorrect GPL header in src/java.instrument/share/classes/java/lang/instrument/package-info.java

[Gary Adams]

Need one reviewer for this trivial correction .

  Issue: https://bugs.openjdk.java.net/browse/JDK-8220474

diff --git 
a/src/java.instrument/share/classes/java/lang/instrument/package-info.java 
b/src/java.instrument/share/classes/java/lang/instrument/package-info.java
--- 
a/src/java.instrument/share/classes/java/lang/instrument/package-info.java
+++ 
b/src/java.instrument/share/classes/java/lang/instrument/package-info.java

@@ -1,11 +1,11 @@
 /*
- * Copyright (c) 2003, 2017, Oracle and/or its affiliates. All rights 
reserved.
+ * Copyright (c) 2003, 2019, 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.  Oracle designates this
- * particular file as subject to the Classpath exception as provided
+ * particular file as subject to the "Classpath" exception as provided
  * by Oracle in the LICENSE file that accompanied this code.
  *
  * This code is distributed in the hope that it will be useful, but 
WITHOUT

RFR: JDK-8220295: sun/tools/jps/TestJps.java still timing out

[Gary Adams]

These attach tests interact with all running Java processes.
From the logs of the failing tests you can see a few infrastructure
Java processes and a number of other Java attaching tests including
jinfo, jmap, jstat, etc.

  JDK-8220295: sun/tools/jps/TestJps.java still timing out
  JDK-8220242: com/sun/tools/attach/TempDirTest.java sometimes times out

It's not clear what happens, if these processes are crossing their
attach requests. To simplify the interactions, the following change
will request the attaching tests to be run non-concurrently.
Testing is in progress. If successful, it may be possible to reduce the
timeout because a fixed number of Java processes will be running in each
test run.

diff --git a/test/jdk/TEST.ROOT b/test/jdk/TEST.ROOT
--- a/test/jdk/TEST.ROOT
+++ b/test/jdk/TEST.ROOT
@@ -22,7 +22,7 @@
 javax/management sun/awt sun/java2d javax/xml/jaxp/testng/validation 
java/lang/ProcessHandle


 # Tests that cannot run concurrently
-exclusiveAccess.dirs=java/rmi/Naming java/util/prefs 
sun/management/jmxremote sun/tools/jstatd sun/security/mscapi 
java/util/stream java/util/Arrays/largeMemory java/util/BitSet/stream 
javax/rmi
+exclusiveAccess.dirs=java/rmi/Naming java/util/prefs 
sun/management/jmxremote sun/tools/jstatd sun/tools/jps com/sun/tools 
sun/security/mscapi java/util/stream java/util/Arrays/largeMemory 
java/util/BitSet/stream javax/rmi

 # Group definitions
 groups=TEST.groups

Re: RFR: JDK-8149461: jmap kills process if non-java pid is specified in the command line

[Gary Adams]

I'd like to bring closure to this issue.

Even if we could reliable detect libjvm has been loaded in
a process, it does not guarantee the VM has entered the
live phase, has set up the signal handlers and is prepared
proceed with the attach mechanism.

Without any further input, I'll close this bug on Wed Mar 13, 2019.
I'll add as much documentation as possible before closing the issue,
but it is unlikely to be repopened as the use case falls outside
the reasonable use of the feature.

On 2/19/19, 10:50 PM, David Holmes wrote:

On 19/02/2019 10:07 pm, gary.ad...@oracle.com wrote:

I'm fine with just closing the bug as will not fix.


I personally think it is a non-issue but others (Hi Thomas!) consider 
it a real problem.



The issue deals with a situation where the user provided the wrong pid
and the consequences were unexpected.

The changes I was prototyping demonstrate that it is possible to 
restrict

the cases where the attach would be limited to those Java processes
that are visible to jps.

Extending to those processes that are run with -XX:-UsePerfData will 
require

some additional mechanism. A command line approach would introduce
an incompatibility. Is there some other mechanism that could be used?


Only what Thomas suggested in using a platform specific mechanism to 
verify the pid is for a Java process. Though a simple binary name 
check doesn't suffice as the VM may be embedded in another process, so 
actually checking for libjvm as Thomas suggested may be the only 
practical and reliable way to do this.


Thanks,
David
-


On 2/15/19 9:20 PM, David Holmes wrote:
But this will still break existing behaviour as without UsePerfData 
you will now have to use -f whereas it currently "just works".


This will need a CSR request for any new flag or change in behaviour.

But to be blunt I don't like where this is going and the cure seems 
far worse than the disease.


David

On 16/02/2019 4:39 am, Gary Adams wrote:
It isn't pretty, but it's functional : "-f to force 
communication. ...


   Revised webrev: 
http://cr.openjdk.java.net/~gadams/8149461/webrev.02/


On 2/15/19, 11:57 AM, Daniel D. Daugherty wrote:

Yes. That's the direction I was thinking about.
Don't know about the '-F' versus '-F ', but
that a cmd line option parsing detail.

Dan


On 2/15/19 11:37 AM, Gary Adams wrote:

Here's a quick dirty "-F" that gets past
the "-XX:-UsePerfData" setting for jcmd.
Need to follow up on docs and usage
for the other commands.

Is this the direction you were thinking?

diff --git 
a/src/jdk.jcmd/share/classes/sun/tools/common/ProcessArgumentMatcher.javab/src/jdk.jcmd/share/classes/sun/tools/common/ProcessArgumentMatcher.java 

--- 
a/src/jdk.jcmd/share/classes/sun/tools/common/ProcessArgumentMatcher.java 

+++ 
b/src/jdk.jcmd/share/classes/sun/tools/common/ProcessArgumentMatcher.java 


@@ -48,16 +48,27 @@
 public class ProcessArgumentMatcher {
 private String matchClass;
 private String singlePid;
+private static boolean bypassPid;
+private long pid;

 public ProcessArgumentMatcher(String pidArg) {
 if (pidArg == null || pidArg.isEmpty()) {
 throw new IllegalArgumentException("Pid string is 
invalid");

 }
 if (pidArg.charAt(0) == '-') {
+if (pidArg.charAt(1) == 'F') {
+// Allow -F to bypass the pid check
+pid = Long.parseLong(pidArg.substring(2));
+if (pid != 0) {
+singlePid = String.valueOf(pid);
+}
+bypassPid = true;
+} else {
 throw new IllegalArgumentException("Unrecognized " + 
pidArg);

 }
+}
 try {
-long pid = Long.parseLong(pidArg);
+pid = Long.parseLong(pidArg);
 if (pid != 0) {
 singlePid = String.valueOf(pid);
 }
@@ -170,4 +181,21 @@
 public Collection getVirtualMachinePids() {
 return this.getVirtualMachinePids(null);
 }
+
+  // Check that pid matches a known running Java process
+  public static boolean checkJavaPid(String pid) {
+  // Skip the perf data pid visibility check if 
"-F" requested.

+  if (bypassPid) {
+  return true;
 }
+  List l = VirtualMachine.list();
+  boolean found = false;
+  for (VirtualMachineDescriptor vmd: l) {
+  if (vmd.id().equals(pid)) {
+  found = true;
+  break;
+  }
+  }
+  return found;
+  }
+}
On 2/15/19, 10:24 AM, Daniel D. Daugherty wrote:

On 2/15/19 8:44 AM, Gary Adams wrote:

Confirmed
  -XX:-UsePerfData

prevents visibility to jps, but allows direct access via pid.

The new check would block access before the attach is attempted.

May be best to close this bug as "will not fix".
Requires a valid Java process pid.


Or make it a best ef

Re: RFR: JDK-8218128: vmTestbase/nsk/jvmti/ResourceExhausted/resexhausted003 and 004 use wrong path to test classes

[Gary Adams]

On 3/8/19, 12:10 PM, Chris Plummer wrote:
You can remove the quarantine and exclude keywords. I think that's 
appropriate if the test is off the problemlist and working. It was 
just nonconcurrent removal that I was against.

Done


As for the resexhausted001 failure you are still seeing, how could 
jtreg exclude it if it was not on the problemlist. I didn't think 
there was any other mechanism. I don't believe jtreg looks at the 
tonga keywords.


I believe the @ignore is a jtreg keyword.
With the -ignore command line flag jtreg can be directed to
quietly ignore a test, or force an error, or attempt to run the test
even though the @ignore directive is there.



thanks,

Chris

On 3/8/19 5:07 AM, Gary Adams wrote:

I'll revert the comments that were left in during the tonga conversion.

I did come across an interesting test failure in resexhausted001
which had an
  @ignore 7013634

JDK-7013634: jvmti resexhausted001 can timeout or fail due to 
excessive thread creation


The test was closed because it was not reproducible.
Even though the test was not on the ProblemList, I believe
jtreg was excluding the test from running.

The original problem reported an "out of swap" condition.

The current failure reports:
--System.out:(3/217)--
Creating threads...
Timeout refired 480 times
[730.871s][warning][os,thread] Failed to start thread - _beginthreadex failed 
(EACCES) for attributes: stacksize: default, flags: CREATE_SUSPENDED 
STACK_SIZE_PARAM_IS.


On 3/7/19, 4:02 PM, Chris Plummer wrote:

Hi Gary,

Why did you remove the "nonconcurrent" keyword. I know these are 
just comments for reference that were added when the test was ported 
from tonga, but as a comment it is still applicable. The test should 
not be run concurrent with others (which you have also fixed with 
the addition of the "exclusiveAccess.dirs=.").


Otherwise changes look good.

thanks,

Chris

On 3/7/19 10:57 AM, Gary Adams wrote:

This proposed fix will restore the ResourceExhausted tests.

Test 3 and 4 were on the ProblemList because of the potential
path issues in finding the correct classes. This change searches the
test.class.path for the appropriate vmTestbase classes rather than 
using

incorrect settings on the command line.

Some clean up has been done to remove quarantine keyword
and @ignore directives. Should additional clean up be done to remove
bug numbers, etc.?

TEST.PROPERTIES were added so test 3 so it is consistent with the 
other tests

in the group.

  Issue: https://bugs.openjdk.java.net/browse/JDK-8218128
  Webrev: 
http://cr.openjdk.java.net/~gadams/8218128/webrev.00/index.html


Local testing has been successful on a linux-x64-debug build.
Testing on mach5 for other platforms next.

RFR: JDK-8013728: nsk/jdi/BScenarios/hotswap/tc10x001 Unrecognized Windows Sockets error: 0: recv failed

[Gary Adams]

I've been able to reproduce the socket teardown failure in tc10x001.
It appears that the debugee sends it's final output and then exits.
If the debugee exits while the debugger is receiving the final output,
it can result in a socket error.

This proposed fix forces the debugee to wait until the debugger
acknowledges it has processed the final output before exitting.

  Webrev: http://cr.openjdk.java.net/~gadams/8013728/webrev.01/

The original bug was filed before these tests were in the open repos.

None of the other tests in the nsk/jdi/BScenarios set have shown this 
problem,

because they do not have an early debuggee exit sequence.

Re: RFR: JDK-8218128: vmTestbase/nsk/jvmti/ResourceExhausted/resexhausted003 and 004 use wrong path to test classes

[Gary Adams]

I'll revert the comments that were left in during the tonga conversion.

I did come across an interesting test failure in resexhausted001
which had an
  @ignore 7013634

JDK-7013634: jvmti resexhausted001 can timeout or fail due to 
excessive thread creation


The test was closed because it was not reproducible.
Even though the test was not on the ProblemList, I believe
jtreg was excluding the test from running.

The original problem reported an "out of swap" condition.

The current failure reports:

--System.out:(3/217)--
Creating threads...
Timeout refired 480 times
[730.871s][warning][os,thread] Failed to start thread - _beginthreadex failed 
(EACCES) for attributes: stacksize: default, flags: CREATE_SUSPENDED 
STACK_SIZE_PARAM_IS.



On 3/7/19, 4:02 PM, Chris Plummer wrote:

Hi Gary,

Why did you remove the "nonconcurrent" keyword. I know these are just 
comments for reference that were added when the test was ported from 
tonga, but as a comment it is still applicable. The test should not be 
run concurrent with others (which you have also fixed with the 
addition of the "exclusiveAccess.dirs=.").


Otherwise changes look good.

thanks,

Chris

On 3/7/19 10:57 AM, Gary Adams wrote:

This proposed fix will restore the ResourceExhausted tests.

Test 3 and 4 were on the ProblemList because of the potential
path issues in finding the correct classes. This change searches the
test.class.path for the appropriate vmTestbase classes rather than using
incorrect settings on the command line.

Some clean up has been done to remove quarantine keyword
and @ignore directives. Should additional clean up be done to remove
bug numbers, etc.?

TEST.PROPERTIES were added so test 3 so it is consistent with the 
other tests

in the group.

  Issue: https://bugs.openjdk.java.net/browse/JDK-8218128
  Webrev: 
http://cr.openjdk.java.net/~gadams/8218128/webrev.00/index.html


Local testing has been successful on a linux-x64-debug build.
Testing on mach5 for other platforms next.

Re: RFR: JDK-8218128: vmTestbase/nsk/jvmti/ResourceExhausted/resexhausted003 and 004 use wrong path to test classes

[Gary Adams]

The tests have been excluded for a long time now.
It appears that the tests are working at this time.
I'll have more confidence in taking them off the ProblemList when
the testing has been done on more platforms.

Please let me know what you'd like the description to say to bring it up 
to date.


On 3/7/19, 4:06 PM, Leonid Mesnik wrote:

Hi
The problemlist

http://cr.openjdk.java.net/~gadams/8218128/webrev.00/test/hotspot/jtreg/ProblemList.txt.udiff.html

Tests are excluded because of
https://bugs.openjdk.java.net/browse/JDK-6606767
which is not fixed yet. Why do you want to remove them from
problemlist?

Also I think it is a good time to update test desrption in the

http://cr.openjdk.java.net/~gadams/8218128/webrev.00/test/hotspot/jtreg/vmTestbase/nsk/jvmti/ResourceExhausted/resexhausted003/TestDescription.java.udiff.html

Currently it says about PermGen OOME.

Leonid

On Thu, 2019-03-07 at 13:57 -0500, Gary Adams wrote:

This proposed fix will restore the ResourceExhausted tests.

Test 3 and 4 were on the ProblemList because of the potential
path issues in finding the correct classes. This change searches the
test.class.path for the appropriate vmTestbase classes rather than
using
incorrect settings on the command line.

Some clean up has been done to remove quarantine keyword
and @ignore directives. Should additional clean up be done to remove
bug numbers, etc.?

TEST.PROPERTIES were added so test 3 so it is consistent with the
other
tests
in the group.

Issue: https://bugs.openjdk.java.net/browse/JDK-8218128
Webrev:
http://cr.openjdk.java.net/~gadams/8218128/webrev.00/index.html

Local testing has been successful on a linux-x64-debug build.
Testing on mach5 for other platforms next.

RFR: JDK-8218128: vmTestbase/nsk/jvmti/ResourceExhausted/resexhausted003 and 004 use wrong path to test classes

[Gary Adams]

This proposed fix will restore the ResourceExhausted tests.

Test 3 and 4 were on the ProblemList because of the potential
path issues in finding the correct classes. This change searches the
test.class.path for the appropriate vmTestbase classes rather than using
incorrect settings on the command line.

Some clean up has been done to remove quarantine keyword
and @ignore directives. Should additional clean up be done to remove
bug numbers, etc.?

TEST.PROPERTIES were added so test 3 so it is consistent with the other 
tests

in the group.

  Issue: https://bugs.openjdk.java.net/browse/JDK-8218128
  Webrev: http://cr.openjdk.java.net/~gadams/8218128/webrev.00/index.html

Local testing has been successful on a linux-x64-debug build.
Testing on mach5 for other platforms next.

RFR: JDK-8218166: [Graal] com/sun/jdi/SimulResumerTest.java failure

[Gary Adams]

While trying to reproduce the timeout reported in
  JDK-8000669: com/sun/jdi/SimulResumerTest.java times out

I was unable to reproduce the timeout failure, but I did occasionally
see the ObjectCollectedException. The output from the test is very verbose
and may be the source of the occasional timeout. I'd like to close 
JDK-8000669

as cannot reproduce and if it shows up again look into limiting the amount
of non-essential output from the test.

This is a racy test to begin with and it already is ignoring exceptions
due to unexpected thread states. Adding the ignore for 
ObjectCollectedException

allows the test to complete without errors.

The graal label was recently removed. We should also remove it from the 
summary.


Proposed changeset:


diff --git a/test/jdk/com/sun/jdi/SimulResumerTest.java 
b/test/jdk/com/sun/jdi/SimulResumerTest.java

--- a/test/jdk/com/sun/jdi/SimulResumerTest.java
+++ b/test/jdk/com/sun/jdi/SimulResumerTest.java
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2008, 2015, Oracle and/or its affiliates. All rights 
reserved.
+ * Copyright (c) 2008, 2019, 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
@@ -211,6 +211,8 @@

 } catch (IncompatibleThreadStateException ee) {
 // ignore
+} catch (ObjectCollectedException ee) {
+// ignore
 } catch (VMDisconnectedException ee) {
 // This is how we stop.  The debuggee runs to completion
 // and we get this exception.

Re: RFR: 8218464: vmTestbase/nsk/jdi/VirtualMachine/allThreads/allthreads001/TestDescription.java failed

[Gary Adams]

Looks OK to me.

On 3/5/19, 5:47 PM, serguei.spit...@oracle.com wrote:

Hi Daniil,

The fix looks good.

Thanks,
Serguei


On 3/5/19 1:59 PM, Daniil Titov wrote:
Please review a fix for this test that intermittently fails with 
Graal on.


The problem here is that the test does two consequent calls to 
VirtualMachine.allThreads() and checks that the number of threads 
returned by these calls is the same. With Graal on there is a chance 
that a new JVMCI compiler thread could be started between these calls 
that makes test to fail. The fix temporary suspends the debuggee VM 
while these two calls to VirtualMachine.allThreads() are made.


Webrev: http://cr.openjdk.java.net/~dtitov/8218464/webrev.01
Bug: https://bugs.openjdk.java.net/browse/JDK-8218464

Thanks!
--Daniil

RFR: JDK-8201252: unquarantine nsk/jdi/ThreadReference/resume/resume001

[Gary Adams]

After 1000's of test runs with the resume001 test off the ProblemList,
no new sightings have been observed. I intend to use the JDK-8201252
issue to restore the test. JDK-8072701 will be updated and left for
sustaining, if the fix needs to be applied to older releases.


diff --git a/test/hotspot/jtreg/ProblemList.txt 
b/test/hotspot/jtreg/ProblemList.txt

--- a/test/hotspot/jtreg/ProblemList.txt
+++ b/test/hotspot/jtreg/ProblemList.txt
@@ -157,7 +157,6 @@
 
vmTestbase/nsk/monitoring/MemoryPoolMBean/isUsageThresholdExceeded/isexceeded005/TestDescription.java
 8153598 generic-all
 
vmTestbase/nsk/monitoring/ThreadMXBean/ThreadInfo/Deadlock/JavaDeadlock001/TestDescription.java
 8060733 generic-all

-vmTestbase/nsk/jdi/ThreadReference/resume/resume001/TestDescription.java 8072701 
generic-all
 vmTestbase/nsk/jdi/ThreadReference/stop/stop001/TestDescription.java 
7034630 generic-all
 vmTestbase/nsk/jdi/BScenarios/hotswap/tc10x001/TestDescription.java 
8013728 generic-all

 vmTestbase/nsk/jdi/EventRequest/setEnabled/setenabled003/TestDescription.java 
8066993 generic-all

Re: RFR(XS) 8220030: JdbStopThreadidTest.java failed due to "Unexpected IO error while writing command 'quit' to jdb stdin stream"

[Gary Adams]

This looks OK to me.

If a specific return code is expected,
then the test should wait until it gets the response it needs.

If a specific return is not required, the test should be lenient
about what information is required for the test to complete.

On 3/4/19, 10:41 PM, Chris Plummer wrote:

Hello,

Please review the following. Details are in the bug. In short, the way 
I was checking for the application exit was a bit non-standard and 
exposed what is probably a bug in the JdbTest shutdown code. I changed 
the test to conform to the way other test run to exit:


https://bugs.openjdk.java.net/browse/JDK-8220030

diff --git a/test/jdk/com/sun/jdi/JdbStopThreadidTest.java 
b/test/jdk/com/sun/jdi/JdbStopThreadidTest.java

--- a/test/jdk/com/sun/jdi/JdbStopThreadidTest.java
+++ b/test/jdk/com/sun/jdi/JdbStopThreadidTest.java
@@ -32,6 +32,7 @@
  * @run main/othervm JdbStopThreadidTest
  */

+import jdk.test.lib.process.OutputAnalyzer;
 import lib.jdb.Jdb;
 import lib.jdb.JdbCommand;
 import lib.jdb.JdbTest;
@@ -138,6 +139,7 @@
 jdb.command(JdbCommand.cont().waitForPrompt("Breakpoint hit: 
\"thread=MYTHREAD-2\", \\S+MyThread.brkMethod", true));
 // Continue until the application exits. Once again, hitting 
a breakpoint will cause

 // a failure because we are not suppose to hit one.
- jdb.command(JdbCommand.cont().waitForPrompt(Jdb.APPLICATION_EXIT, 
true));

+jdb.contToExit(1);
+new 
OutputAnalyzer(getJdbOutput()).shouldContain(Jdb.APPLICATION_EXIT);


thanks,

Chris

Re: RFR: JDK-4903717: nsk/jdi/ThreadReference/isSuspended/issuspended002 failing

[Gary Adams]

I will move the logging statement up.

Leaving the "docontinue" in the synchronized block
will guarantee the debugger suspends the debuggee
main thread without any chance of interference
with logging commands.

I'm sure this test was left with verbose logging enabled in the
hopes of catching more details about the failing sequence of
events. In this case it was the excessive logging that was causing
the test to hang.

Now that the test can run reliably, I think we should turn off
the verbose logging.

On 2/28/19, 4:44 PM, Alex Menkov wrote:

Hi Gary,

Now "mainThread is out of: synchronized (lockingObject) {" record is 
logged at the end of the scenario and that makes it useless.


I think it would be better to call
pipe.println("docontinue") after logging of this message:

  log1("mainThread is out of: synchronized (lockingObject) {");
+  pipe.println("docontinue");
+  // wait until the scenario is completed to avoid deadlocks on logging
+  synchronized (Threadissuspended002a.waitnotifyObj) {
+Threadissuspended002a.waitnotifyObj.wait();
+  }

--alex

On 02/27/2019 02:29, gary.ad...@oracle.com wrote:

Revised webrev : http://cr.openjdk.java.net/~gadams/4903717/webrev.02/

To prevent the main thread from holding the logging lock when
the debugger suspends the debuggee, an explicit wait/notify is used
so the test thread can safely issue log messages while the main thread
remains suspended.

Testing is in progress.

On 2/26/19 3:49 PM, gary.ad...@oracle.com wrote:

New fix will be coming tomorrow...

I dumped the main and thread2 stacks at the
point of the timeout and found they were
both contending for the java.io.PrintStream
writeln() synchronization block.

While writing log messages main thread held the
lock and was suspended and thread2 was blocked
trying to report it's progress. So thread2 never reaches the
breakpoint.

java.nio.Buffer:292 in thread instance of 
java.lang.Thread(name='main', id=1)
java.nio.CharBuffer:1260 in thread instance of 
java.lang.Thread(name='main', id=1)
java.nio.CharBuffer:265 in thread instance of 
java.lang.Thread(name='main', id=1)
java.nio.Buffer:223 in thread instance of 
java.lang.Thread(name='main', id=1)
java.nio.CharBuffer:284 in thread instance of 
java.lang.Thread(name='main', id=1)
java.nio.HeapCharBuffer:77 in thread instance of 
java.lang.Thread(name='main', id=1)
java.nio.CharBuffer:396 in thread instance of 
java.lang.Thread(name='main', id=1)
sun.nio.cs.StreamEncoder:280 in thread instance of 
java.lang.Thread(name='main', id=1)
sun.nio.cs.StreamEncoder:125 in thread instance of 
java.lang.Thread(name='main', id=1)
java.io.OutputStreamWriter:211 in thread instance of 
java.lang.Thread(name='main', id=1)
java.io.BufferedWriter:120 in thread instance of 
java.lang.Thread(name='main', id=1)
*java.io.PrintStream:653 in thread instance of 
java.lang.Thread(name='main', id=1)*
java.io.PrintStream:958 in thread instance of 
java.lang.Thread(name='main', id=1)
nsk.jdi.ThreadReference.isSuspended.issuspended002a:49 in thread 
instance of java.lang.Thread(name='main', id=1)
nsk.jdi.ThreadReference.isSuspended.issuspended002a:124 in thread 
instance of java.lang.Thread(name='main', id=1)



*java.io.PrintStream:650 in thread instance of 
nsk.jdi.ThreadReference.isSuspended.Threadissuspended002a(name='testedThread', 
id=589)*
java.io.PrintStream:958 in thread instance of 
nsk.jdi.ThreadReference.isSuspended.Threadissuspended002a(name='testedThread', 
id=589)
nsk.jdi.ThreadReference.isSuspended.issuspended002a:53 in thread 
instance of 
nsk.jdi.ThreadReference.isSuspended.Threadissuspended002a(name='testedThread', 
id=589)
nsk.jdi.ThreadReference.isSuspended.Threadissuspended002a:203 in 
thread instance of 
nsk.jdi.ThreadReference.isSuspended.Threadissuspended002a(name='testedThread', 
id=589)


On 2/22/19 11:21 PM, serguei.spit...@oracle.com wrote:

Hi Gary,

Thank you for cosmetic changes.

One thing to notice there is no guard against spurious wakeup for 
this:

   108 Threadissuspended002a.waitnotifyObj.wait();

But I doubt it causes these timeouts as they are very well 
reproducible.


This time I looked more to the debugger side.
The code is poorly structured, it is hard to understand what it is 
doing.


>   - resumed main thread after breakpoint enabled and thread2
 double resume

Could you, please, explain more how does this help and why timeouts 
are intermittent?


Thanks,
Serguei


On 2/22/19 11:52 AM, Gary Adams wrote:
After 1000 successful testruns, it seems like a reasonable 
solution is

to resume the main thread to resolve any locks holding back the
thread2 reaching the desired breakpoint.

  Updated webrev: 
http://cr.openjdk.java.net/~gadams/4903717/webrev.01/


Changes in this iteration:
  - cosmetic changes
 - spaces around operators
 - removed extra spaces around parens
 - removed extra blank lines
 - fixed typo in ERRROR
 - fixed curl braces indenting
  - res

Re: RFR: JDK-4903717: nsk/jdi/ThreadReference/isSuspended/issuspended002 failing

[Gary Adams]

MethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
at 
java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
at java.base/java.lang.reflect.Method.invoke(Method.java:567)
at 
com.sun.javatest.regtest.agent.MainWrapper$MainThread.run(MainWrapper.java:127)
at java.base/java.lang.Thread.run(Thread.java:835)
*-->  debugger:   resuming both second and main thread
debugee.stderr>  **>  mainThread: waiting for an instruction from the debugger 
...
-->  debugger:  the end of testing
debugee.stderr>  **>  thread2: entered into block:  synchronized (lockingObject)
debugee.stderr>  **>  thread2: exited from block:  synchronized (lockingObject)
debugee.stderr>  **>  thread2: call to the method 'runt1'
debugee.stderr>  **>  thread2: method 'runt1' enter*
-->  debugger:  : returned string is 'checkend'
debugee.stderr>  **>  mainThread: waiting for an instruction from the debugger 
...


On 2/20/19 5:15 PM, Alex Menkov wrote:

+1

But could you please remove old "pipe.println("docontinue")" 
statement instead of commenting it out (no new webrev is required)


--alex

On 02/20/2019 09:58, Chris Plummer wrote:

Looks good.

Chris

On 2/20/19 7:57 AM, Gary Adams wrote:
The issuspended002 has been on the ProblemList since the tests 
were moved to
the open repos. The proposed changeset applies the same fix that 
was used in issuspended001.


The current test fails when the debuggee replies with the 
"docontinue" response,
while it is still holding a lock in a syncrhonized block. When 
the debugger then
suspends the vm the debuggee test thread fails to proceed to the 
expected breakpoint.

Testing in progress.

  Issue: https://bugs.openjdk.java.net/browse/JDK-4903717
  Webrev: http://cr.openjdk.java.net/~gadams/4903717/webrev.00/

JDK-8149460: jmap give unrelated error message if non java process id is specified in the command.

[Gary Adams]

I'd like to close this bug as "cannot reproduce".

The diagnostic output that suggested to use the "-F"
option was removed when the tmtools were decoupled
from the serviceability agent. See JDK-8155091 (May 2016).

  Issue: https://bugs.openjdk.java.net/browse/JDK-8149460

RFR: JDK-4903717: nsk/jdi/ThreadReference/isSuspended/issuspended002 failing

[Gary Adams]

The issuspended002 has been on the ProblemList since the tests were 
moved to
the open repos. The proposed changeset applies the same fix that was 
used in issuspended001.


The current test fails when the debuggee replies with the "docontinue" 
response,
while it is still holding a lock in a syncrhonized block. When the 
debugger then
suspends the vm the debuggee test thread fails to proceed to the 
expected breakpoint.

Testing in progress.

  Issue: https://bugs.openjdk.java.net/browse/JDK-4903717
  Webrev: http://cr.openjdk.java.net/~gadams/4903717/webrev.00/

Re: RFR: JDK-8219388: Misleading log message "issuspended002a debuggee launched"

[Gary Adams]

Can I have a second reviewer or a confirmation that this is a trivial change
only needing one reviewer?

On 2/19/19, 6:15 PM, Chris Plummer wrote:

Looks good.

Chris

On 2/19/19 1:52 PM, gary.ad...@oracle.com wrote:

Sorry, my bad, used the wrong list of files when I made the webrev.
Added the location, interrupt and setvalue debuggee references.

  Webrev: http://cr.openjdk.java.net/~gadams/8219388/webrev.01/

On 2/19/19 3:01 PM, Chris Plummer wrote:

Hi Gary,

On 2/19/19 11:38 AM, Gary Adams wrote:

On my first pass I went after cleaning up the issuspend002a messages,
because I'm investigating pulling it off the ProblemList.

I added the setvalue003a misleading log messages in this updated 
webrev.

3 of the ones I pointed out below that were not setvalue003a.
I think the raw "debugee launched" messages are in context of other 
messages.

I'd prefer to leave that level of clean to a future effort.

Ok.


  Webrev: http://cr.openjdk.java.net/~gadams/8219388/webrev.01/


There's something wrong with your webrev. Look at the end of the 
index page. Those files showing no diffs are all duplicates, and 
also show up twice in the patch file. I also don't see the 
setvalue003a changes in it.


Chris



On 2/19/19, 1:59 PM, Chris Plummer wrote:

Hi Gary,

Changes look good, but there are other similar incorrect messages 
you might also want to address:


./StackFrame/thisObject/thisobject002.java:158: log2("setvalue003a 
debuggee launched");
./StackFrame/thisObject/thisobject001.java:159: log2("setvalue003a 
debuggee launched");
./StackFrame/visibleVariableByName/visiblevarbyname001.java:161: 
log2("setvalue003a debuggee launched");
./StackFrame/visibleVariableByName/visiblevarbyname002.java:152: 
log2("setvalue003a debuggee launched");
./StackFrame/thread/thread001.java:156: log2("location001a 
debuggee launched");
./StackFrame/visibleVariables/visiblevariables001.java:156: 
log2("setvalue003a debuggee launched");
./StackFrame/visibleVariables/visiblevariables002.java:153: 
log2("setvalue003a debuggee launched");
./ThreadReference/interrupt/interrupt001.java:156: 
log2("interrupt002a debuggee launched");
./LocalVariable/isVisible/isvisible001.java:162: 
log2("setvalue003a debuggee launched");
./ClassType/invokeMethod/invokemethod001.java:162: 
log2("location001a debuggee launched");
./Value/type/type002/type002.java:199: log2("setvalue003a debuggee 
launched");
./VoidValue/equals/equals001/equals001.java:207: 
log2("setvalue003a debuggee launched");
./VoidValue/hashCode/hashcode001/hashcode001.java:207: 
log2("setvalue003a debuggee launched");


And I noticed a very large number of tests that only have:

log2("debuggee launched");

But there are so many that if you are to fix them, it should be 
done as a separate CR.


Chris

On 2/19/19 10:19 AM, Gary Adams wrote:

A log message should have been parameterized
with the debuggeeName.

  Issue: https://bugs.openjdk.java.net/browse/JDK-8219388
  Webrev: http://cr.openjdk.java.net/~gadams/8219388/webrev.00/

Re: RFR: JDK-8219388: Misleading log message "issuspended002a debuggee launched"

[Gary Adams]

On my first pass I went after cleaning up the issuspend002a messages,
because I'm investigating pulling it off the ProblemList.

I added the setvalue003a misleading log messages in this updated webrev.
I think the raw "debugee launched" messages are in context of other 
messages.

I'd prefer to leave that level of clean to a future effort.

  Webrev: http://cr.openjdk.java.net/~gadams/8219388/webrev.01/

On 2/19/19, 1:59 PM, Chris Plummer wrote:

Hi Gary,

Changes look good, but there are other similar incorrect messages you 
might also want to address:


./StackFrame/thisObject/thisobject002.java:158: log2("setvalue003a 
debuggee launched");
./StackFrame/thisObject/thisobject001.java:159: log2("setvalue003a 
debuggee launched");
./StackFrame/visibleVariableByName/visiblevarbyname001.java:161: 
log2("setvalue003a debuggee launched");
./StackFrame/visibleVariableByName/visiblevarbyname002.java:152: 
log2("setvalue003a debuggee launched");
./StackFrame/thread/thread001.java:156:log2("location001a 
debuggee launched");
./StackFrame/visibleVariables/visiblevariables001.java:156: 
log2("setvalue003a debuggee launched");
./StackFrame/visibleVariables/visiblevariables002.java:153: 
log2("setvalue003a debuggee launched");
./ThreadReference/interrupt/interrupt001.java:156: log2("interrupt002a 
debuggee launched");
./LocalVariable/isVisible/isvisible001.java:162: log2("setvalue003a 
debuggee launched");
./ClassType/invokeMethod/invokemethod001.java:162: log2("location001a 
debuggee launched");
./Value/type/type002/type002.java:199:log2("setvalue003a 
debuggee launched");
./VoidValue/equals/equals001/equals001.java:207: log2("setvalue003a 
debuggee launched");
./VoidValue/hashCode/hashcode001/hashcode001.java:207: 
log2("setvalue003a debuggee launched");


And I noticed a very large number of tests that only have:

log2("debuggee launched");

But there are so many that if you are to fix them, it should be done 
as a separate CR.


Chris

On 2/19/19 10:19 AM, Gary Adams wrote:

A log message should have been parameterized
with the debuggeeName.

  Issue: https://bugs.openjdk.java.net/browse/JDK-8219388
  Webrev: http://cr.openjdk.java.net/~gadams/8219388/webrev.00/

RFR: JDK-8219388: Misleading log message "issuspended002a debuggee launched"

[Gary Adams]

A log message should have been parameterized
with the debuggeeName.

  Issue: https://bugs.openjdk.java.net/browse/JDK-8219388
  Webrev: http://cr.openjdk.java.net/~gadams/8219388/webrev.00/

Re: RFR: JDK-8149461: jmap kills process if non-java pid is specified in the command line

[Gary Adams]

It isn't pretty, but it's functional : "-f to force communication. ...

  Revised webrev: http://cr.openjdk.java.net/~gadams/8149461/webrev.02/

On 2/15/19, 11:57 AM, Daniel D. Daugherty wrote:

Yes. That's the direction I was thinking about.
Don't know about the '-F' versus '-F ', but
that a cmd line option parsing detail.

Dan


On 2/15/19 11:37 AM, Gary Adams wrote:

Here's a quick dirty "-F" that gets past
the "-XX:-UsePerfData" setting for jcmd.
Need to follow up on docs and usage
for the other commands.

Is this the direction you were thinking?

diff --git 
a/src/jdk.jcmd/share/classes/sun/tools/common/ProcessArgumentMatcher.javab/src/jdk.jcmd/share/classes/sun/tools/common/ProcessArgumentMatcher.java
--- 
a/src/jdk.jcmd/share/classes/sun/tools/common/ProcessArgumentMatcher.java
+++ 
b/src/jdk.jcmd/share/classes/sun/tools/common/ProcessArgumentMatcher.java

@@ -48,16 +48,27 @@
 public class ProcessArgumentMatcher {
 private String matchClass;
 private String singlePid;
+private static boolean bypassPid;
+private long pid;

 public ProcessArgumentMatcher(String pidArg) {
 if (pidArg == null || pidArg.isEmpty()) {
 throw new IllegalArgumentException("Pid string is invalid");
 }
 if (pidArg.charAt(0) == '-') {
+if (pidArg.charAt(1) == 'F') {
+// Allow -F to bypass the pid check
+pid = Long.parseLong(pidArg.substring(2));
+if (pid != 0) {
+singlePid = String.valueOf(pid);
+}
+bypassPid = true;
+} else {
 throw new IllegalArgumentException("Unrecognized " + 
pidArg);

 }
+}
 try {
-long pid = Long.parseLong(pidArg);
+pid = Long.parseLong(pidArg);
 if (pid != 0) {
 singlePid = String.valueOf(pid);
 }
@@ -170,4 +181,21 @@
 public Collection getVirtualMachinePids() {
 return this.getVirtualMachinePids(null);
 }
+
+  // Check that pid matches a known running Java process
+  public static boolean checkJavaPid(String pid) {
+  // Skip the perf data pid visibility check if "-F" 
requested.

+  if (bypassPid) {
+  return true;
 }
+  List l = VirtualMachine.list();
+  boolean found = false;
+  for (VirtualMachineDescriptor vmd: l) {
+  if (vmd.id().equals(pid)) {
+  found = true;
+  break;
+  }
+  }
+  return found;
+  }
+}
On 2/15/19, 10:24 AM, Daniel D. Daugherty wrote:

On 2/15/19 8:44 AM, Gary Adams wrote:

Confirmed
  -XX:-UsePerfData

prevents visibility to jps, but allows direct access via pid.

The new check would block access before the attach is attempted.

May be best to close this bug as "will not fix".
Requires a valid Java process pid.


Or make it a best effort solution. The idea that a jmap on a non-Java
PID could kill that PID violates the "do no harm" principle for an
observer tool.

Of what I have seen on the thread so far, I like these:

- make the PID check on the specified PID only (should be pretty fast)
- add a force option that tries to attach to the PID regardless
  of what the sanity check says; that will solve the -XX:-UsePerfData
  problem.

Writing/updating tests for this is going to be "interesting".

Dan





On 2/15/19, 8:29 AM, Bernd Eckenfels wrote:
I wonder, instead of listing all VMs, would it be better to check 
only the target PID?


BTW speaking of hs_perf files: is it supposed to work without 
-XX:-UserPerfData also?


Gruss
Bernd

Gruss
Bernd
--
http://bernd.eckenfels.net
----
*Von:* Gary Adams 
*Gesendet:* Freitag, Februar 15, 2019 2:19 PM
*An:* gary.ad...@oracle.com
*Cc:* Bernd Eckenfels; OpenJDK Serviceability
*Betreff:* Re: RFR: JDK-8149461: jmap kills process if non-java 
pid is specified in the command line

On a linux system with 1 Java process and 500 non-Java processes,
/tmp is not tmpfs mounted, 20 runs average 255.5 ms stddev 9.32.

JDK-8176828 is the issue that enabled perfmemory visibility once 
the vm is in live mode.


For containers that are configured without a shared view of /tmp, 
it may be necessary

to include a override of the pid check.

On 2/15/19, 7:29 AM, Gary Adams wrote:

I'll get some measurements on some local systems so we have a
specific idea about the overhead of the pid check.
I imagine in most production environments the /tmp tmpfs
is memory only set of checks.

One of the "not all vms are recognized" was fixed recently.
When starting a libjdwp session with server=y and suspend=y,
the vm was not recognized until a debugger was attached.

I'm not opposed to adding a force option to skip the valid pid
check. It might be better effort fixing the h

Re: RFR: JDK-8149461: jmap kills process if non-java pid is specified in the command line

[Gary Adams]

Here's a quick dirty "-F" that gets past
the "-XX:-UsePerfData" setting for jcmd.
Need to follow up on docs and usage
for the other commands.

Is this the direction you were thinking?

diff --git 
a/src/jdk.jcmd/share/classes/sun/tools/common/ProcessArgumentMatcher.java b/src/jdk.jcmd/share/classes/sun/tools/common/ProcessArgumentMatcher.java
--- 
a/src/jdk.jcmd/share/classes/sun/tools/common/ProcessArgumentMatcher.java
+++ 
b/src/jdk.jcmd/share/classes/sun/tools/common/ProcessArgumentMatcher.java

@@ -48,16 +48,27 @@
 public class ProcessArgumentMatcher {
 private String matchClass;
 private String singlePid;
+private static boolean bypassPid;
+private long pid;

 public ProcessArgumentMatcher(String pidArg) {
 if (pidArg == null || pidArg.isEmpty()) {
 throw new IllegalArgumentException("Pid string is invalid");
 }
 if (pidArg.charAt(0) == '-') {
+if (pidArg.charAt(1) == 'F') {
+// Allow -F to bypass the pid check
+pid = Long.parseLong(pidArg.substring(2));
+if (pid != 0) {
+singlePid = String.valueOf(pid);
+}
+bypassPid = true;
+} else {
 throw new IllegalArgumentException("Unrecognized " + pidArg);
 }
+}
 try {
-long pid = Long.parseLong(pidArg);
+pid = Long.parseLong(pidArg);
 if (pid != 0) {
 singlePid = String.valueOf(pid);
 }
@@ -170,4 +181,21 @@
 public Collection getVirtualMachinePids() {
 return this.getVirtualMachinePids(null);
 }
+
+  // Check that pid matches a known running Java process
+  public static boolean checkJavaPid(String pid) {
+  // Skip the perf data pid visibility check if "-F" 
requested.

+  if (bypassPid) {
+  return true;
 }
+  List l = VirtualMachine.list();
+  boolean found = false;
+  for (VirtualMachineDescriptor vmd: l) {
+  if (vmd.id().equals(pid)) {
+  found = true;
+  break;
+  }
+  }
+  return found;
+  }
+}
On 2/15/19, 10:24 AM, Daniel D. Daugherty wrote:

On 2/15/19 8:44 AM, Gary Adams wrote:

Confirmed
  -XX:-UsePerfData

prevents visibility to jps, but allows direct access via pid.

The new check would block access before the attach is attempted.

May be best to close this bug as "will not fix".
Requires a valid Java process pid.


Or make it a best effort solution. The idea that a jmap on a non-Java
PID could kill that PID violates the "do no harm" principle for an
observer tool.

Of what I have seen on the thread so far, I like these:

- make the PID check on the specified PID only (should be pretty fast)
- add a force option that tries to attach to the PID regardless
  of what the sanity check says; that will solve the -XX:-UsePerfData
  problem.

Writing/updating tests for this is going to be "interesting".

Dan





On 2/15/19, 8:29 AM, Bernd Eckenfels wrote:
I wonder, instead of listing all VMs, would it be better to check 
only the target PID?


BTW speaking of hs_perf files: is it supposed to work without 
-XX:-UserPerfData also?


Gruss
Bernd

Gruss
Bernd
--
http://bernd.eckenfels.net
----
*Von:* Gary Adams 
*Gesendet:* Freitag, Februar 15, 2019 2:19 PM
*An:* gary.ad...@oracle.com
*Cc:* Bernd Eckenfels; OpenJDK Serviceability
*Betreff:* Re: RFR: JDK-8149461: jmap kills process if non-java pid 
is specified in the command line

On a linux system with 1 Java process and 500 non-Java processes,
/tmp is not tmpfs mounted, 20 runs average 255.5 ms stddev 9.32.

JDK-8176828 is the issue that enabled perfmemory visibility once the 
vm is in live mode.


For containers that are configured without a shared view of /tmp, it 
may be necessary

to include a override of the pid check.

On 2/15/19, 7:29 AM, Gary Adams wrote:

I'll get some measurements on some local systems so we have a
specific idea about the overhead of the pid check.
I imagine in most production environments the /tmp tmpfs
is memory only set of checks.

One of the "not all vms are recognized" was fixed recently.
When starting a libjdwp session with server=y and suspend=y,
the vm was not recognized until a debugger was attached.

I'm not opposed to adding a force option to skip the valid pid
check. It might be better effort fixing the hung vm case if we
have a concrete failure to investigate.

On 2/15/19, 6:47 AM, Bernd Eckenfels wrote:

Hello,

I see possible issues here, not sure if they still exist but I 
wanted to mention them:


the list-vm function might be slow on a loaded system (as it is a 
complex function). It’s not the best Situation if your diagnostic 
attempts are slow down in such a situation.

Re: RFR: JDK-8149461: jmap kills process if non-java pid is specified in the command line

[Gary Adams]

Confirmed
  -XX:-UsePerfData

prevents visibility to jps, but allows direct access via pid.

The new check would block access before the attach is attempted.

May be best to close this bug as "will not fix".
Requires a valid Java process pid.

On 2/15/19, 8:29 AM, Bernd Eckenfels wrote:
I wonder, instead of listing all VMs, would it be better to check only 
the target PID?


BTW speaking of hs_perf files: is it supposed to work without 
-XX:-UserPerfData also?


Gruss
Bernd

Gruss
Bernd
--
http://bernd.eckenfels.net
----
*Von:* Gary Adams 
*Gesendet:* Freitag, Februar 15, 2019 2:19 PM
*An:* gary.ad...@oracle.com
*Cc:* Bernd Eckenfels; OpenJDK Serviceability
*Betreff:* Re: RFR: JDK-8149461: jmap kills process if non-java pid is 
specified in the command line

On a linux system with 1 Java process and 500 non-Java processes,
/tmp is not tmpfs mounted, 20 runs average 255.5 ms stddev 9.32.

JDK-8176828 is the issue that enabled perfmemory visibility once the 
vm is in live mode.


For containers that are configured without a shared view of /tmp, it 
may be necessary

to include a override of the pid check.

On 2/15/19, 7:29 AM, Gary Adams wrote:

I'll get some measurements on some local systems so we have a
specific idea about the overhead of the pid check.
I imagine in most production environments the /tmp tmpfs
is memory only set of checks.

One of the "not all vms are recognized" was fixed recently.
When starting a libjdwp session with server=y and suspend=y,
the vm was not recognized until a debugger was attached.

I'm not opposed to adding a force option to skip the valid pid
check. It might be better effort fixing the hung vm case if we
have a concrete failure to investigate.

On 2/15/19, 6:47 AM, Bernd Eckenfels wrote:

Hello,

I see possible issues here, not sure if they still exist but I 
wanted to mention them:


the list-vm function might be slow on a loaded system (as it is a 
complex function). It’s not the best Situation if your diagnostic 
attempts are slow down in such a situation.


Also in the past not all VMs might be recognized by the list 
function, a more targeted attach could still succeed. Is that 
addressed since the container-PID changes? In both cases a force 
option would help.


Gruss
Bernd

Gruss
Bernd
--
http://bernd.eckenfels.net

*Von:* serviceability-dev 
 im Auftrag von 
gary.ad...@oracle.com

*Gesendet:* Freitag, Februar 15, 2019 12:07 PM
*An:* Thomas Stüfe; David Holmes; Chris Plummer
*Cc:* OpenJDK Serviceability
*Betreff:* Re: RFR: JDK-8149461: jmap kills process if non-java pid 
is specified in the command line

Let me clarify a few things about the proposed fix.

The VirtualMachine.list() mechanism is based on
Java processes that are up and running.
During VM startup when agent libraries are loaded,
the fact is recorded in the filesystem that a Java process
is eligible for an attach request.

This is a much smaller list than scanning for all the
running processes and works across all supported
platforms. It also doesn't rely on fragile command line
parsing to determine a Java launcher is used.

I believe most of the reported hang situations
are not for the first level information for pid and
command line requests. I believe the hangs are due
to the second level requests that actually attach
to the process and issue a command to the running
Java process.

The overhead for the pid check should be relatively small.
In a standalone command for a one time check at startup
with 10's of Java processes. I know the documentation
states the user is responsible for verifying with jps
that a Java process pid or vmid is provided. But the cost
of human error can be a terminated process.

Selection by name also has some drawbacks when multiple
copies of a command are running at the same time.

Running "jcmd 0 ..." is the documented way to run a command on
all running Java processes.

May I count you as a reviewer on the current changeset?

On 2/15/19 3:54 AM, Thomas Stüfe wrote:



On Fri, Feb 15, 2019 at 3:26 AM David Holmes 
mailto:david.hol...@oracle.com>> wrote:


Gary,

What is the overhead of doing the validation? How do we list
VMs? Do we
need to examine every running process to get the list of VMs?
Wouldn't
it be better to check the given process is a VM rather than
checking all
potential VM processes?


I think this is a valid point. Listing VMs is normally quick (just 
use jcmd without any parms) but I have seen this fail or hang 
rarely in situations where I then still was able to talk to my VM 
via pid. I never investigated this but I would not like to be 
unable to connect to my VM just because some rogue VM mailfunctions.


This would be an argument for the alternative I offered in my first 
answer - just use the proc file system or a similar mechanism to 
ch

Re: RFR: JDK-8149461: jmap kills process if non-java pid is specified in the command line

[Gary Adams]

On 2/15/19, 8:18 AM, David Holmes wrote:

On 15/02/2019 8:04 pm, gary.ad...@oracle.com wrote:

Let me clarify a few things about the proposed fix.

The VirtualMachine.list() mechanism is based on
Java processes that are up and running.
During VM startup when agent libraries are loaded,
the fact is recorded in the filesystem that a Java process
is eligible for an attach request.


I don't follow. The target VM is not started with an agent, so how is 
it recorded in the file system?

The VirtualMachine list is based on the /tmp/hsperfdata_/.
Perf data initialization is handled when the VM enters live mode.

A bug addressed last year fixed an issue with agent initialization and VM
visibility.



David
-


This is a much smaller list than scanning for all the
running processes and works across all supported
platforms. It also doesn't rely on fragile command line
parsing to determine a Java launcher is used.

I believe most of the reported hang situations
are not for the first level information for pid and
command line requests. I believe the hangs are due
to the second level requests that actually attach
to the process and issue a command to the running
Java process.

The overhead for the pid check should be relatively small.
In a standalone command for a one time check at startup
with 10's of Java processes. I know the documentation
states the user is responsible for verifying with jps
that a Java process pid or vmid is provided. But the cost
of human error can be a terminated process.

Selection by name also has some drawbacks when multiple
copies of a command are running at the same time.

Running "jcmd 0 ..." is the documented way to run a command on
all running Java processes.

May I count you as a reviewer on the current changeset?

On 2/15/19 3:54 AM, Thomas Stüfe wrote:



On Fri, Feb 15, 2019 at 3:26 AM David Holmes 
mailto:david.hol...@oracle.com>> wrote:


Gary,

What is the overhead of doing the validation? How do we list VMs?
Do we
need to examine every running process to get the list of VMs?
Wouldn't
it be better to check the given process is a VM rather than
checking all
potential VM processes?


I think this is a valid point. Listing VMs is normally quick (just 
use jcmd without any parms) but I have seen this fail or hang rarely 
in situations where I then still was able to talk to my VM via pid. 
I never investigated this but I would not like to be unable to 
connect to my VM just because some rogue VM mailfunctions.


This would be an argument for the alternative I offered in my first 
answer - just use the proc file system or a similar mechanism to 
check only the pid you plan on sending sigquit to...


I think there is an onus of responsibility on the user to provide a
correct pid here, rather than trying to make this foolproof.


Oh but it can happen quite easily. For example, by pulling up an 
older jcmd from your shell history and forgetting to modify the pid. 
Thank god for the command name argument option on jcmd, which I now 
use mostly.


We also had a customer which tried to find all VMs on his machine by 
attempting to attach with jcmd to every process, killing them left 
and right :)


... Thomas

Thanks,
David

On 15/02/2019 5:12 am, Gary Adams wrote:
> The following commands present a similar kill process behavior:
> jcmd
> jinfo
> jmap
> jstack
>
> The following commands do not attach.
> jstat sun.jvmstat.monitor.MonitorException "not found"
> jps no pid arguments
>
> This update moves the checkJavaPid method into the
> common/ProcessArgumentsMatcher.java
> and applies the check before the pid is used for an attach
operation.
>
>Revised webrev:
http://cr.openjdk.java.net/~gadams/8149461/webrev.01/
>
> On 2/14/19, 12:07 PM, Chris Plummer wrote:
>> Hi Gary,
>>
>> What about the other tools that attach to a user specified
    process. Do
>> they also have this issue?
>>
>> thanks,
>>
>> Chris
>>
>> On 2/14/19 8:35 AM, Gary Adams wrote:
>>> There is an old reported problem that using jmap on a process
that is
>>> not running
>>> Java could cause the process to terminate. This is due to the
SIGQUIT
>>> used
>>> when attaching to the process.
>>>
>>> It is a fairly simple operation to validate that the pid
matches one
>>> of the known
>>> running Java processes using VirtualMachine.list().
>>>
>>>   Issue: https://bugs.openjdk.java.net/browse/JDK-8149461
>>>
>>> Proposed fix:
>>>
>>> diff --git a/src/jdk.jcmd/share/classes/sun/tools/jmap/JMap.java
>>> b/src/jdk.jcmd/share/classes/sun/tools/jmap/JMap.java
>>> --- a/src/jdk.jcmd/share/classes/sun/tools/jmap/JMap.java
>>> +++ b/src/jdk

Re: RFR: JDK-8149461: jmap kills process if non-java pid is specified in the command line

[Gary Adams]

On a linux system with 1 Java process and 500 non-Java processes,
/tmp is not tmpfs mounted, 20 runs average 255.5 ms stddev 9.32.

JDK-8176828 is the issue that enabled perfmemory visibility once the vm 
is in live mode.


For containers that are configured without a shared view of /tmp, it may 
be necessary

to include a override of the pid check.

On 2/15/19, 7:29 AM, Gary Adams wrote:

I'll get some measurements on some local systems so we have a
specific idea about the overhead of the pid check.
I imagine in most production environments the /tmp tmpfs
is memory only set of checks.

One of the "not all vms are recognized" was fixed recently.
When starting a libjdwp session with server=y and suspend=y,
the vm was not recognized until a debugger was attached.

I'm not opposed to adding a force option to skip the valid pid
check. It might be better effort fixing the hung vm case if we
have a concrete failure to investigate.

On 2/15/19, 6:47 AM, Bernd Eckenfels wrote:

Hello,

I see possible issues here, not sure if they still exist but I wanted 
to mention them:


the list-vm function might be slow on a loaded system (as it is a 
complex function). It’s not the best Situation if your diagnostic 
attempts are slow down in such a situation.


Also in the past not all VMs might be recognized by the list 
function, a more targeted attach could still succeed. Is that 
addressed since the container-PID changes? In both cases a force 
option would help.


Gruss
Bernd

Gruss
Bernd
--
http://bernd.eckenfels.net

*Von:* serviceability-dev 
 im Auftrag von 
gary.ad...@oracle.com

*Gesendet:* Freitag, Februar 15, 2019 12:07 PM
*An:* Thomas Stüfe; David Holmes; Chris Plummer
*Cc:* OpenJDK Serviceability
*Betreff:* Re: RFR: JDK-8149461: jmap kills process if non-java pid 
is specified in the command line

Let me clarify a few things about the proposed fix.

The VirtualMachine.list() mechanism is based on
Java processes that are up and running.
During VM startup when agent libraries are loaded,
the fact is recorded in the filesystem that a Java process
is eligible for an attach request.

This is a much smaller list than scanning for all the
running processes and works across all supported
platforms. It also doesn't rely on fragile command line
parsing to determine a Java launcher is used.

I believe most of the reported hang situations
are not for the first level information for pid and
command line requests. I believe the hangs are due
to the second level requests that actually attach
to the process and issue a command to the running
Java process.

The overhead for the pid check should be relatively small.
In a standalone command for a one time check at startup
with 10's of Java processes. I know the documentation
states the user is responsible for verifying with jps
that a Java process pid or vmid is provided. But the cost
of human error can be a terminated process.

Selection by name also has some drawbacks when multiple
copies of a command are running at the same time.

Running "jcmd 0 ..." is the documented way to run a command on
all running Java processes.

May I count you as a reviewer on the current changeset?

On 2/15/19 3:54 AM, Thomas Stüfe wrote:



On Fri, Feb 15, 2019 at 3:26 AM David Holmes 
mailto:david.hol...@oracle.com>> wrote:


Gary,

What is the overhead of doing the validation? How do we list
VMs? Do we
need to examine every running process to get the list of VMs?
Wouldn't
it be better to check the given process is a VM rather than
checking all
potential VM processes?


I think this is a valid point. Listing VMs is normally quick (just 
use jcmd without any parms) but I have seen this fail or hang rarely 
in situations where I then still was able to talk to my VM via pid. 
I never investigated this but I would not like to be unable to 
connect to my VM just because some rogue VM mailfunctions.


This would be an argument for the alternative I offered in my first 
answer - just use the proc file system or a similar mechanism to 
check only the pid you plan on sending sigquit to...


I think there is an onus of responsibility on the user to provide a
correct pid here, rather than trying to make this foolproof.


Oh but it can happen quite easily. For example, by pulling up an 
older jcmd from your shell history and forgetting to modify the pid. 
Thank god for the command name argument option on jcmd, which I now 
use mostly.


We also had a customer which tried to find all VMs on his machine by 
attempting to attach with jcmd to every process, killing them left 
and right :)


... Thomas

Thanks,
David

    On 15/02/2019 5:12 am, Gary Adams wrote:
> The following commands present a similar kill process behavior:
> jcmd
> jinfo
> jmap
> jstack
>
> The following commands do not attach.
&

Re: RFR: JDK-8149461: jmap kills process if non-java pid is specified in the command line

[Gary Adams]

I'll get some measurements on some local systems so we have a
specific idea about the overhead of the pid check.
I imagine in most production environments the /tmp tmpfs
is memory only set of checks.

One of the "not all vms are recognized" was fixed recently.
When starting a libjdwp session with server=y and suspend=y,
the vm was not recognized until a debugger was attached.

I'm not opposed to adding a force option to skip the valid pid
check. It might be better effort fixing the hung vm case if we
have a concrete failure to investigate.

On 2/15/19, 6:47 AM, Bernd Eckenfels wrote:

Hello,

I see possible issues here, not sure if they still exist but I wanted 
to mention them:


the list-vm function might be slow on a loaded system (as it is a 
complex function). It’s not the best Situation if your diagnostic 
attempts are slow down in such a situation.


Also in the past not all VMs might be recognized by the list function, 
a more targeted attach could still succeed. Is that addressed since 
the container-PID changes? In both cases a force option would help.


Gruss
Bernd

Gruss
Bernd
--
http://bernd.eckenfels.net

*Von:* serviceability-dev 
 im Auftrag von 
gary.ad...@oracle.com

*Gesendet:* Freitag, Februar 15, 2019 12:07 PM
*An:* Thomas Stüfe; David Holmes; Chris Plummer
*Cc:* OpenJDK Serviceability
*Betreff:* Re: RFR: JDK-8149461: jmap kills process if non-java pid is 
specified in the command line

Let me clarify a few things about the proposed fix.

The VirtualMachine.list() mechanism is based on
Java processes that are up and running.
During VM startup when agent libraries are loaded,
the fact is recorded in the filesystem that a Java process
is eligible for an attach request.

This is a much smaller list than scanning for all the
running processes and works across all supported
platforms. It also doesn't rely on fragile command line
parsing to determine a Java launcher is used.

I believe most of the reported hang situations
are not for the first level information for pid and
command line requests. I believe the hangs are due
to the second level requests that actually attach
to the process and issue a command to the running
Java process.

The overhead for the pid check should be relatively small.
In a standalone command for a one time check at startup
with 10's of Java processes. I know the documentation
states the user is responsible for verifying with jps
that a Java process pid or vmid is provided. But the cost
of human error can be a terminated process.

Selection by name also has some drawbacks when multiple
copies of a command are running at the same time.

Running "jcmd 0 ..." is the documented way to run a command on
all running Java processes.

May I count you as a reviewer on the current changeset?

On 2/15/19 3:54 AM, Thomas Stüfe wrote:



On Fri, Feb 15, 2019 at 3:26 AM David Holmes <mailto:david.hol...@oracle.com>> wrote:


Gary,

What is the overhead of doing the validation? How do we list VMs?
Do we
need to examine every running process to get the list of VMs?
Wouldn't
it be better to check the given process is a VM rather than
checking all
potential VM processes?


I think this is a valid point. Listing VMs is normally quick (just 
use jcmd without any parms) but I have seen this fail or hang rarely 
in situations where I then still was able to talk to my VM via pid. I 
never investigated this but I would not like to be unable to connect 
to my VM just because some rogue VM mailfunctions.


This would be an argument for the alternative I offered in my first 
answer - just use the proc file system or a similar mechanism to 
check only the pid you plan on sending sigquit to...


I think there is an onus of responsibility on the user to provide a
correct pid here, rather than trying to make this foolproof.


Oh but it can happen quite easily. For example, by pulling up an 
older jcmd from your shell history and forgetting to modify the pid. 
Thank god for the command name argument option on jcmd, which I now 
use mostly.


We also had a customer which tried to find all VMs on his machine by 
attempting to attach with jcmd to every process, killing them left 
and right :)


... Thomas

Thanks,
David

    On 15/02/2019 5:12 am, Gary Adams wrote:
> The following commands present a similar kill process behavior:
> jcmd
> jinfo
> jmap
> jstack
>
> The following commands do not attach.
> jstat sun.jvmstat.monitor.MonitorException "not found"
> jps no pid arguments
>
> This update moves the checkJavaPid method into the
> common/ProcessArgumentsMatcher.java
> and applies the check before the pid is used for an attach
operation.
>
>Revised webrev:
http://cr.openjdk.java.net/~g

Re: RFR: JDK-8149461: jmap kills process if non-java pid is specified in the command line

[Gary Adams]

The following commands present a similar kill process behavior:
   jcmd
   jinfo
   jmap
   jstack

The following commands do not attach.
   jstat sun.jvmstat.monitor.MonitorException "not found"
   jps no pid arguments

This update moves the checkJavaPid method into the 
common/ProcessArgumentsMatcher.java

and applies the check before the pid is used for an attach operation.

  Revised webrev: http://cr.openjdk.java.net/~gadams/8149461/webrev.01/

On 2/14/19, 12:07 PM, Chris Plummer wrote:

Hi Gary,

What about the other tools that attach to a user specified process. Do 
they also have this issue?


thanks,

Chris

On 2/14/19 8:35 AM, Gary Adams wrote:
There is an old reported problem that using jmap on a process that is 
not running
Java could cause the process to terminate. This is due to the SIGQUIT 
used

when attaching to the process.

It is a fairly simple operation to validate that the pid matches one 
of the known

running Java processes using VirtualMachine.list().

  Issue: https://bugs.openjdk.java.net/browse/JDK-8149461

Proposed fix:

diff --git a/src/jdk.jcmd/share/classes/sun/tools/jmap/JMap.java 
b/src/jdk.jcmd/share/classes/sun/tools/jmap/JMap.java

--- a/src/jdk.jcmd/share/classes/sun/tools/jmap/JMap.java
+++ b/src/jdk.jcmd/share/classes/sun/tools/jmap/JMap.java
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2005, 2018, Oracle and/or its affiliates. All 
rights reserved.
+ * Copyright (c) 2005, 2019, 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
@@ -30,6 +30,7 @@
 import java.io.InputStream;
 import java.io.UnsupportedEncodingException;
 import java.util.Collection;
+import java.util.List;

 import com.sun.tools.attach.VirtualMachine;
 import com.sun.tools.attach.VirtualMachineDescriptor;
@@ -125,6 +126,10 @@
 private static void executeCommandForPid(String pid, String 
command, Object ... args)

 throws AttachNotSupportedException, IOException,
UnsupportedEncodingException {
+// Before attaching, confirm that pid is a known Java process
+if (!checkJavaPid(pid)) {
+throw new AttachNotSupportedException();
+}
 VirtualMachine vm = VirtualMachine.attach(pid);

 // Cast to HotSpotVirtualMachine as this is an
@@ -196,6 +201,19 @@
 executeCommandForPid(pid, "dumpheap", filename, liveopt);
 }

+// Check that pid matches a known running Java process
+static boolean checkJavaPid(String pid) {
+List l = VirtualMachine.list();
+boolean found = false;
+for (VirtualMachineDescriptor vmd: l) {
+if (vmd.id().equals(pid)) {
+found = true;
+break;
+}
+}
+return found;
+}
+
 private static void checkForUnsupportedOptions(String[] args) {
 // Check arguments for -F, -m, and non-numeric value
 // and warn the user that SA is not supported anymore

RFR: JDK-8219002: Some comments and error messages refer to VMDisconnectException

[Gary Adams]

Trivial cleanup of a misspelled exception :
  VMDisconnectException
  VMDisconnectedException

  Webrev: http://cr.openjdk.java.net/~gadams/8219002/webrev.00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8219002

RFR: JDK-8149461: jmap kills process if non-java pid is specified in the command line

[Gary Adams]

There is an old reported problem that using jmap on a process that is 
not running

Java could cause the process to terminate. This is due to the SIGQUIT used
when attaching to the process.

It is a fairly simple operation to validate that the pid matches one of 
the known

running Java processes using VirtualMachine.list().

  Issue: https://bugs.openjdk.java.net/browse/JDK-8149461

Proposed fix:

diff --git a/src/jdk.jcmd/share/classes/sun/tools/jmap/JMap.java 
b/src/jdk.jcmd/share/classes/sun/tools/jmap/JMap.java

--- a/src/jdk.jcmd/share/classes/sun/tools/jmap/JMap.java
+++ b/src/jdk.jcmd/share/classes/sun/tools/jmap/JMap.java
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2005, 2018, Oracle and/or its affiliates. All rights 
reserved.
+ * Copyright (c) 2005, 2019, 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
@@ -30,6 +30,7 @@
 import java.io.InputStream;
 import java.io.UnsupportedEncodingException;
 import java.util.Collection;
+import java.util.List;

 import com.sun.tools.attach.VirtualMachine;
 import com.sun.tools.attach.VirtualMachineDescriptor;
@@ -125,6 +126,10 @@
 private static void executeCommandForPid(String pid, String 
command, Object ... args)

 throws AttachNotSupportedException, IOException,
UnsupportedEncodingException {
+// Before attaching, confirm that pid is a known Java process
+if (!checkJavaPid(pid)) {
+throw new AttachNotSupportedException();
+}
 VirtualMachine vm = VirtualMachine.attach(pid);

 // Cast to HotSpotVirtualMachine as this is an
@@ -196,6 +201,19 @@
 executeCommandForPid(pid, "dumpheap", filename, liveopt);
 }

+// Check that pid matches a known running Java process
+static boolean checkJavaPid(String pid) {
+List l = VirtualMachine.list();
+boolean found = false;
+for (VirtualMachineDescriptor vmd: l) {
+if (vmd.id().equals(pid)) {
+found = true;
+break;
+}
+}
+return found;
+}
+
 private static void checkForUnsupportedOptions(String[] args) {
 // Check arguments for -F, -m, and non-numeric value
 // and warn the user that SA is not supported anymore

RFR: JDK-8066993: nsk.jdi.EventRequest.setEnabled.setenabled003 fails: event IS NOT a breakpoint

[Gary Adams]

The setenabled003 test has been on the ProblemList since it was first
moved to the open repos. After 1000 test runs on {solaris, macosx, 
windows, linux}

for the test/hotspot/jtreg/vmTestbase/nsk/jdi/EventRequest/setEnabled tests,
the problem does not currently reproduce.

My recommendation is that we remove it from the ProblemList at this time.


diff --git a/test/hotspot/jtreg/ProblemList.txt 
b/test/hotspot/jtreg/ProblemList.txt

--- a/test/hotspot/jtreg/ProblemList.txt
+++ b/test/hotspot/jtreg/ProblemList.txt
@@ -161,10 +161,8 @@
 vmTestbase/nsk/jdi/ThreadReference/resume/resume001/TestDescription.java 
8072701 generic-all
 vmTestbase/nsk/jdi/ThreadReference/stop/stop001/TestDescription.java 
7034630 generic-all
 vmTestbase/nsk/jdi/BScenarios/hotswap/tc10x001/TestDescription.java 
8013728 generic-all
-vmTestbase/nsk/jdi/EventRequest/setEnabled/setenabled003/TestDescription.java 
8066993 generic-all

 
vmTestbase/nsk/jdi/VirtualMachine/redefineClasses/redefineclasses021/TestDescription.java
 8065773 generic-all
 
vmTestbase/nsk/jdi/VirtualMachine/redefineClasses/redefineclasses023/TestDescription.java
 8065773 generic-all

Re: RFR: JDK-8218754: JDK-8068225 regression in JDIBreakpointTest

[Gary Adams]

On 2/12/19, 7:14 AM, David Holmes wrote:

On 12/02/2019 10:11 pm, Gary Adams wrote:

Yes, see the revised webrev, we do have to guard against
multiple calls to dispose, e.g. catch and ignore VMDisconnectException.
We don't need to guard against a null vm. That would only exist if
the vm was never initialized and we were shutting down.


Okay. Revised webrev is better in that regard.

One more round to add the import for VMDisconnectedException.

  Webrev: http://cr.openjdk.java.net/~gadams/8218754/webrev.01/



There is a race condition between the debuggee and the debugger process.
In the original endDebugee, it attempted to do 2 things in parallel.
By calling dispose then waitFor, in most cases the debugee would finish
and report status before all the dispose operations completed.
But some tests would fail if the dispose happened quicker than the 
debuggee

could report final exit status.

The tests based on JDIBreakpointTest on  the other hand don't really
care about the the final exit status. Once all the events have been 
seen and handled, the test wants to shutdown the session.


I'm still not seeing how JDIBreakpointTest started hanging/timing-out 
after you switched the order of dispose and waitFor. Does dispose 
affect the debuggee process or the debugger process?

The JDIBreakpointTest would timeout on the endDebugee call to waitFor.
There was no reason for the debugee to exit at that point.
The call to dispose, provided the debugee with it's reason to exit.
Not sure which operation induces the debugee to exit.


Thanks,
David



On 2/12/19, 6:59 AM, David Holmes wrote:

Hi Gary,

On 12/02/2019 8:08 pm, gary.ad...@oracle.com wrote:

The recent change to JDK-8068225 changed the order of operations
in Debugee.endDebugee() to wait for the debugee to exit before
disposing of the vm on the debugger side of the connection.
For the tests based on JDIBreakpointTest the debuggee exit
status is not used and the tests relied on the
debugger side dispose operation to end the test.

Since JDIBreakpointTest already includes a call to wait for
the debugee, if does not need to use endDebuggee()
to dispose and wait for the debugee to finish.


I agree that potentially calling waitFor twice seems pointless. But 
how did the reordering of vm.dispose() and waitFor() cause all these 
tests to hang if they were waiting anyway? Does vm.dispose() have an 
effect on destroying the process?


Also what concerns me is that dispose() is not resilient the way 
that endDebuggee is:


 public void dispose() {
vm.dispose();
 }

versus

public int endDebugee() {
if (vm != null) {
try {
vm.dispose();
} catch (VMDisconnectedException ignore) {
}
vm = null;
}

Do we need to be concerned with a null VM or getting 
VMDisconnectedException?


Thanks,
David

Testing in progress. The vm/mlvm tests are included in tiers 2, 3 
and 6.


diff --git 
a/test/hotspot/jtreg/vmTestbase/vm/mlvm/share/jdi/JDIBreakpointTest.java 
b/test/hotspot/jtreg/vmTestbase/vm/mlvm/share/jdi/JDIBreakpointTest.java 

--- 
a/test/hotspot/jtreg/vmTestbase/vm/mlvm/share/jdi/JDIBreakpointTest.java 

+++ 
b/test/hotspot/jtreg/vmTestbase/vm/mlvm/share/jdi/JDIBreakpointTest.java 


@@ -1,5 +1,5 @@
  /*
- * Copyright (c) 2011, 2018, Oracle and/or its affiliates. All 
rights reserved.
+ * Copyright (c) 2011, 2019, 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

@@ -359,7 +359,7 @@
  }.go();

  if (!debuggee.terminated())
-debuggee.endDebugee();
+debuggee.dispose();

  debuggee.waitFor();
  return true;

Re: RFR: JDK-8218754: JDK-8068225 regression in JDIBreakpointTest

[Gary Adams]

Yes, see the revised webrev, we do have to guard against
multiple calls to dispose, e.g. catch and ignore VMDisconnectException.
We don't need to guard against a null vm. That would only exist if
the vm was never initialized and we were shutting down.

There is a race condition between the debuggee and the debugger process.
In the original endDebugee, it attempted to do 2 things in parallel.
By calling dispose then waitFor, in most cases the debugee would finish
and report status before all the dispose operations completed.
But some tests would fail if the dispose happened quicker than the debuggee
could report final exit status.

The tests based on JDIBreakpointTest on  the other hand don't really
care about the the final exit status. Once all the events have been seen and
handled, the test wants to shutdown the session.

On 2/12/19, 6:59 AM, David Holmes wrote:

Hi Gary,

On 12/02/2019 8:08 pm, gary.ad...@oracle.com wrote:

The recent change to JDK-8068225 changed the order of operations
in Debugee.endDebugee() to wait for the debugee to exit before
disposing of the vm on the debugger side of the connection.
For the tests based on JDIBreakpointTest the debuggee exit
status is not used and the tests relied on the
debugger side dispose operation to end the test.

Since JDIBreakpointTest already includes a call to wait for
the debugee, if does not need to use endDebuggee()
to dispose and wait for the debugee to finish.


I agree that potentially calling waitFor twice seems pointless. But 
how did the reordering of vm.dispose() and waitFor() cause all these 
tests to hang if they were waiting anyway? Does vm.dispose() have an 
effect on destroying the process?


Also what concerns me is that dispose() is not resilient the way that 
endDebuggee is:


 public void dispose() {
vm.dispose();
 }

versus

public int endDebugee() {
if (vm != null) {
try {
vm.dispose();
} catch (VMDisconnectedException ignore) {
}
vm = null;
}

Do we need to be concerned with a null VM or getting 
VMDisconnectedException?


Thanks,
David


Testing in progress. The vm/mlvm tests are included in tiers 2, 3 and 6.

diff --git 
a/test/hotspot/jtreg/vmTestbase/vm/mlvm/share/jdi/JDIBreakpointTest.java 
b/test/hotspot/jtreg/vmTestbase/vm/mlvm/share/jdi/JDIBreakpointTest.java
--- 
a/test/hotspot/jtreg/vmTestbase/vm/mlvm/share/jdi/JDIBreakpointTest.java
+++ 
b/test/hotspot/jtreg/vmTestbase/vm/mlvm/share/jdi/JDIBreakpointTest.java

@@ -1,5 +1,5 @@
  /*
- * Copyright (c) 2011, 2018, Oracle and/or its affiliates. All 
rights reserved.
+ * Copyright (c) 2011, 2019, 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

@@ -359,7 +359,7 @@
  }.go();

  if (!debuggee.terminated())
-debuggee.endDebugee();
+debuggee.dispose();

  debuggee.waitFor();
  return true;

Re: RFR: JDK-8218754: JDK-8068225 regression in JDIBreakpointTest

[Gary Adams]

Have to guard against multiple calls to dispose.

  Revised webrev: http://cr.openjdk.java.net/~gadams/8218754/webrev.00/
  Issue: https://bugs.openjdk.java.net/browse/JDK-8218754


On 2/12/19, 5:08 AM, gary.ad...@oracle.com wrote:

The recent change to JDK-8068225 changed the order of operations
in Debugee.endDebugee() to wait for the debugee to exit before
disposing of the vm on the debugger side of the connection.
For the tests based on JDIBreakpointTest the debuggee exit
status is not used and the tests relied on the
debugger side dispose operation to end the test.

Since JDIBreakpointTest already includes a call to wait for
the debugee, if does not need to use endDebuggee()
to dispose and wait for the debugee to finish.

Testing in progress. The vm/mlvm tests are included in tiers 2, 3 and 6.

diff --git 
a/test/hotspot/jtreg/vmTestbase/vm/mlvm/share/jdi/JDIBreakpointTest.java 
b/test/hotspot/jtreg/vmTestbase/vm/mlvm/share/jdi/JDIBreakpointTest.java
--- 
a/test/hotspot/jtreg/vmTestbase/vm/mlvm/share/jdi/JDIBreakpointTest.java
+++ 
b/test/hotspot/jtreg/vmTestbase/vm/mlvm/share/jdi/JDIBreakpointTest.java

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2011, 2018, Oracle and/or its affiliates. All rights 
reserved.
+ * Copyright (c) 2011, 2019, 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
@@ -359,7 +359,7 @@
 }.go();

 if (!debuggee.terminated())
-debuggee.endDebugee();
+debuggee.dispose();

 debuggee.waitFor();
 return true;

Re: RFR: JDK-8068225: nsk/jdi/EventQueue/remove_l/remove_l005 intermittently times out

[Gary Adams]

I missed a couple of tests in the vm/mlvm area {breakpoint, 
breakpointInCompiledCode,
breakpointOtherStratum}. I filed JDK-8218754 to follow up with those 
timeouts during
debugee exiting. The good news is the problem is easily reproduced in a 
local linux
debug build, so it should be easier to track down. e.g. 
JDIBreakpointTest has a

direct call to debugee.endDebugee(). May need the same fix used for
canBeModified, e.g. resume the debugee to let it finish before
waiting for an exit status.


On 2/8/19, 1:37 PM, serguei.spit...@oracle.com wrote:

Hi Gary,

I'm Okay with the fix if the test run is good.

Thanks,
Serguei


On 2/8/19 05:25, Gary Adams wrote:
The intermittent failures for remove_l005 appears to be due to a 
timing issue
in the shutdown handling after the test has completed. The debugger 
has sent
a "quit" command, but the debugee has not completed processing the 
command

and exited as expected.

This proposed change modifies the Debugee.endDebugee() processing to
wait for the debuggee to complete before doing the tear down processing
on the debugger side of the connection. This sequencing did expose
an issue in the canBeModified test, where the debugee was never resumed
after the VMStart event was detected. Resuming the debugee allows it to
respond as expected when the direct call to endDebugee is invoked.

Testing is still in progress, but it looks like this will resolve a 
number of

tail end intermittent failures.

  Issue: https://bugs.openjdk.java.net/browse/JDK-8068225
  Webrev: http://cr.openjdk.java.net/~gadams/8068225/webrev.00/

Re: RFR: JDK-8065773: JDI: UOE is not thrown, when redefineClasses changes a class modifier

[Gary Adams]

On 2/5/19, 4:59 PM, David Holmes wrote:

On 5/02/2019 10:17 pm, Gary Adams wrote:

On 2/4/19, 8:04 PM, David Holmes wrote:

Hi Gary,

On 5/02/2019 12:01 am, Gary Adams wrote:
Two of the redefine classes tests (021, 023) have been on the 
ProblemList since they
were first brought into the open repos. Both tests made an 
incorrect assumption
about the access modifiers in class files. There is a single bit in 
the binary class file that
defines if an interface is public or not public (See JVMS Table 
4.1-B ACC_PUBLIC).
When the test classes are compiled for use in the redefine 
operation, if a source

used public or protected the ACC_PUBLIC bit was set.

Several modifiers are used in these tests to confirm 
UnsupportedOperationException
is thrown or not thrown. This proposed change simply corrects the 
expected results

according to the JVM Specification.

   Webrev: 
http://cr.openjdk.java.net/~gadams/8065773/webrev.00/index.html


test/hotspot/jtreg/vmTestbase/nsk/jdi/VirtualMachine/redefineClasses/redefineclasses021.java 



62  * Test cases 3 (private) and 4 (static) map to not public, so will

"static" is not related to access modifiers so the ACC_PUBLIC bit is 
not relevant. "static" can only be applied to (nested) member types 
and is represented by the ACC_STATIC bit as per JVMS "Table 4.7.6-A. 
Nested class access and property flags".


What this testcase does is add "static" to a nested interface and 
expects JVM TI to check if the class modifier has changed. But the 
"static" is not encoded in the class "access flags", but in the 
inner class attribute (inner_class_access_flags). To me this 
testcase should throw UOE, but JVM TI is not checking the right flags.


David
I think the test is demonstrating that static does not change the 
ACC_PUBLIC bit.


And what is that a demonstration of? static has nothing to do with 
access control so if it did change the ACC_PUBLIC bit we'd have a 
serious issue.




This changeset does not change the handling of that testcase.


The test is adding a new class modifier "static" which it originally 
expected to trigger a UOE but it doesn't because JVM TI is not 
checking for that kind of change correctly.


You modified the test to no longer expect UOE so that it passes but:
a) that's wrong as it shouldn't pass; and
b) it's now documented as passing because it doesn't change the 
public-ness of the class but that's completely irrelevant.


So your change to the static testcase is not correct and should not be 
applied.


David
-

I think you misread the webrev.
The 4th test case was already being allowed to not throw UOE.

The change I proposed allows the 3rd testcase to not throw UOE.
e.g. redefining package to private accessibility is still not ACC_PUBLIC.

RFR: JDK-8068225: nsk/jdi/EventQueue/remove_l/remove_l005 intermittently times out

[Gary Adams]

The intermittent failures for remove_l005 appears to be due to a timing 
issue

in the shutdown handling after the test has completed. The debugger has sent
a "quit" command, but the debugee has not completed processing the command
and exited as expected.

This proposed change modifies the Debugee.endDebugee() processing to
wait for the debuggee to complete before doing the tear down processing
on the debugger side of the connection. This sequencing did expose
an issue in the canBeModified test, where the debugee was never resumed
after the VMStart event was detected. Resuming the debugee allows it to
respond as expected when the direct call to endDebugee is invoked.

Testing is still in progress, but it looks like this will resolve a 
number of

tail end intermittent failures.

  Issue: https://bugs.openjdk.java.net/browse/JDK-8068225
  Webrev: http://cr.openjdk.java.net/~gadams/8068225/webrev.00/

Re: RFR: JDK-8068225: nsk/jdi/EventQueue/remove_l/remove_l005 intermittently times out

[Gary Adams]

Yes, the test was hung in waitFor.

The debugger process made some calls after the VMStart event
was observed, but never resumed the debuggee.

Here's a simple change that makes test run to completion cleanly.


diff --git 
a/test/hotspot/jtreg/vmTestbase/nsk/jdi/VirtualMachine/canBeModified/canbemodified001.java 
b/test/hotspot/jtreg/vmTestbase/nsk/jdi/VirtualMachine/canBeModified/canbemodified001.java
--- 
a/test/hotspot/jtreg/vmTestbase/nsk/jdi/VirtualMachine/canBeModified/canbemodified001.java
+++ 
b/test/hotspot/jtreg/vmTestbase/nsk/jdi/VirtualMachine/canBeModified/canbemodified001.java

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2003, 2018, Oracle and/or its affiliates. All rights 
reserved.
+ * Copyright (c) 2003, 2019, 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
@@ -78,6 +78,7 @@
 exitStatus = Consts.TEST_FAILED;
 e.printStackTrace();
 } finally {
+debugee.resume();
 debugee.endDebugee();
 }
 display("Test finished. exitStatus = " + exitStatus);

On 2/6/19, 1:16 PM, Chris Plummer wrote:

Does it timeout in waitFor()?

Chris

On 2/6/19 10:01 AM, Gary Adams wrote:
So far the only test that has an issue with the change in waitFor 
order is
nsk/jdi/VirtualMachine/canBeModified, which does not appear to 
synchronize

with the debuggee.

On 2/6/19, 12:15 PM, Chris Plummer wrote:
What happens if endDebugee() is called when the caller is not 
expecting a clean exit of the debugeee? Won't waitFor() wait forever 
in that case? I think by having the vm.dispose() first, you avoid 
that issue.


Chris

On 2/6/19 8:04 AM, Gary Adams wrote:
Testing an alternate fix that will wait for the debugee process to 
terminate after processing the "quit"

command before doing the vm tear down.


diff --git 
a/test/hotspot/jtreg/vmTestbase/nsk/share/jdi/Debugee.java 
b/test/hotspot/jtreg/vmTestbase/nsk/share/jdi/Debugee.java

--- a/test/hotspot/jtreg/vmTestbase/nsk/share/jdi/Debugee.java
+++ b/test/hotspot/jtreg/vmTestbase/nsk/share/jdi/Debugee.java
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2001, 2018, Oracle and/or its affiliates. All 
rights reserved.
+ * Copyright (c) 2001, 2019, 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

@@ -557,6 +557,7 @@
  * exit status code.
  */
 public int endDebugee() {
+int status = waitFor();
 if (vm != null) {
 try {
 vm.dispose();
@@ -564,7 +565,7 @@
 }
 vm = null;
 }
-return waitFor();
+return status;
 }


On 2/6/19, 8:14 AM, Gary Adams wrote:

Just a quick update on the failure on shutdown ...
I've come across a couple of other tests with a similar failure mode.

I'm attempting to get some additional diagnostic information from 
these
tests with "-jdi.trace=all", but the diagnostic output may be 
interfering

with the timing.

My current investigation is around a possible race condition
in the Debgugee.quit() processing.
   - the "quit" string is sent to the deuggee via the IOpipe
   - the vm.dispose() is handled from the JDWP communication

If the context switching is not quick enough, the debugee
may not have read and processed the "quit" command before
tear down is handled.

From earlier comments in the bug report the debuggee was observed
still at the breakpoint, so the resume and the disabling of the 
breakpoint

had not been processed, yet.

It might be worth having an "ack" returned when the "quit" is
processed in the debugee.

...

On 2/1/19, 2:15 PM, serguei.spit...@oracle.com wrote:

Hi Gary,

The debugee.quit() is to complete the session.
It makes this call: sendSignal(SGNL_QUIT);

A call to the debugee.quit() is at the end of execTest() method:

display("");
}

display("");
display("=");
display("TEST FINISHES\n");

brkpReq.disable();
debugee.resume();
debugee.quit();
}

Thanks,
Serguei


On 2/1/19 10:00, Gary Adams wrote:
When the remove_l005 runs to completion, it never signals the 
debuggee

that all iterations have completed.

  Issue: https://bugs.openjdk.java.net/browse/JDK-8068225

diff --git a/test/hotspot/jtreg/ProblemList.txt 
b/test/hotspot/jtreg/ProblemList.txt

--- a/test/hotspot/jtreg/ProblemList.txt
+++ b/test/hotspot/jtreg/ProblemList.txt
@@ -163,7 +163,6 @@
 vmTestbase/nsk/jdi/EventRequest/setEnabled/setenabled003/TestDescription.java 
8066993 generic-all
 vmTestbase/nsk/jdi/VirtualMachine/redefineClasses/redefineclasses021/TestDescription.java 
8065773 generic-all
 vmTestbase/nsk/jdi/VirtualMachi

Re: RFR: JDK-8068225: nsk/jdi/EventQueue/remove_l/remove_l005 intermittently times out

[Gary Adams]

So far the only test that has an issue with the change in waitFor order is
nsk/jdi/VirtualMachine/canBeModified, which does not appear to synchronize
with the debuggee.

On 2/6/19, 12:15 PM, Chris Plummer wrote:
What happens if endDebugee() is called when the caller is not 
expecting a clean exit of the debugeee? Won't waitFor() wait forever 
in that case? I think by having the vm.dispose() first, you avoid that 
issue.


Chris

On 2/6/19 8:04 AM, Gary Adams wrote:
Testing an alternate fix that will wait for the debugee process to 
terminate after processing the "quit"

command before doing the vm tear down.


diff --git a/test/hotspot/jtreg/vmTestbase/nsk/share/jdi/Debugee.java 
b/test/hotspot/jtreg/vmTestbase/nsk/share/jdi/Debugee.java

--- a/test/hotspot/jtreg/vmTestbase/nsk/share/jdi/Debugee.java
+++ b/test/hotspot/jtreg/vmTestbase/nsk/share/jdi/Debugee.java
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2001, 2018, Oracle and/or its affiliates. All 
rights reserved.
+ * Copyright (c) 2001, 2019, 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
@@ -557,6 +557,7 @@
  * exit status code.
  */
 public int endDebugee() {
+int status = waitFor();
 if (vm != null) {
 try {
 vm.dispose();
@@ -564,7 +565,7 @@
 }
 vm = null;
 }
-return waitFor();
+return status;
 }


On 2/6/19, 8:14 AM, Gary Adams wrote:

Just a quick update on the failure on shutdown ...
I've come across a couple of other tests with a similar failure mode.

I'm attempting to get some additional diagnostic information from these
tests with "-jdi.trace=all", but the diagnostic output may be 
interfering

with the timing.

My current investigation is around a possible race condition
in the Debgugee.quit() processing.
   - the "quit" string is sent to the deuggee via the IOpipe
   - the vm.dispose() is handled from the JDWP communication

If the context switching is not quick enough, the debugee
may not have read and processed the "quit" command before
tear down is handled.

From earlier comments in the bug report the debuggee was observed
still at the breakpoint, so the resume and the disabling of the 
breakpoint

had not been processed, yet.

It might be worth having an "ack" returned when the "quit" is
processed in the debugee.

...

On 2/1/19, 2:15 PM, serguei.spit...@oracle.com wrote:

Hi Gary,

The debugee.quit() is to complete the session.
It makes this call: sendSignal(SGNL_QUIT);

A call to the debugee.quit() is at the end of execTest() method:

display("");
}

display("");
display("=");
display("TEST FINISHES\n");

brkpReq.disable();
    debugee.resume();
debugee.quit();
}

Thanks,
Serguei


On 2/1/19 10:00, Gary Adams wrote:
When the remove_l005 runs to completion, it never signals the 
debuggee

that all iterations have completed.

  Issue: https://bugs.openjdk.java.net/browse/JDK-8068225

diff --git a/test/hotspot/jtreg/ProblemList.txt 
b/test/hotspot/jtreg/ProblemList.txt

--- a/test/hotspot/jtreg/ProblemList.txt
+++ b/test/hotspot/jtreg/ProblemList.txt
@@ -163,7 +163,6 @@
 vmTestbase/nsk/jdi/EventRequest/setEnabled/setenabled003/TestDescription.java 
8066993 generic-all
 vmTestbase/nsk/jdi/VirtualMachine/redefineClasses/redefineclasses021/TestDescription.java 
8065773 generic-all
 vmTestbase/nsk/jdi/VirtualMachine/redefineClasses/redefineclasses023/TestDescription.java 
8065773 generic-all
-vmTestbase/nsk/jdi/EventQueue/remove_l/remove_l005/TestDescription.java 
8068225 generic-all


 vmTestbase/metaspace/gc/firstGC_10m/TestDescription.java 8208250 
generic-all
 vmTestbase/metaspace/gc/firstGC_50m/TestDescription.java 8208250 
generic-all



diff --git 
a/test/hotspot/jtreg/vmTestbase/nsk/jdi/EventQueue/remove_l/remove_l005.java 
b/test/hotspot/jtreg/vmTestbase/nsk/jdi/EventQueue/remove_l/remove_l005.java 

--- 
a/test/hotspot/jtreg/vmTestbase/nsk/jdi/EventQueue/remove_l/remove_l005.java
+++ 
b/test/hotspot/jtreg/vmTestbase/nsk/jdi/EventQueue/remove_l/remove_l005.java

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2002, 2018, Oracle and/or its affiliates. All 
rights reserved.
+ * Copyright (c) 2002, 2019, 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

@@ -134,6 +134,7 @@
 }
 display("");
 }
+debugee.sendSignal(SGNL_QUIT);

 display("");
 display("=");

Re: RFR: JDK-8068225: nsk/jdi/EventQueue/remove_l/remove_l005 intermittently times out

[Gary Adams]

Testing an alternate fix that will wait for the debugee process to 
terminate after processing the "quit"

command before doing the vm tear down.


diff --git a/test/hotspot/jtreg/vmTestbase/nsk/share/jdi/Debugee.java 
b/test/hotspot/jtreg/vmTestbase/nsk/share/jdi/Debugee.java

--- a/test/hotspot/jtreg/vmTestbase/nsk/share/jdi/Debugee.java
+++ b/test/hotspot/jtreg/vmTestbase/nsk/share/jdi/Debugee.java
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2001, 2018, Oracle and/or its affiliates. All rights 
reserved.
+ * Copyright (c) 2001, 2019, 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
@@ -557,6 +557,7 @@
  * exit status code.
  */
 public int endDebugee() {
+int status = waitFor();
 if (vm != null) {
 try {
 vm.dispose();
@@ -564,7 +565,7 @@
 }
 vm = null;
 }
-return waitFor();
+return status;
 }


On 2/6/19, 8:14 AM, Gary Adams wrote:

Just a quick update on the failure on shutdown ...
I've come across a couple of other tests with a similar failure mode.

I'm attempting to get some additional diagnostic information from these
tests with "-jdi.trace=all", but the diagnostic output may be interfering
with the timing.

My current investigation is around a possible race condition
in the Debgugee.quit() processing.
   - the "quit" string is sent to the deuggee via the IOpipe
   - the vm.dispose() is handled from the JDWP communication

If the context switching is not quick enough, the debugee
may not have read and processed the "quit" command before
tear down is handled.

From earlier comments in the bug report the debuggee was observed
still at the breakpoint, so the resume and the disabling of the 
breakpoint

had not been processed, yet.

It might be worth having an "ack" returned when the "quit" is
processed in the debugee.

...

On 2/1/19, 2:15 PM, serguei.spit...@oracle.com wrote:

Hi Gary,

The debugee.quit() is to complete the session.
It makes this call: sendSignal(SGNL_QUIT);

A call to the debugee.quit() is at the end of execTest() method:

display("");
}

display("");
display("=");
display("TEST FINISHES\n");

brkpReq.disable();
    debugee.resume();
debugee.quit();
}

Thanks,
Serguei


On 2/1/19 10:00, Gary Adams wrote:

When the remove_l005 runs to completion, it never signals the debuggee
that all iterations have completed.

  Issue: https://bugs.openjdk.java.net/browse/JDK-8068225

diff --git a/test/hotspot/jtreg/ProblemList.txt 
b/test/hotspot/jtreg/ProblemList.txt

--- a/test/hotspot/jtreg/ProblemList.txt
+++ b/test/hotspot/jtreg/ProblemList.txt
@@ -163,7 +163,6 @@
 vmTestbase/nsk/jdi/EventRequest/setEnabled/setenabled003/TestDescription.java 
8066993 generic-all
 vmTestbase/nsk/jdi/VirtualMachine/redefineClasses/redefineclasses021/TestDescription.java 
8065773 generic-all
 vmTestbase/nsk/jdi/VirtualMachine/redefineClasses/redefineclasses023/TestDescription.java 
8065773 generic-all
-vmTestbase/nsk/jdi/EventQueue/remove_l/remove_l005/TestDescription.java 
8068225 generic-all


 vmTestbase/metaspace/gc/firstGC_10m/TestDescription.java 8208250 
generic-all
 vmTestbase/metaspace/gc/firstGC_50m/TestDescription.java 8208250 
generic-all



diff --git 
a/test/hotspot/jtreg/vmTestbase/nsk/jdi/EventQueue/remove_l/remove_l005.java 
b/test/hotspot/jtreg/vmTestbase/nsk/jdi/EventQueue/remove_l/remove_l005.java 

--- 
a/test/hotspot/jtreg/vmTestbase/nsk/jdi/EventQueue/remove_l/remove_l005.java
+++ 
b/test/hotspot/jtreg/vmTestbase/nsk/jdi/EventQueue/remove_l/remove_l005.java

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2002, 2018, Oracle and/or its affiliates. All 
rights reserved.
+ * Copyright (c) 2002, 2019, 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

@@ -134,6 +134,7 @@
 }
 display("");
 }
+debugee.sendSignal(SGNL_QUIT);

 display("");
 display("=");

Re: RFR: JDK-8068225: nsk/jdi/EventQueue/remove_l/remove_l005 intermittently times out

[Gary Adams]

Just a quick update on the failure on shutdown ...
I've come across a couple of other tests with a similar failure mode.

I'm attempting to get some additional diagnostic information from these
tests with "-jdi.trace=all", but the diagnostic output may be interfering
with the timing.

My current investigation is around a possible race condition
in the Debgugee.quit() processing.
   - the "quit" string is sent to the deuggee via the IOpipe
   - the vm.dispose() is handled from the JDWP communication

If the context switching is not quick enough, the debugee
may not have read and processed the "quit" command before
tear down is handled.

From earlier comments in the bug report the debuggee was observed
still at the breakpoint, so the resume and the disabling of the breakpoint
had not been processed, yet.

It might be worth having an "ack" returned when the "quit" is
processed in the debugee.

...

On 2/1/19, 2:15 PM, serguei.spit...@oracle.com wrote:

Hi Gary,

The debugee.quit() is to complete the session.
It makes this call: sendSignal(SGNL_QUIT);

A call to the debugee.quit() is at the end of execTest() method:

display("");
}

display("");
display("=");
display("TEST FINISHES\n");

brkpReq.disable();
    debugee.resume();
debugee.quit();
}

Thanks,
Serguei


On 2/1/19 10:00, Gary Adams wrote:

When the remove_l005 runs to completion, it never signals the debuggee
that all iterations have completed.

  Issue: https://bugs.openjdk.java.net/browse/JDK-8068225

diff --git a/test/hotspot/jtreg/ProblemList.txt 
b/test/hotspot/jtreg/ProblemList.txt

--- a/test/hotspot/jtreg/ProblemList.txt
+++ b/test/hotspot/jtreg/ProblemList.txt
@@ -163,7 +163,6 @@
 vmTestbase/nsk/jdi/EventRequest/setEnabled/setenabled003/TestDescription.java 
8066993 generic-all
 vmTestbase/nsk/jdi/VirtualMachine/redefineClasses/redefineclasses021/TestDescription.java 
8065773 generic-all
 vmTestbase/nsk/jdi/VirtualMachine/redefineClasses/redefineclasses023/TestDescription.java 
8065773 generic-all
-vmTestbase/nsk/jdi/EventQueue/remove_l/remove_l005/TestDescription.java 
8068225 generic-all


 vmTestbase/metaspace/gc/firstGC_10m/TestDescription.java 8208250 
generic-all
 vmTestbase/metaspace/gc/firstGC_50m/TestDescription.java 8208250 
generic-all



diff --git 
a/test/hotspot/jtreg/vmTestbase/nsk/jdi/EventQueue/remove_l/remove_l005.java 
b/test/hotspot/jtreg/vmTestbase/nsk/jdi/EventQueue/remove_l/remove_l005.java 

--- 
a/test/hotspot/jtreg/vmTestbase/nsk/jdi/EventQueue/remove_l/remove_l005.java
+++ 
b/test/hotspot/jtreg/vmTestbase/nsk/jdi/EventQueue/remove_l/remove_l005.java

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2002, 2018, Oracle and/or its affiliates. All 
rights reserved.
+ * Copyright (c) 2002, 2019, 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
@@ -134,6 +134,7 @@
 }
 display("");
 }
+debugee.sendSignal(SGNL_QUIT);

 display("");
 display("=");

Re: RFR: JDK-8065773: JDI: UOE is not thrown, when redefineClasses changes a class modifier

[Gary Adams]

On 2/4/19, 8:04 PM, David Holmes wrote:

Hi Gary,

On 5/02/2019 12:01 am, Gary Adams wrote:
Two of the redefine classes tests (021, 023) have been on the 
ProblemList since they
were first brought into the open repos. Both tests made an incorrect 
assumption
about the access modifiers in class files. There is a single bit in 
the binary class file that
defines if an interface is public or not public (See JVMS Table 4.1-B 
ACC_PUBLIC).
When the test classes are compiled for use in the redefine operation, 
if a source

used public or protected the ACC_PUBLIC bit was set.

Several modifiers are used in these tests to confirm 
UnsupportedOperationException
is thrown or not thrown. This proposed change simply corrects the 
expected results

according to the JVM Specification.

   Webrev: 
http://cr.openjdk.java.net/~gadams/8065773/webrev.00/index.html


test/hotspot/jtreg/vmTestbase/nsk/jdi/VirtualMachine/redefineClasses/redefineclasses021.java 



62  * Test cases 3 (private) and 4 (static) map to not public, so will

"static" is not related to access modifiers so the ACC_PUBLIC bit is 
not relevant. "static" can only be applied to (nested) member types 
and is represented by the ACC_STATIC bit as per JVMS "Table 4.7.6-A. 
Nested class access and property flags".


What this testcase does is add "static" to a nested interface and 
expects JVM TI to check if the class modifier has changed. But the 
"static" is not encoded in the class "access flags", but in the inner 
class attribute (inner_class_access_flags). To me this testcase should 
throw UOE, but JVM TI is not checking the right flags.


David
I think the test is demonstrating that static does not change the 
ACC_PUBLIC bit.


This changeset does not change the handling of that testcase.

RFR: JDK-8065773: JDI: UOE is not thrown, when redefineClasses changes a class modifier

[Gary Adams]

Two of the redefine classes tests (021, 023) have been on the 
ProblemList since they
were first brought into the open repos. Both tests made an incorrect 
assumption
about the access modifiers in class files. There is a single bit in the 
binary class file that
defines if an interface is public or not public (See JVMS Table 4.1-B 
ACC_PUBLIC).
When the test classes are compiled for use in the redefine operation, if 
a source

used public or protected the ACC_PUBLIC bit was set.

Several modifiers are used in these tests to confirm 
UnsupportedOperationException
is thrown or not thrown. This proposed change simply corrects the 
expected results

according to the JVM Specification.

  Webrev: http://cr.openjdk.java.net/~gadams/8065773/webrev.00/index.html

RFR: JDK-8068225: nsk/jdi/EventQueue/remove_l/remove_l005 intermittently times out

[Gary Adams]

When the remove_l005 runs to completion, it never signals the debuggee
that all iterations have completed.

  Issue: https://bugs.openjdk.java.net/browse/JDK-8068225

diff --git a/test/hotspot/jtreg/ProblemList.txt 
b/test/hotspot/jtreg/ProblemList.txt

--- a/test/hotspot/jtreg/ProblemList.txt
+++ b/test/hotspot/jtreg/ProblemList.txt
@@ -163,7 +163,6 @@
 vmTestbase/nsk/jdi/EventRequest/setEnabled/setenabled003/TestDescription.java 
8066993 generic-all
 
vmTestbase/nsk/jdi/VirtualMachine/redefineClasses/redefineclasses021/TestDescription.java
 8065773 generic-all
 
vmTestbase/nsk/jdi/VirtualMachine/redefineClasses/redefineclasses023/TestDescription.java
 8065773 generic-all
-vmTestbase/nsk/jdi/EventQueue/remove_l/remove_l005/TestDescription.java 
8068225 generic-all


 vmTestbase/metaspace/gc/firstGC_10m/TestDescription.java 8208250 
generic-all
 vmTestbase/metaspace/gc/firstGC_50m/TestDescription.java 8208250 
generic-all



diff --git 
a/test/hotspot/jtreg/vmTestbase/nsk/jdi/EventQueue/remove_l/remove_l005.java 
b/test/hotspot/jtreg/vmTestbase/nsk/jdi/EventQueue/remove_l/remove_l005.java
--- 
a/test/hotspot/jtreg/vmTestbase/nsk/jdi/EventQueue/remove_l/remove_l005.java
+++ 
b/test/hotspot/jtreg/vmTestbase/nsk/jdi/EventQueue/remove_l/remove_l005.java

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2002, 2018, Oracle and/or its affiliates. All rights 
reserved.
+ * Copyright (c) 2002, 2019, 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
@@ -134,6 +134,7 @@
 }
 display("");
 }
+debugee.sendSignal(SGNL_QUIT);

 display("");
 display("=");

Re: RFR: JDK-8215550: Debugger does not work after reattach

[Gary Adams]

Following the trail from debugLoop_run, at the bottom of the loop
there is a path through debugInit_reset that involves eventHandler_reset
and eventually eventHelper_reset. This seems like a better place to
clear the flag back to original state.

  Revised webrev: http://cr.openjdk.java.net/~gadams/8215550/webrev.01/

On 1/29/19, 6:11 PM, Chris Plummer wrote:
Ok, so you can't do a "cont" because threads are not suspended. That 
means someone resumed them. When/where was this done?


Regarding threadIDs changing, my guess is that debugLoop_run() is 
re-entered when the new connection is established. This will result in 
commonRef_reset() being called, which invalidates all reference IDs, 
including threadIDs. So the first time the agent needs to send a 
threadID to the debugger, it needs to create a new one.


Chris

On 1/29/19 1:52 PM, gary.ad...@oracle.com wrote:

Issuing a "cont" in the second session does not work, because
the threads are not suspended.

It's interesting that the thread ids have all changed in
the reconnected session.

...

main[1] threads
Group system:
  (java.lang.ref.Reference$ReferenceHandler)0x374 Reference Handler 
running

  (java.lang.ref.Finalizer$FinalizerThread)0x375 Finalizer cond. waiting
  (java.lang.Thread)0x376 Signal Dispatcher 
running

Group main:
  (java.lang.Thread)0x1 main  running (at breakpoint)
Group InnocuousThreadGroup:
  (jdk.internal.misc.InnocuousThread)0x377 Common-Cleanercond. 
waiting

main[1] exit
...
> threads
Group system:
  (java.lang.ref.Reference$ReferenceHandler)0x3b2 Reference Handler 
running

  (java.lang.ref.Finalizer$FinalizerThread)0x3b3 Finalizer cond. waiting
  (java.lang.Thread)0x3b4 Signal Dispatcher 
running

Group main:
  (java.lang.Thread)0x3b7 main  running
Group InnocuousThreadGroup:
  (jdk.internal.misc.InnocuousThread)0x3b8 Common-Cleanercond. 
waiting

> cont
> Nothing suspended.






On 1/29/19 2:27 PM, Chris Plummer wrote:
What is the state of the threads after the detach? If they are all 
automatically resumed by the agent, then I think the unblocking 
should be done by the same code that resumes the threads. If they 
are still suspended, then why would we want to unblock when the next 
connection comes in? It should be up to the debugger to detect that 
all threads are suspended and act accordingly.


Also, what happens if after attaching again you issue a "cont" command?

Chris

On 1/29/19 9:55 AM, Gary Adams wrote:
As far as I can tell, the quit and exit commands are only handled 
locally

on the debugger side of the connection. There is no packet sent to the
libjdwp agentlib.

On 1/29/19, 12:17 PM, Chris Plummer wrote:

Hi Gary,

When the "exit" or "quit" is done, aren't all threads resumed at 
that point, and shouldn't that result in the command loop being 
unblocked?


thanks,

Chris

On 1/29/19 8:09 AM, Gary Adams wrote:

Significant protections are put in place to protect the commandLoop
from multiple events that that have a suspend-all policy. The
commandLoop uses a special block variable to ensure only
a VirtualMachine or ThreadReference call to resume() will unblock
the commandLoop. See

  src/jdk.jdwp.agent/share/native/libjdwp/eventHelper.c

In this particular bug report, the user was stopped at a breakpoint
when they sent the "exit" command. The same effect can be produced
with a "quit" command or any killing of the debugger process.

When the second session is started the commandLoop is still blocked,
so a new breakpoint will never be dequeued from the commandQueue.

The proposed workaround will ensure the commandLoop is unblocked
when a new session is started.

  Issue: https://bugs.openjdk.java.net/browse/JDK-8215550
  Webrev: http://cr.openjdk.java.net/~gadams/8215550/webrev.00/

All testing has been done by manually replicating the reported
command sequences. I'll see if an existing breakpoint test can be
enhanced to cover this scenario.

Re: RFR: JDK-8215550: Debugger does not work after reattach

[Gary Adams]

As far as I can tell, the quit and exit commands are only handled locally
on the debugger side of the connection. There is no packet sent to the
libjdwp agentlib.

On 1/29/19, 12:17 PM, Chris Plummer wrote:

Hi Gary,

When the "exit" or "quit" is done, aren't all threads resumed at that 
point, and shouldn't that result in the command loop being unblocked?


thanks,

Chris

On 1/29/19 8:09 AM, Gary Adams wrote:

Significant protections are put in place to protect the commandLoop
from multiple events that that have a suspend-all policy. The
commandLoop uses a special block variable to ensure only
a VirtualMachine or ThreadReference call to resume() will unblock
the commandLoop. See

  src/jdk.jdwp.agent/share/native/libjdwp/eventHelper.c

In this particular bug report, the user was stopped at a breakpoint
when they sent the "exit" command. The same effect can be produced
with a "quit" command or any killing of the debugger process.

When the second session is started the commandLoop is still blocked,
so a new breakpoint will never be dequeued from the commandQueue.

The proposed workaround will ensure the commandLoop is unblocked
when a new session is started.

  Issue: https://bugs.openjdk.java.net/browse/JDK-8215550
  Webrev: http://cr.openjdk.java.net/~gadams/8215550/webrev.00/

All testing has been done by manually replicating the reported
command sequences. I'll see if an existing breakpoint test can be
enhanced to cover this scenario.

RFR: JDK-8215550: Debugger does not work after reattach

[Gary Adams]

Significant protections are put in place to protect the commandLoop
from multiple events that that have a suspend-all policy. The
commandLoop uses a special block variable to ensure only
a VirtualMachine or ThreadReference call to resume() will unblock
the commandLoop. See

  src/jdk.jdwp.agent/share/native/libjdwp/eventHelper.c

In this particular bug report, the user was stopped at a breakpoint
when they sent the "exit" command. The same effect can be produced
with a "quit" command or any killing of the debugger process.

When the second session is started the commandLoop is still blocked,
so a new breakpoint will never be dequeued from the commandQueue.

The proposed workaround will ensure the commandLoop is unblocked
when a new session is started.

  Issue: https://bugs.openjdk.java.net/browse/JDK-8215550
  Webrev: http://cr.openjdk.java.net/~gadams/8215550/webrev.00/

All testing has been done by manually replicating the reported
command sequences. I'll see if an existing breakpoint test can be
enhanced to cover this scenario.

Re: RFR JDK-8216386: vmTestbase/nsk/jvmti/PopFrame/popframe005/TestDescription.java fails

[Gary Adams]

I like the fact that test.timeout.factor is applied as a multiplier.

It's not clear why an upper limit had to be added.
Is that 50 or 5 seconds?

 148 objWaiterThr1.join(Math.min(WAIT_TIME, 5));

Why are the other wait times not limited?

 136 objWaiterThr2.join(WAIT_TIME);

...

 169 popFrameClsThr.join(WAIT_TIME);


If you need to apply the upper limit, then it would be better to apply it
at the beginning.

  38 static final long WAIT_TIME = Math.min(Utils.adjustTimeout(2000),5000);





On 1/16/19, 6:28 PM, Alex Menkov wrote:

Hi all,

please review a fix for
https://bugs.openjdk.java.net/browse/JDK-8216386
webrev:
http://cr.openjdk.java.net/~amenkov/popframe005_wait_time/webrev/

The fix updates WAIT_TIME to depend on test.timeout.factor system 
property.

WAIT_TIME value is used as argument of Thread.join().
For the case when the thread is expected to be alive (i.e. 
Thread.join() exits by timeout) the timeout value is restricted by 5 
seconds to avoid long run time with big timeout.factor values.


--alex

Re: RFR: JDK-8158066: SourceDebugExtensionTest fails to rename file

[Gary Adams]

The interference of a virus scanner for

  delete A
  rename C to A

makes sense, if the original A is not completely deleted
when the rename is requested.

For the suggested replacement

  rename A to A1

The file A1 does not exist so there is no collision taking place.
If  a virus scanner was holding a reference to A1, it
would not collide with the

   rename C to A

There were no failures during testing with the fix in place.

Do you have any specific references to "renames on Windows"
restrictions?

Should we hold back on the "rename A to A1" workaround?

Another possibility is to use the newer Windows MoveFile function...


On 1/16/19, 11:24 AM, Daniel D. Daugherty wrote:

> rename A to A1

I believe the virus scanner will also mess with file renames on Win*
so you may see intermittent failures for this operation.

Dan


On 1/11/19 11:01 AM, Gary Adams wrote:

I've been reading recently that Windows delete file operations
can be delayed, if there are open handles against the file.
Others have reported virus checkers or other indexing
programs may hold a handle on a file preventing the
deletion being completed.

The basic logic in InstallSDE is
   A + B = C
   delete A
   rename C to A

I think we can work around the issue with
  rename A to A1
  A1 + B = C
  delete A1
  rename C to A

diff --git a/test/jdk/com/sun/jdi/sde/InstallSDE.java 
b/test/jdk/com/sun/jdi/sde/InstallSDE.java

--- a/test/jdk/com/sun/jdi/sde/InstallSDE.java
+++ b/test/jdk/com/sun/jdi/sde/InstallSDE.java
@@ -31,9 +31,17 @@
 }

 static void install(File inOutClassFile, File attrFile) throws 
IOException {

-File tmpFile = new File(inOutClassFile.getPath() + "tmp");
-new InstallSDE(inOutClassFile, attrFile, tmpFile);
-if (!inOutClassFile.delete()) {
+File tmpFile = new File(inOutClassFile.getPath() + "tmp-out");
+File tmpInOutClassFile = new File(inOutClassFile.getPath() + 
"tmp-in");

+
+// Workaround delayed file deletes on Windows using a tmp 
file name

+if (!inOutClassFile.renameTo(tmpInOutClassFile)) {
+throw new IOException("tmp copy of inOutClassFile failed");
+}
+
+new InstallSDE(tmpInOutClassFile, attrFile, tmpFile);
+
+if (!tmpInOutClassFile.delete()) {
 throw new IOException("inOutClassFile.delete() failed");
 }
 if (!tmpFile.renameTo(inOutClassFile)) {


Testing in progress ...

On 1/11/19, 7:40 AM, David Holmes wrote:

Hi Gary,

On 11/01/2019 9:22 pm, gary.ad...@oracle.com wrote:
After ~1000 testruns I hit a failure in MangleTest and a jtreg 
agent communication issue with SourceDebugExtension.


https://java.se.oracle.com:10065/mdash/builds/2019-01-10-1159535.gary.adams.jdk-jdb/results?search=status%3Afailed%20AND%20-state%3Ainvalid 




*java.io.IOException: tmpFile.renameTo(inOutClassFile) failed*
at InstallSDE.install(InstallSDE.java:40)
at MangleTest.testSetUp(MangleTest.java:38)
at MangleTest.main(MangleTest.java:31)


You might want to add some code in:

./java.base/windows/native/libjava/WinNTFileSystem_md.c:Java_java_io_WinNTFileSystem_rename0 



that checks the result of _wrename to see what error is occurring. I 
suspect it will be EACCES which won't be that helpful:


https://docs.microsoft.com/en-us/cpp/c-runtime-library/reference/rename-wrename?view=vs-2017 



but it would be good to confirm.



result: Error. Agent communication error: 
java.io.IOException:*Agent: unexpected op: 71;*  check console log 
for any additional details


I added some additional print outs for the rename issue and have 
run ~2000 additional
testruns with no test failures repeated, yet. But there were 2 
mach5 reported errors:


TimeoutException in CLEANUP.


That's a mach5 issue.

David



I'll do some more investigation with making the rename operation 
more robust

before removing SourceDebugExtensionTest from the problem list.

On 1/10/19 8:28 PM, David Holmes wrote:

Hi Gary,

On 10/01/2019 11:02 pm, gary.ad...@oracle.com wrote:

SourceDebugExtensionTest was placed on the ProblemList
because of filesystem rename issues on Windows 2012 test systems.
The test does not appear to fail on the current mach5 test systems.
There was some question concerning symbolic links that might have
been problematic at that time. Additional testing in progress...


Also note that there are two sets of SDE related tests and two 
versions of the InstallSDE.java file that both use renameTo. In 
nsk the testcase seems to be:


./hotspot/jtreg/vmTestbase/vm/mlvm/share/StratumClassesBuilder.java

and AFAICS it is not excluded. So removing the exclusion seems 
reasonable to me. It was most like a concurrency issue with the 
virus scanner.


Thanks,
David



   Issue: https://bugs.openjdk.java.net/browse/JDK-8158066

diff --git a/test/jdk/ProblemList.txt b/test/jdk/ProblemList.txt
--- a/test/jdk/ProblemList.txt
+++ b/test/jdk/Proble

Re: RFR: JDK-8158066: SourceDebugExtensionTest fails to rename file

[Gary Adams]

Looking through the test history for the vm/mlvm version of 
InstallSDE.java there has never
been a reported problem with the renameTo operation. I'm inclined to not 
apply the change

made to the com/sun/jdi/sde test until the failure is observed.

I'm satisfied with the testing at this point and could use another 
reviewer before

pushing.

On 1/11/19, 6:14 PM, David Holmes wrote:

Hi Gary,

As I mentioned earlier there are two versions of InstallSDE.java in 
the tests, so both should be updated.


Your strategy to avoid the windows delete issue seems reasonable.

David

On 12/01/2019 4:53 am, Gary Adams wrote:

Here's a webrev for review purposes.
No failures after ~1000 testruns.

   Webrev: http://cr.openjdk.java.net/~gadams/8158066/webrev.00/
   Issue: https://bugs.openjdk.java.net/browse/JDK-8158066

On 1/11/19, 11:01 AM, Gary Adams wrote:

I've been reading recently that Windows delete file operations
can be delayed, if there are open handles against the file.
Others have reported virus checkers or other indexing
programs may hold a handle on a file preventing the
deletion being completed.

The basic logic in InstallSDE is
   A + B = C
   delete A
   rename C to A

I think we can work around the issue with
  rename A to A1
  A1 + B = C
  delete A1
  rename C to A

diff --git a/test/jdk/com/sun/jdi/sde/InstallSDE.java 
b/test/jdk/com/sun/jdi/sde/InstallSDE.java

--- a/test/jdk/com/sun/jdi/sde/InstallSDE.java
+++ b/test/jdk/com/sun/jdi/sde/InstallSDE.java
@@ -31,9 +31,17 @@
 }

 static void install(File inOutClassFile, File attrFile) throws 
IOException {

-File tmpFile = new File(inOutClassFile.getPath() + "tmp");
-new InstallSDE(inOutClassFile, attrFile, tmpFile);
-if (!inOutClassFile.delete()) {
+File tmpFile = new File(inOutClassFile.getPath() + "tmp-out");
+File tmpInOutClassFile = new File(inOutClassFile.getPath() 
+ "tmp-in");

+
+// Workaround delayed file deletes on Windows using a tmp 
file name

+if (!inOutClassFile.renameTo(tmpInOutClassFile)) {
+throw new IOException("tmp copy of inOutClassFile 
failed");

+}
+
+new InstallSDE(tmpInOutClassFile, attrFile, tmpFile);
+
+if (!tmpInOutClassFile.delete()) {
 throw new IOException("inOutClassFile.delete() failed");
 }
 if (!tmpFile.renameTo(inOutClassFile)) {


Testing in progress ...

On 1/11/19, 7:40 AM, David Holmes wrote:

Hi Gary,

On 11/01/2019 9:22 pm, gary.ad...@oracle.com wrote:
After ~1000 testruns I hit a failure in MangleTest and a jtreg 
agent communication issue with SourceDebugExtension.


https://java.se.oracle.com:10065/mdash/builds/2019-01-10-1159535.gary.adams.jdk-jdb/results?search=status%3Afailed%20AND%20-state%3Ainvalid 




*java.io.IOException: tmpFile.renameTo(inOutClassFile) failed*
at InstallSDE.install(InstallSDE.java:40)
at MangleTest.testSetUp(MangleTest.java:38)
at MangleTest.main(MangleTest.java:31)


You might want to add some code in:

./java.base/windows/native/libjava/WinNTFileSystem_md.c:Java_java_io_WinNTFileSystem_rename0 



that checks the result of _wrename to see what error is occurring. 
I suspect it will be EACCES which won't be that helpful:


https://docs.microsoft.com/en-us/cpp/c-runtime-library/reference/rename-wrename?view=vs-2017 



but it would be good to confirm.



result: Error. Agent communication error: 
java.io.IOException:*Agent: unexpected op: 71;*  check console log 
for any additional details


I added some additional print outs for the rename issue and have 
run ~2000 additional
testruns with no test failures repeated, yet. But there were 2 
mach5 reported errors:


TimeoutException in CLEANUP.


That's a mach5 issue.

David



I'll do some more investigation with making the rename operation 
more robust

before removing SourceDebugExtensionTest from the problem list.

On 1/10/19 8:28 PM, David Holmes wrote:

Hi Gary,

On 10/01/2019 11:02 pm, gary.ad...@oracle.com wrote:

SourceDebugExtensionTest was placed on the ProblemList
because of filesystem rename issues on Windows 2012 test systems.
The test does not appear to fail on the current mach5 test systems.
There was some question concerning symbolic links that might have
been problematic at that time. Additional testing in progress...


Also note that there are two sets of SDE related tests and two 
versions of the InstallSDE.java file that both use renameTo. In 
nsk the testcase seems to be:


./hotspot/jtreg/vmTestbase/vm/mlvm/share/StratumClassesBuilder.java

and AFAICS it is not excluded. So removing the exclusion seems 
reasonable to me. It was most like a concurrency issue with the 
virus scanner.


Thanks,
David



   Issue: https://bugs.openjdk.java.net/browse/JDK-8158066

diff --git a/test/jdk/ProblemList.txt b/test/jdk/ProblemList.txt
--- a/test/jdk/ProblemList.txt
+++ b/test/jdk/ProblemList.txt
@@ -838,8 +838,6 @@

  com/sun/

Re: RFR: JDK-8158066: SourceDebugExtensionTest fails to rename file

[Gary Adams]

Here's a webrev for review purposes.
No failures after ~1000 testruns.

  Webrev: http://cr.openjdk.java.net/~gadams/8158066/webrev.00/
  Issue: https://bugs.openjdk.java.net/browse/JDK-8158066

On 1/11/19, 11:01 AM, Gary Adams wrote:

I've been reading recently that Windows delete file operations
can be delayed, if there are open handles against the file.
Others have reported virus checkers or other indexing
programs may hold a handle on a file preventing the
deletion being completed.

The basic logic in InstallSDE is
   A + B = C
   delete A
   rename C to A

I think we can work around the issue with
  rename A to A1
  A1 + B = C
  delete A1
  rename C to A

diff --git a/test/jdk/com/sun/jdi/sde/InstallSDE.java 
b/test/jdk/com/sun/jdi/sde/InstallSDE.java

--- a/test/jdk/com/sun/jdi/sde/InstallSDE.java
+++ b/test/jdk/com/sun/jdi/sde/InstallSDE.java
@@ -31,9 +31,17 @@
 }

 static void install(File inOutClassFile, File attrFile) throws 
IOException {

-File tmpFile = new File(inOutClassFile.getPath() + "tmp");
-new InstallSDE(inOutClassFile, attrFile, tmpFile);
-if (!inOutClassFile.delete()) {
+File tmpFile = new File(inOutClassFile.getPath() + "tmp-out");
+File tmpInOutClassFile = new File(inOutClassFile.getPath() + 
"tmp-in");

+
+// Workaround delayed file deletes on Windows using a tmp 
file name

+if (!inOutClassFile.renameTo(tmpInOutClassFile)) {
+throw new IOException("tmp copy of inOutClassFile failed");
+}
+
+new InstallSDE(tmpInOutClassFile, attrFile, tmpFile);
+
+if (!tmpInOutClassFile.delete()) {
 throw new IOException("inOutClassFile.delete() failed");
 }
 if (!tmpFile.renameTo(inOutClassFile)) {


Testing in progress ...

On 1/11/19, 7:40 AM, David Holmes wrote:

Hi Gary,

On 11/01/2019 9:22 pm, gary.ad...@oracle.com wrote:
After ~1000 testruns I hit a failure in MangleTest and a jtreg agent 
communication issue with SourceDebugExtension.


https://java.se.oracle.com:10065/mdash/builds/2019-01-10-1159535.gary.adams.jdk-jdb/results?search=status%3Afailed%20AND%20-state%3Ainvalid 




*java.io.IOException: tmpFile.renameTo(inOutClassFile) failed*
at InstallSDE.install(InstallSDE.java:40)
at MangleTest.testSetUp(MangleTest.java:38)
at MangleTest.main(MangleTest.java:31)


You might want to add some code in:

./java.base/windows/native/libjava/WinNTFileSystem_md.c:Java_java_io_WinNTFileSystem_rename0 



that checks the result of _wrename to see what error is occurring. I 
suspect it will be EACCES which won't be that helpful:


https://docs.microsoft.com/en-us/cpp/c-runtime-library/reference/rename-wrename?view=vs-2017 



but it would be good to confirm.



result: Error. Agent communication error: 
java.io.IOException:*Agent: unexpected op: 71;*  check console log 
for any additional details


I added some additional print outs for the rename issue and have run 
~2000 additional
testruns with no test failures repeated, yet. But there were 2 mach5 
reported errors:


TimeoutException in CLEANUP.


That's a mach5 issue.

David



I'll do some more investigation with making the rename operation 
more robust

before removing SourceDebugExtensionTest from the problem list.

On 1/10/19 8:28 PM, David Holmes wrote:

Hi Gary,

On 10/01/2019 11:02 pm, gary.ad...@oracle.com wrote:

SourceDebugExtensionTest was placed on the ProblemList
because of filesystem rename issues on Windows 2012 test systems.
The test does not appear to fail on the current mach5 test systems.
There was some question concerning symbolic links that might have
been problematic at that time. Additional testing in progress...


Also note that there are two sets of SDE related tests and two 
versions of the InstallSDE.java file that both use renameTo. In nsk 
the testcase seems to be:


./hotspot/jtreg/vmTestbase/vm/mlvm/share/StratumClassesBuilder.java

and AFAICS it is not excluded. So removing the exclusion seems 
reasonable to me. It was most like a concurrency issue with the 
virus scanner.


Thanks,
David



   Issue: https://bugs.openjdk.java.net/browse/JDK-8158066

diff --git a/test/jdk/ProblemList.txt b/test/jdk/ProblemList.txt
--- a/test/jdk/ProblemList.txt
+++ b/test/jdk/ProblemList.txt
@@ -838,8 +838,6 @@

  com/sun/jdi/RepStep.java 8043571 generic-all

-com/sun/jdi/sde/SourceDebugExtensionTest.java 8158066 windows-all
-
  com/sun/jdi/NashornPopFrameTest.java 8187143 generic-all

Re: RFR: JDK-8158066: SourceDebugExtensionTest fails to rename file

[Gary Adams]

I've been reading recently that Windows delete file operations
can be delayed, if there are open handles against the file.
Others have reported virus checkers or other indexing
programs may hold a handle on a file preventing the
deletion being completed.

The basic logic in InstallSDE is
   A + B = C
   delete A
   rename C to A

I think we can work around the issue with
  rename A to A1
  A1 + B = C
  delete A1
  rename C to A

diff --git a/test/jdk/com/sun/jdi/sde/InstallSDE.java 
b/test/jdk/com/sun/jdi/sde/InstallSDE.java

--- a/test/jdk/com/sun/jdi/sde/InstallSDE.java
+++ b/test/jdk/com/sun/jdi/sde/InstallSDE.java
@@ -31,9 +31,17 @@
 }

 static void install(File inOutClassFile, File attrFile) throws 
IOException {

-File tmpFile = new File(inOutClassFile.getPath() + "tmp");
-new InstallSDE(inOutClassFile, attrFile, tmpFile);
-if (!inOutClassFile.delete()) {
+File tmpFile = new File(inOutClassFile.getPath() + "tmp-out");
+File tmpInOutClassFile = new File(inOutClassFile.getPath() + 
"tmp-in");

+
+// Workaround delayed file deletes on Windows using a tmp file 
name

+if (!inOutClassFile.renameTo(tmpInOutClassFile)) {
+throw new IOException("tmp copy of inOutClassFile failed");
+}
+
+new InstallSDE(tmpInOutClassFile, attrFile, tmpFile);
+
+if (!tmpInOutClassFile.delete()) {
 throw new IOException("inOutClassFile.delete() failed");
 }
 if (!tmpFile.renameTo(inOutClassFile)) {


Testing in progress ...

On 1/11/19, 7:40 AM, David Holmes wrote:

Hi Gary,

On 11/01/2019 9:22 pm, gary.ad...@oracle.com wrote:
After ~1000 testruns I hit a failure in MangleTest and a jtreg agent 
communication issue with SourceDebugExtension.


https://java.se.oracle.com:10065/mdash/builds/2019-01-10-1159535.gary.adams.jdk-jdb/results?search=status%3Afailed%20AND%20-state%3Ainvalid 




*java.io.IOException: tmpFile.renameTo(inOutClassFile) failed*
at InstallSDE.install(InstallSDE.java:40)
at MangleTest.testSetUp(MangleTest.java:38)
at MangleTest.main(MangleTest.java:31)


You might want to add some code in:

./java.base/windows/native/libjava/WinNTFileSystem_md.c:Java_java_io_WinNTFileSystem_rename0 



that checks the result of _wrename to see what error is occurring. I 
suspect it will be EACCES which won't be that helpful:


https://docs.microsoft.com/en-us/cpp/c-runtime-library/reference/rename-wrename?view=vs-2017 



but it would be good to confirm.



result: Error. Agent communication error: java.io.IOException:*Agent: 
unexpected op: 71;*  check console log for any additional details


I added some additional print outs for the rename issue and have run 
~2000 additional
testruns with no test failures repeated, yet. But there were 2 mach5 
reported errors:


TimeoutException in CLEANUP.


That's a mach5 issue.

David



I'll do some more investigation with making the rename operation more 
robust

before removing SourceDebugExtensionTest from the problem list.

On 1/10/19 8:28 PM, David Holmes wrote:

Hi Gary,

On 10/01/2019 11:02 pm, gary.ad...@oracle.com wrote:

SourceDebugExtensionTest was placed on the ProblemList
because of filesystem rename issues on Windows 2012 test systems.
The test does not appear to fail on the current mach5 test systems.
There was some question concerning symbolic links that might have
been problematic at that time. Additional testing in progress...


Also note that there are two sets of SDE related tests and two 
versions of the InstallSDE.java file that both use renameTo. In nsk 
the testcase seems to be:


./hotspot/jtreg/vmTestbase/vm/mlvm/share/StratumClassesBuilder.java

and AFAICS it is not excluded. So removing the exclusion seems 
reasonable to me. It was most like a concurrency issue with the 
virus scanner.


Thanks,
David



   Issue: https://bugs.openjdk.java.net/browse/JDK-8158066

diff --git a/test/jdk/ProblemList.txt b/test/jdk/ProblemList.txt
--- a/test/jdk/ProblemList.txt
+++ b/test/jdk/ProblemList.txt
@@ -838,8 +838,6 @@

  com/sun/jdi/RepStep.java 8043571 generic-all

-com/sun/jdi/sde/SourceDebugExtensionTest.java 8158066 windows-all
-
  com/sun/jdi/NashornPopFrameTest.java 8187143 generic-all

Re: RFR: JDK-8213001: vmTestbase/nsk/jvmti/ThreadStart/threadstart002/TestDescription.java debug agent times out

[Gary Adams]

I could use another reviewer, or an ok to push as a trivial change.

On 1/8/19, 11:32 AM, Gary Adams wrote:
A number failures of threadstart002 have been reported on 
windows-x64-debug builds.


This configuration has a jtreg timeout factor assigned so the test can 
run 10x slower

and not experience a timeout.

Unfortunately, these older vmTestbase tests were not fully integrated 
with the jtreg

timeout and factor capabilities.

This test in particular uses a 2 second timeout when monitoring 
transitions

between starting a thread and the check for the thread being resumed.
This hard coded timeout is not scaled.

As a quick workaround the timeout can be scaled up to allow 20 second 
window,

as if the scaling factor had been applied as jtreg harness intended.

  Issue: https://bugs.openjdk.java.net/browse/JDK-8213001

diff --git 
a/test/hotspot/jtreg/vmTestbase/nsk/jvmti/ThreadStart/threadstart002/threadstart002.cpp 
b/test/hotspot/jtreg/vmTestbase/nsk/jvmti/ThreadStart/threadstart002/threadstart002.cpp 

--- 
a/test/hotspot/jtreg/vmTestbase/nsk/jvmti/ThreadStart/threadstart002/threadstart002.cpp
+++ 
b/test/hotspot/jtreg/vmTestbase/nsk/jvmti/ThreadStart/threadstart002/threadstart002.cpp

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2003, 2018, Oracle and/or its affiliates. All rights 
reserved.
+ * Copyright (c) 2003, 2019, 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
@@ -34,7 +34,7 @@

 #define PASSED 0
 #define STATUS_FAILED 2
-#define WAIT_TIME 2000
+#define WAIT_TIME 2

 static jvmtiEnv *jvmti = NULL;
 static jvmtiCapabilities caps;

RFR: JDK-8213001: vmTestbase/nsk/jvmti/ThreadStart/threadstart002/TestDescription.java debug agent times out

[Gary Adams]

A number failures of threadstart002 have been reported on 
windows-x64-debug builds.


This configuration has a jtreg timeout factor assigned so the test can 
run 10x slower

and not experience a timeout.

Unfortunately, these older vmTestbase tests were not fully integrated 
with the jtreg

timeout and factor capabilities.

This test in particular uses a 2 second timeout when monitoring transitions
between starting a thread and the check for the thread being resumed.
This hard coded timeout is not scaled.

As a quick workaround the timeout can be scaled up to allow 20 second 
window,

as if the scaling factor had been applied as jtreg harness intended.

  Issue: https://bugs.openjdk.java.net/browse/JDK-8213001

diff --git 
a/test/hotspot/jtreg/vmTestbase/nsk/jvmti/ThreadStart/threadstart002/threadstart002.cpp 
b/test/hotspot/jtreg/vmTestbase/nsk/jvmti/ThreadStart/threadstart002/threadstart002.cpp
--- 
a/test/hotspot/jtreg/vmTestbase/nsk/jvmti/ThreadStart/threadstart002/threadstart002.cpp
+++ 
b/test/hotspot/jtreg/vmTestbase/nsk/jvmti/ThreadStart/threadstart002/threadstart002.cpp

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2003, 2018, Oracle and/or its affiliates. All rights 
reserved.
+ * Copyright (c) 2003, 2019, 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
@@ -34,7 +34,7 @@

 #define PASSED 0
 #define STATUS_FAILED 2
-#define WAIT_TIME 2000
+#define WAIT_TIME 2

 static jvmtiEnv *jvmti = NULL;
 static jvmtiCapabilities caps;

RFR: JDK-8216059: nsk_jvmti_parseoptions still has dependency on tilde separator

[Gary Adams]

The test/hotspot/jtreg/vmTestbase/vm/mlvm tests were missed when the 
parser was updated for

JDK-8211343. Those tests use tilde (~) as an option separator.

Here's a quick fix to add tilde back as an option separator. Testing is 
in progress.


diff --git 
a/test/hotspot/jtreg/vmTestbase/nsk/share/jvmti/jvmti_tools.cpp 
b/test/hotspot/jtreg/vmTestbase/nsk/share/jvmti/jvmti_tools.cpp

--- a/test/hotspot/jtreg/vmTestbase/nsk/share/jvmti/jvmti_tools.cpp
+++ b/test/hotspot/jtreg/vmTestbase/nsk/share/jvmti/jvmti_tools.cpp
@@ -221,7 +221,7 @@
 char *str = NULL;
 char *name = NULL;
 char *value = NULL;
-const char *delimiters = " ,";
+const char *delimiters = " ,~";
 if (options == NULL)
 return success;

Re: RFR(S): 8215975: [testbug] Adapt nsk tests to the PPC, S390 and AIX platforms.

[Gary Adams]

I had no further comments.

On 1/2/19, 10:47 AM, Lindenmaier, Goetz wrote:

Hi Gary,

as promised, I'll do a follow up fixing all these.
But all the tests you mention are either not configured for aix (@requires 
linux etc)
or are passing currently.  So no need for action here.

I just want to do a fix for the nsk tests.
So can I consider my updated webrev as reviewed by you?
http://cr.openjdk.java.net/~goetz/wr18/8215975-nskTests/02/

Best regards,
   Goetz.



-Original Message-
From: gary.ad...@oracle.com
Sent: Montag, 31. Dezember 2018 16:59
To: Lindenmaier, Goetz; serviceability-dev
(serviceability-dev@openjdk.java.net)
Subject: Re: RFR(S): 8215975: [testbug] Adapt nsk tests to the PPC, S390 and
AIX platforms.

Here are few more DYLD_LIBRARY_PATH locations that would be worth
checking

./jdk/vm/JniInvocationTest.java:50: env.compute("DYLD_LIBRARY_PATH", (k,
v) ->  (k == null) ? libdir : v + ":" + libdir);
./jdk/tools/launcher/ExecutionEnvironment.java:66:?
"DYLD_LIBRARY_PATH"
./jdk/tools/launcher/JliLaunchTest.java:56:String
LD_LIBRARY_PATH = Platform.isOSX() ? "DYLD_LIBRARY_PATH" :
"LD_LIBRARY_PATH";
./jdk/sun/security/krb5/auto/BasicProc.java:311:
p.env("DYLD_LIBRARY_PATH", lib.substring(0, pos));
./jdk/sun/security/krb5/auto/ReplayCacheTestProc.java:402:
.env("DYLD_LIBRARY_PATH", libDir)
./jdk/sun/security/krb5/auto/KDC.java:1725: "DYLD_LIBRARY_PATH",
nativePath + "/lib",
./jdk/sun/security/krb5/auto/KDC.java:1818: "DYLD_LIBRARY_PATH",
nativePath + "/lib",
./hotspot/jtreg/vmTestbase/ExecDriver.java:122: name = Platform.isOSX()
? "DYLD_LIBRARY_PATH" : "LD_LIBRARY_PATH";
./hotspot/jtreg/runtime/signal/SigTestDriver.java:72: (Platform.isOSX()
? "DYLD_LIBRARY_PATH" : "LD_LIBRARY_PATH");

On 12/31/18 10:19 AM, Lindenmaier, Goetz wrote:

Hi Gary,


Would it make sense to add a method to

test/lib/jdk/test/lib/Platform.java

similar to sharedLibraryExt() to cover the envName setting?

Good point. I'll post a follow up change next year ... and believe me,
that's not too far in the future 
I'd like to keep this out of this test fix, as I want to downport it to
11 at some point and thus keep it small so that it will apply cleanly.


Are the other places DYLD_LIBRARY_PATH is set also need to be
updated for AIX?

Alloc001 might need it. The other vmTestbase tests are all passing,
so I didn't touch them.
Webrev with alloc001:
http://cr.openjdk.java.net/~goetz/wr18/8215975-nskTests/02/

Best regards,
Goetz.




On 12/31/18 8:48 AM, Lindenmaier, Goetz wrote:

Hi,

Some of the nsk tests are not properly configured for the
above platforms:

nsk/jvmti/RetransformClasses/retransform003/TestDriver.java:


nsk/jvmti/SetNativeMethodPrefix/SetNativeMethodPrefix002/TestDriver.ja

va

The tests use the path to native libraries, which is named "LIBPATH" on

AIX.

nsk/share/jdi/ArgumentHandler.java
Add ppc/s390/aix which don't have a shared memory connector. This

disables a row of failing tests.

Please review this change. I would like to push it to jdk12, as it is a mere

test fix.

http://cr.openjdk.java.net/~goetz/wr18/8215975-

nskTests/01/index.html

Best regards,
 Goetz.

RFR: JDK-8211343: nsk_jvmti_parseoptions should handle multiple suboptions

[Gary Adams]

Here is a first pass at a replacement parser for jvmti test options.

  Webrev: http://cr.openjdk.java.net/~gadams/8211343/webrev.00/
  Issue: https://bugs.openjdk.java.net/browse/JDK-8211343

Testing is in progress. Need to check out more platforms.
On local linux testing one jvmti test failed SetNativeMethodPrefix001
which passed an empty options string.
...

Re: JDK-8211343: nsk_jvmti_parseoptions should handle multiple suboptions

[Gary Adams]

t_end == NSK_JVMTI_OPTION_START) {
//break;
//}
}

if (!add_option(opt, opt_len, val_sep, val_len)) {
success = NSK_FALSE;
break;
}
opt_end++;
opt = opt_end;
}


On 12/20/18 8:33 AM, Gary Adams wrote:

I prototyped with strsep, because strtok is not safe
and did not want to deal with the strtok_r versus
strtok_s (windows) platform variants.

...
#include 
#include 

int  main(int argc, char **argv){
  char *str = strdup("waittime=5,verbose foo bar baz=11");
  char *name;
  char *value;

  while ((name = strsep (, " ,")) != NULL) {
value = index(name, '=');

if (value == NULL) {
  printf ("%s\n", name);
} else {
  *value++ = '\0';
  printf ("%s=%s\n", name, value);
}
  }

...

./main
waittime=5
verbose
foo
bar
baz=11


On 12/20/18, 10:54 AM, JC Beyler wrote:

Hi Gary,

I had seen that too and forgot to file it! So thanks!

I think the comment you removed is interesting, I'm not sure what it 
means exactly and I've tried re-reading a few times but the use of 
"in this question"  is weird :-)


Anyway, the webrev looks good except perhaps for the use of strsep, 
which is BSD, instead of strtok, which is C89/C99/Posix.


Thanks for doing this!
Jc

On Thu, Dec 20, 2018 at 5:17 AM Gary Adams <mailto:gary.ad...@oracle.com>> wrote:


During some earlier jvmti test debugging, I noticed that it was not
possible to
add a quick argument to the current tests and rerun the test. e.g.
expand "waittime=5" to
"waittime=5,verbose". I tracked down the options parsing in
jvmti_tools.cpp and saw some
comments that only a single option could be parsed.

So I filed this bug to revisit the issue for the next release:

   Issue: https://bugs.openjdk.java.net/browse/JDK-8211343

I think the option parsing should be ripped out and redone,
but for now I think a simple tweak to use strsep(), might go a
long way
to solving the multiple option support. I just started testing,
but wanted to know if anyone else has been down this rabbit hole
before.


diff --git
a/test/hotspot/jtreg/vmTestbase/nsk/share/jvmti/jvmti_tools.cpp
b/test/hotspot/jtreg/vmTestbase/nsk/share/jvmti/jvmti_tools.cpp
--- a/test/hotspot/jtreg/vmTestbase/nsk/share/jvmti/jvmti_tools.cpp
+++ b/test/hotspot/jtreg/vmTestbase/nsk/share/jvmti/jvmti_tools.cpp
@@ -196,22 +196,14 @@
  }


-/**
- *
- * The current option will not perform more than one
- * single option which given, this is due to places explained
- * in this question.
- *
- **/
-
   /*
-  * This whole play can be reduced with simple StringTokenizer
(strtok).
-  *
+   * Parse a comma or space separated list of options.
*/

  int nsk_jvmti_parseOptions(const char options[]) {
  size_t len;
  const char* opt;
+char *str = strdup(options);
  int success = NSK_TRUE;

  context.options.string = NULL;
@@ -232,7 +224,7 @@
  context.options.string[len] = '\0';
  context.options.string[len+1] = '\0';

-for (opt = context.options.string; ;) {
+while ((opt = strsep(, " ,")) != NULL) {
  const char* opt_end;
  const char* val_sep;
  int opt_len=0;



--

Thanks,
Jc

Re: JDK-8211343: nsk_jvmti_parseoptions should handle multiple suboptions

[Gary Adams]

I prototyped with strsep, because strtok is not safe
and did not want to deal with the strtok_r versus
strtok_s (windows) platform variants.

...
#include 
#include 

int  main(int argc, char **argv){
  char *str = strdup("waittime=5,verbose foo bar baz=11");
  char *name;
  char *value;

  while ((name = strsep (, " ,")) != NULL) {
value = index(name, '=');

if (value == NULL) {
  printf ("%s\n", name);
} else {
  *value++ = '\0';
  printf ("%s=%s\n", name, value);
}
  }

...

./main
waittime=5
verbose
foo
bar
baz=11


On 12/20/18, 10:54 AM, JC Beyler wrote:

Hi Gary,

I had seen that too and forgot to file it! So thanks!

I think the comment you removed is interesting, I'm not sure what it 
means exactly and I've tried re-reading a few times but the use of "in 
this question"  is weird :-)


Anyway, the webrev looks good except perhaps for the use of strsep, 
which is BSD, instead of strtok, which is C89/C99/Posix.


Thanks for doing this!
Jc

On Thu, Dec 20, 2018 at 5:17 AM Gary Adams <mailto:gary.ad...@oracle.com>> wrote:


During some earlier jvmti test debugging, I noticed that it was not
possible to
add a quick argument to the current tests and rerun the test. e.g.
expand "waittime=5" to
"waittime=5,verbose". I tracked down the options parsing in
jvmti_tools.cpp and saw some
comments that only a single option could be parsed.

So I filed this bug to revisit the issue for the next release:

   Issue: https://bugs.openjdk.java.net/browse/JDK-8211343

I think the option parsing should be ripped out and redone,
but for now I think a simple tweak to use strsep(), might go a
long way
to solving the multiple option support. I just started testing,
but wanted to know if anyone else has been down this rabbit hole
before.


diff --git
a/test/hotspot/jtreg/vmTestbase/nsk/share/jvmti/jvmti_tools.cpp
b/test/hotspot/jtreg/vmTestbase/nsk/share/jvmti/jvmti_tools.cpp
--- a/test/hotspot/jtreg/vmTestbase/nsk/share/jvmti/jvmti_tools.cpp
+++ b/test/hotspot/jtreg/vmTestbase/nsk/share/jvmti/jvmti_tools.cpp
@@ -196,22 +196,14 @@
  }


-/**
- *
- * The current option will not perform more than one
- * single option which given, this is due to places explained
- * in this question.
- *
- **/
-
   /*
-  * This whole play can be reduced with simple StringTokenizer
(strtok).
-  *
+   * Parse a comma or space separated list of options.
*/

  int nsk_jvmti_parseOptions(const char options[]) {
  size_t len;
  const char* opt;
+char *str = strdup(options);
  int success = NSK_TRUE;

  context.options.string = NULL;
@@ -232,7 +224,7 @@
  context.options.string[len] = '\0';
  context.options.string[len+1] = '\0';

-for (opt = context.options.string; ;) {
+while ((opt = strsep(, " ,")) != NULL) {
  const char* opt_end;
  const char* val_sep;
  int opt_len=0;



--

Thanks,
Jc

JDK-8211343: nsk_jvmti_parseoptions should handle multiple suboptions

[Gary Adams]

During some earlier jvmti test debugging, I noticed that it was not 
possible to
add a quick argument to the current tests and rerun the test. e.g. 
expand "waittime=5" to
"waittime=5,verbose". I tracked down the options parsing in 
jvmti_tools.cpp and saw some

comments that only a single option could be parsed.

So I filed this bug to revisit the issue for the next release:

  Issue: https://bugs.openjdk.java.net/browse/JDK-8211343

I think the option parsing should be ripped out and redone,
but for now I think a simple tweak to use strsep(), might go a long way
to solving the multiple option support. I just started testing,
but wanted to know if anyone else has been down this rabbit hole
before.


diff --git 
a/test/hotspot/jtreg/vmTestbase/nsk/share/jvmti/jvmti_tools.cpp 
b/test/hotspot/jtreg/vmTestbase/nsk/share/jvmti/jvmti_tools.cpp

--- a/test/hotspot/jtreg/vmTestbase/nsk/share/jvmti/jvmti_tools.cpp
+++ b/test/hotspot/jtreg/vmTestbase/nsk/share/jvmti/jvmti_tools.cpp
@@ -196,22 +196,14 @@
 }


-/**
- *
- * The current option will not perform more than one
- * single option which given, this is due to places explained
- * in this question.
- *
- **/
-
  /*
-  * This whole play can be reduced with simple StringTokenizer (strtok).
-  *
+   * Parse a comma or space separated list of options.
   */

 int nsk_jvmti_parseOptions(const char options[]) {
 size_t len;
 const char* opt;
+char *str = strdup(options);
 int success = NSK_TRUE;

 context.options.string = NULL;
@@ -232,7 +224,7 @@
 context.options.string[len] = '\0';
 context.options.string[len+1] = '\0';

-for (opt = context.options.string; ;) {
+while ((opt = strsep(, " ,")) != NULL) {
 const char* opt_end;
 const char* val_sep;
 int opt_len=0;

RFR: JDK-8215571: jdb does not include jdk.* in the default class filter

[Gary Adams]

This should be a trivial update.

The default "excludes" filter for jdb
should have been updated when the jdk.* packages were first introduced.


diff --git 
a/src/jdk.jdi/share/classes/com/sun/tools/example/debug/tty/Env.java 
b/src/jdk.jdi/share/classes/com/sun/tools/example/debug/tty/Env.java

--- a/src/jdk.jdi/share/classes/com/sun/tools/example/debug/tty/Env.java
+++ b/src/jdk.jdi/share/classes/com/sun/tools/example/debug/tty/Env.java
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 1998, 2011, 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
@@ -108,7 +108,7 @@

 static private List excludes() {
 if (excludes == null) {
-setExcludes("java.*, javax.*, sun.*, com.sun.*");
+setExcludes("java.*, javax.*, sun.*, com.sun.*, jdk.*");
 }
 return excludes;
 }