JDK-8219143: jdb should support breakpoint thread filters

2019-02-19 Thread Chris Plummer 

Hello,

Please review the following:

http://cr.openjdk.java.net/~cjplummer/8219143/webrev.00/
https://bugs.openjdk.java.net/browse/JDK-8219143

Normally when a breakpoint is set in jdb, it is set globally (all 
threads). JDI supports the ability to have the breakpoint be 
automatically filtered so it will only be delivered on a specified 
thread and ignored on all other threads. This change allows making use 
of that JDI feature from jdb. So instead of something like:


  stop at Foo:23

You can now do:

  stop at threadid 7 Foo:23

Where 7 is the threadID for the thread as seen in the output of the 
"threads" command. The format of the stop command is now:


   stop [go|thread]  [threadid ] 

It is still fully backwards compatible.

As part of this change I also cleaned up the "stop" command parsing and 
error handling. It was kind of a mess, and was near impossible to add 
the "threadid" option until after I did much of the cleanup. I've also 
cleaned up the help output a lot, and added help for the "go" and 
"thread" options. One last change was to remove the distinction between 
"stop at" and "stop in", which was suggested already in a comment. They 
can be used interchangeably now. The changes in Commands.java are pretty 
much all just the above cleanup, except for the one short section with 
the 'Handle "threadid" modifier' comment.


thanks,

Chris

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

2019-02-19 Thread David Holmes 

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

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

2019-02-19 Thread Chris Plummer 

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"

2019-02-19 Thread gary.ad...@oracle.com 

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"

2019-02-19 Thread Chris Plummer 

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]8215622: Add dump to file support for jmap histo

2019-02-19 Thread serguei . spitsyn 

Hi Paul and Lin,

I'm Okay with the fix.

Thanks,
Serguei


On 2/18/19 2:44 PM, Hohensee, Paul wrote:

Looks good. Imo "-histo:" should be valid because it's backward compatible. 
Serguei, if you concur I'll push the patch for Lin.

Thanks,

Paul

On 2/17/19, 7:28 PM, "臧琳"  wrote:

 Dear All,
 Thanks a lot for your reviewing.
 Here I refined the patch to fix a minor bug that cause 
BasicJMapTest fail with jmap -histo:all.
 May I ask your help again to review it? 
http://cr.openjdk.java.net/~xiaofeya/8215622/webrev.09/
 Thanks!
 
 
 Cheers,

 Lin
 
 From: serguei.spit...@oracle.com 
 Sent: Wednesday, February 13, 2019 2:57:15 PM
 To: 臧琳; Hohensee, Paul; Joe Darcy; Daniel Fuchs; David Holmes; JC Beyler
 Cc: serviceability-dev@openjdk.java.net
 Subject: Re: [RFR]8215622: Add dump to file support for jmap histo
 
 Hi Lin,
 
 The update looks good to me.

 The only question I have if the option "-histo:" needs to be valid.
 But I feel it is not that a big problem if it is valid.
 Let's wait for other opinions.
 
 Thanks,

 Serguei
 
 
 On 2/12/19 21:26, 臧琳 wrote:
 
 Dear All,

 I have uploaded new webrev at 
http://cr.openjdk.java.net/~xiaofeya/8215622/webrev.08/
 Here are the changes I made:
 * fixed the grammar issue.
 * fixed the help info of jmap -histo
 * added several unit test in BasicJmapTest.java
Would you like to help review it? Thanks.
 
 
 BRs,

 Lin
 
 From: serguei.spit...@oracle.com 

 Sent: Wednesday, February 13, 2019 4:00
 To: Hohensee, Paul; Joe Darcy; Daniel Fuchs; 臧琳; David Holmes; JC Beyler
 Cc: 
serviceability-dev@openjdk.java.net
 Subject: Re: [RFR]8215622: Add dump to file support for jmap histo
 
 On 2/12/19 10:36 AM, Hohensee, Paul wrote:
 
 
 Thanks, Joe and Daniel. Doesn't seem that we need it for this case anymore, but it's good to know how to proceed in the future. :)
 
 
 
 Agreed.
 
 Thanks,

 Serguei
 
 
 
 
 Paul
 
 On 2/12/19, 9:07 AM, "Joe Darcy"  wrote:
 
  Yes, if you make yourself the assignee, you can change the state of the

  request (and then change the assignee back to the original person).
 
  HTH,
 
  -Joe
 
  On 2/12/2019 7:38 AM, Daniel Fuchs wrote:

  > Hi Paul,
  >
  > On 12/02/2019 16:27, Hohensee, Paul wrote:
  >> I don't see a way to change the Status from Closed to anything 
else.
  >> There's no option I can find to do so.
  >
  > It may be that only the assignee can see this.
  > On the closed CSRs assigned to me I can see a "Back to Draft" button
  > next to "More".
  >
  > Hope this helps,
  >
  > -- daniel

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

2019-02-19 Thread 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/

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

2019-02-19 Thread Chris Plummer 

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"

2019-02-19 Thread 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

2019-02-19 Thread Chris Plummer 
+1. Seemed like a good idea at first, but the potential negative impact 
is just too much.


Chris

On 2/19/19 4:07 AM, gary.ad...@oracle.com wrote:

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

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?

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

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

2019-02-19 Thread gary.ad...@oracle.com 

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

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?

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

Re: [aarch64-port-dev ] RFR: 8209413: AArch64: NPE in clhsdb jstack command

2019-02-19 Thread Andrew Haley 
On 2/19/19 6:21 AM, Nick Gasson (Arm Technology China) wrote:
> How about this:
> 
> http://cr.openjdk.java.net/~ngasson/8209413/webrev.04/
> 
> I changed the two places that currently pass NULL to pass pc() instead 
> and changed the FIXME into an assert. I think the result is much cleaner 
> with no functional change. Ran jtreg with assertions enabled.

That's fine, thanks.

-- 
Andrew Haley
Java Platform Lead Engineer
Red Hat UK Ltd. 
EAC8 43EB D3EF DB98 CC77 2FAD A5CD 6035 332F A671