Re: PING: RFR: 8209790: SA tools not providing option to connect to debug server

Chris Plummer Wed, 03 Jul 2019 16:09:02 -0700

Hi Yasumasa,

Overall these changes look good, but I think there is a bit of cleanup still needed.

You've changed the indentation of the help to always have a few spaces before the /t. If you are going to do this, then perhaps you don't need the /t anymore.

There are 3 checks for "remote != null". Shouldn't they be "remote != NO_REMOTE"?

The old jstack help output is a bit more informative than your output:

jstack [-m] [-l] [server_id@]<remote server IP or hostname>
(to connect to a remote debug server)

--connect <[id@]server> To operate on the remote debug server.

More specifically, you replaced <remote server IP or hostname> with "server". I think the former is a bit more self explanatory. Also, why say "to operate on" rather than "to connect to"? The later seems more direct and correct.

I think you should also mention sadebugd in the jhsdb help output for the -connect option, and not just say "debug server". Just change it to "debug server (sadebugd)".

There are also online jhsdb doc updates that we need to make sure are eventually taken care of properly.

https://docs.oracle.com/en/java/javase/11/tools/jhsdb.html

The CSR should trigger the doc update, but you need to make sure the CSR contains all the needed details and instructions for the doc writer. For example, here's what the jstack docs look like:

https://docs.oracle.com/javase/7/docs/technotes/tools/share/jstack.html

jstack [ option ] [server-id@]remote-hostname-or-IP
...
remote-hostname-or-IP
    remote debug server's (see jsadebugd) hostname or IP address.

It includes a reference to jsadebugd (and an actual URL link). This was important for me just in doing this review, because I had to go and figure out what "debug server" was all about, having never dealt with it before. I found it by looking at the old jstack docs and help output. Adding this to the jhsdb docs should be called out in the CSR.

thanks,

Chris

On 7/1/19 3:26 PM, Yasumasa Suenaga wrote:

PING: Could you review it?

   http://cr.openjdk.java.net/~ysuenaga/JDK-8209790/webrev.03/

Yasumasa

On 2019/06/25 16:47, Yasumasa Suenaga wrote:

Hi,

This enhancement has been retargeted to 14, and the CSR has been approved.
I uploaded a webrev for 14. Could you review it?

   http://cr.openjdk.java.net/~ysuenaga/JDK-8209790/webrev.03/

It includes the fix to rename `--remote` to `--connect` that is
suggested in CSR.
It passed tests on submit repo.

Thanks,

Yasumasa

2019年6月17日(月) 22:11 Yasumasa Suenaga <yasue...@gmail.com>:

Hi David,

On 2019/06/17 21:42, David Holmes wrote:

Hi Yasumasa,

On 17/06/2019 6:50 pm, Yasumasa Suenaga wrote:

Hi David,

8209790 is filed as a bug.

I don't agree this is a "bug" - sorry. For this to be a bug there must
be some specification of behaviour that the implementation is violating.
Is that the case? To me this is missing functionality which makes it an
enhancement.

The feature for connecting to remote debug server has been provided JDK
8 or earlier. However it was missed since JDK 9. So I think we can
handle it as a "bug".
Anyway, I added jdk13-enhancement-request label to JBS. I'm waiting for
the approval.

According to [1], I think we can push the fix to jdk/jdk13. Does it
correct?

I'm not sure which version (13 or 14) should be set on JBS before
pushing.

Just to clarify the process here, you don't want to set the "Fix
Version" to 13 or 14 in JBS before pushing. That will be set based on
the repo you push to. If you push to jdk/jdk it will be 14. If you push
to jdk/jdk13 it will be 13. Any fix pushed to jdk/jdk13 will be
"automatically" forward-ported to jdk/jdk and thus 14.

Thanks! I got it.

Yasumasa

David
-----

(Of course I will push it after the CSR is approved.)

Thanks,

Yasumasa

[1] http://mail.openjdk.java.net/pipermail/jdk-dev/2019-June/003051.html

On 2019/06/17 17:06, David Holmes wrote:

Hi Yasumasa,

On 17/06/2019 8:52 am, Yasumasa Suenaga wrote:

2019年6月17日(月) 6:47 serguei.spit...@oracle.com
<mailto:serguei.spit...@oracle.com> <serguei.spit...@oracle.com
<mailto:serguei.spit...@oracle.com>>:

     Forgot to tell...
     This can be pushed only after the CSR is approved.

Sure!
And I will push it when I get second reviewer.

BTW should I push this change to jdk/jdk? or push to jdk/jdk13
manually?

JDK 13 has now entered RDP1 so if you want this to go into 13 it will
need special approval:

http://openjdk.java.net/jeps/3#Late-Enhancement-Request-Process

Otherwise push to jdk/jdk and it will be for JDK 14.

Cheers,
David

Thanks,

Yasumasa

     Thanks,
     Serguei

     On 6/16/19 14:44, serguei.spit...@oracle.com
     <mailto:serguei.spit...@oracle.com> wrote:
      > Hi Yasumasa,
      >
      >
      > On 6/16/19 07:22, Yasumasa Suenaga wrote:
      >> Hi Serguei,
      >>
      >> >>> One minor suggestion is to use the final field NO_REMOTE
instead
      >> >>> of null for initialization of the local variable "remote".
      >>
      >> I fixed it on new webrev. Could you check again?
      >> http://cr.openjdk.java.net/~ysuenaga/JDK-8209790/webrev.02/
      >
      >
      > It looks good.
      > Thanks you for the update!
      >
      >>
      >> >> IMHO refactoring should be worked on another issue.
      >> >
      >> > Agreed.
      >>
      >> I filed it to JBS:
      >> https://bugs.openjdk.java.net/browse/JDK-8226204
      >
      > Thank you for filing the enhancement!
      >
      > Thanks.
      > Serguei
      >
      >
      >> Thanks,
      >>
      >> Yasumasa
      >>
      >>
      >> On 2019/06/15 15:10, serguei.spit...@oracle.com
     <mailto:serguei.spit...@oracle.com> wrote:
      >>> Hi Yasumasa,
      >>>
      >>>
      >>> On 6/14/19 21:11, Yasumasa Suenaga wrote:
      >>>> Hi Serguei,
      >>>>
      >>>> Thank you for your comment!
      >>>>
      >>>> On 2019/06/15 8:00, serguei.spit...@oracle.com
     <mailto:serguei.spit...@oracle.com> wrote:
      >>>>> Hi Yasumasa,
      >>>>>
      >>>>> I've added myself as a reviewer, so you can finalize it now.
      >>>>
      >>>> I moved CSR to Finalized, and added a comment for your
question.
      >>>
      >>> Okay, thanks!
      >>>
      >>>
      >>>>> The fix looks pretty good to me.
      >>>>>
      >>>>> One minor suggestion is to use the final field NO_REMOTE
instead
      >>>>> of null for initialization of the local variable "remote".
      >>>>
      >>>> I will fix that.
      >>>>
      >>>>
      >>>>> Also just an observation that there is some room for option
      >>>>> processing refactoring.
      >>>>
      >>>> Your suggestion handles all options in one parser method.
      >>>> I concern it might be complex for option validation.
      >>>> (e.g. `jmap -heap` is allowed, but `jstack -heap` is not
allowed.)
      >>>
      >>> This concern is not valid as the list allowed options allowed
     for each
      >>> jhsdb sub-command is controlled with the longOpts argument.
      >>>
      >>> jmap has:
      >>>           String[] longOpts = {"exe=", "core=", "pid=",
"remote=",
      >>>                 "heap", "binaryheap", "dumpfile=", "histo",
      >>> "clstats", "finalizerinfo"};
      >>>
      >>> but jstack has:
      >>>           String[] longOpts = {"exe=", "core=", "pid=",
"remote=",
      >>> "mixed", "locks"};
      >>>
      >>>> IMHO refactoring should be worked on another issue.
      >>>
      >>> Agreed.
      >>>
      >>>
      >>>> If you are OK in above, I will upload new webrev.
      >>>
      >>> Yes, I'm Okay with it.
      >>>
      >>>
      >>> Thanks,
      >>> Serguei
      >>>
      >>>
      >>>> Thanks,
      >>>>
      >>>> Yasumasa
      >>>>
      >>>>
      >>>>> All the jhsdb sub-commands do execute similar loops like
this:
      >>>>> while((s = sg.next(null, longOpts)) != null) {
      >>>>>          . . .
      >>>>>      }
      >>>>>
      >>>>> It can be moved into a separate method instead.
      >>>>> The longOpts can passed in arguments.
      >>>>>
      >>>>> It can be something like this:
      >>>>>
      >>>>>      private ArrayList<String> processOptions(final String[]
     oldArgs,
      >>>>>                                               final String[]
      >>>>> longOpts,
      >>>>>                                               boolean
     allowEmpty) {
      >>>>>          SAGetopt sg = new SAGetopt(oldArgs);
      >>>>>          ArrayList<String> newArgs = new ArrayList();
      >>>>>
      >>>>>          String pid = null;
      >>>>>          String exe = null;
      >>>>>          String core = null;
      >>>>>          String s = null;
      >>>>>          String dumpfile = null;
      >>>>>          String remote = NO_REMOTE;
      >>>>>          boolean requestHeapdump = false;
      >>>>>
      >>>>>          while((s = sg.next(null, longOpts)) != null) {
      >>>>>              if (s.equals("exe")) {
      >>>>>                  exe = sg.getOptarg();
      >>>>>                  continue;
      >>>>>              }
      >>>>>              if (s.equals("core")) {
      >>>>>                  core = sg.getOptarg();
      >>>>>                  continue;
      >>>>>              }
      >>>>>              if (s.equals("pid")) {
      >>>>>                   pid = sg.getOptarg();
      >>>>>                   continue;
      >>>>>              }
      >>>>>              if (s.equals("remote")) {
      >>>>>                  remote = sg.getOptarg();
      >>>>>                  continue;
      >>>>>              }
      >>>>>              if (s.equals("mixed")) {
      >>>>>                  newArgs.add("-m");
      >>>>>                  continue;
      >>>>>              }
      >>>>>              if (s.equals("locks")) {
      >>>>>                  newArgs.add("-l");
      >>>>>                  continue;
      >>>>>              }
      >>>>>              if (s.equals("heap")) {
      >>>>>                  newArgs.add("-heap");
      >>>>>                  continue;
      >>>>>              }
      >>>>>              if (s.equals("binaryheap")) {
      >>>>>                  requestHeapdump = true;
      >>>>>                  continue;
      >>>>>              }
      >>>>>              if (s.equals("dumpfile")) {
      >>>>>                  dumpfile = sg.getOptarg();
      >>>>>                  continue;
      >>>>>              }
      >>>>>              if (s.equals("histo")) {
      >>>>>                  newArgs.add("-histo");
      >>>>>                  continue;
      >>>>>              }
      >>>>>              if (s.equals("clstats")) {
      >>>>>                  newArgs.add("-clstats");
      >>>>>                   continue;
      >>>>>              }
      >>>>>              if (s.equals("finalizerinfo")) {
      >>>>>                  newArgs.add("-finalizerinfo");
      >>>>>                  continue;
      >>>>>              }
      >>>>>              if (s.equals("flags")) {
      >>>>>                  newArgs.add("-flags");
      >>>>>                  continue;
      >>>>>              }
      >>>>>              if (s.equals("sysprops")) {
      >>>>>                  newArgs.add("-sysprops");
      >>>>>                  continue;
      >>>>>              }
      >>>>>              if (s.equals("serverid")) {
      >>>>>                  String serverid = sg.getOptarg();
      >>>>>                  if (serverid != null) {
      >>>>>                      newArgs.add(serverid);
      >>>>>                  }
      >>>>>                  continue;
      >>>>>              }
      >>>>>          }
      >>>>>
      >>>>>          if (!requestHeapdump && (dumpfile != null)) {
      >>>>>              throw new IllegalArgumentException("Unexpected
      >>>>> argument dumpfile");
      >>>>>          }
      >>>>>          if (requestHeapdump) {
      >>>>>              if (dumpfile == null) {
      >>>>>                  newArgs.add("-heap:format=b");
      >>>>>              } else {
      >>>>>                   newArgs.add("-heap:format=b,file=" +
dumpfile);
      >>>>>              }
      >>>>>          }
      >>>>>          buildAttachArgs(newArgs, pid, exe, core, remote,
      >>>>> allowEmpty);
      >>>>>
      >>>>>          return newArgs;
      >>>>>      }
      >>>>>
      >>>>>      private static void runCLHSDB(String[] oldArgs) {
      >>>>>          String[] longOpts = {"exe=", "core=", "pid="};
      >>>>>          ArrayList<String> newArgs = processOptions(oldArgs,
      >>>>> longOpts, true);
      >>>>>
      >>>>>          CLHSDB.main(newArgs.toArray(new
     String[newArgs.size()]));
      >>>>>      }
      >>>>>
      >>>>>      private static void runHSDB(String[] oldArgs) {
      >>>>>          String[] longOpts = {"exe=", "core=", "pid="};
      >>>>>           ArrayList<String> newArgs =
processOptions(oldArgs,
      >>>>> longOpts, true);
      >>>>>
      >>>>>           HSDB.main(newArgs.toArray(new
String[newArgs.size()]));
      >>>>>      }
      >>>>>
      >>>>>      private static void runJSTACK(String[] oldArgs) {
      >>>>>          String[] longOpts = {"exe=", "core=", "pid=",
     "remote=",
      >>>>> "mixed", "locks"};
      >>>>>          ArrayList<String> newArgs = processOptions(oldArgs,
      >>>>> longOpts, false);
      >>>>>
      >>>>>          JStack jstack = new JStack(false, false);
      >>>>>          jstack.runWithArgs(newArgs.toArray(new
      >>>>> String[newArgs.size()]));
      >>>>>      }
      >>>>>
      >>>>>      private static void runJMAP(String[] oldArgs) {
      >>>>>          String[] longOpts = {"exe=", "core=", "pid=",
"remote=",
      >>>>>                "heap", "binaryheap", "dumpfile=", "histo",
      >>>>> "clstats", "finalizerinfo"};
      >>>>>
      >>>>>          ArrayList<String> newArgs = processOptions(oldArgs,
      >>>>> longOpts, false);
      >>>>>
      >>>>>          JMap.main(newArgs.toArray(new
String[newArgs.size()]));
      >>>>>      }
      >>>>>
      >>>>>      private static void runJINFO(String[] oldArgs) {
      >>>>>          String[] longOpts = {"exe=", "core=", "pid=",
     "remote=",
      >>>>> "flags", "sysprops"};
      >>>>>          ArrayList<String> newArgs = processOptions(oldArgs,
      >>>>> longOpts, false);
      >>>>>
      >>>>>          JInfo.main(newArgs.toArray(new
String[newArgs.size()]));
      >>>>>      }
      >>>>>
      >>>>>      private static void runJSNAP(String[] oldArgs) {
      >>>>>          String[] longOpts = {"exe=", "core=", "pid=",
     "remote=",
      >>>>> "all"};
      >>>>>          ArrayList<String> newArgs = processOptions(oldArgs,
      >>>>> longOpts, false);
      >>>>>          JSnap.main(newArgs.toArray(new
String[newArgs.size()]));
      >>>>>      }
      >>>>>
      >>>>>      private static void runDEBUGD(String[] oldArgs) {
      >>>>>          // By default SA agent classes prefer Windows
process
      >>>>> debugger
      >>>>>          // to windbg debugger. SA expects special
properties to
      >>>>> be set
      >>>>>          // to choose other debuggers. We will set those
here
     before
      >>>>>          // attaching to SA agent.
      >>>>>
System.setProperty("sun.jvm.hotspot.debugger.useWindbgDebugger",
      >>>>> "true");
      >>>>>
      >>>>>          String[] longOpts = {"exe=", "core=", "pid=",
     "serverid="};
      >>>>>          ArrayList<String> newArgs = processOptions(oldArgs,
      >>>>> longOpts, false);
      >>>>>
      >>>>>          // delegate to the actual SA debug server.
      >>>>> sun.jvm.hotspot.DebugServer.main(newArgs.toArray(new
      >>>>> String[newArgs.size()]));
      >>>>>      }
      >>>>>
      >>>>>
      >>>>> Please, let me know what do you think.
      >>>>>
      >>>>> Thanks,
      >>>>> Serguei
      >>>>>
      >>>>>
      >>>>> On 6/9/19 7:29 PM, Yasumasa Suenaga wrote:
      >>>>>> Sorry, new webrev is here:
      >>>>>> http://cr.openjdk.java.net/~ysuenaga/JDK-8209790/webrev.01/
      >>>>>>
      >>>>>> Yasumasa
      >>>>>>
      >>>>>> 2019年6月10日(月) 11:27 Yasumasa Suenaga<yasue...@gmail.com
     <mailto:yasue...@gmail.com>>:
      >>>>>>> PING: Could you review them?
      >>>>>>>
      >>>>>>>>>>> JBS:https://bugs.openjdk.java.net/browse/JDK-8209790
      >>>>>>>>>>> CSR:https://bugs.openjdk.java.net/browse/JDK-8224979
      >>>>>>>>>>>
     webrev:http://cr.openjdk.java.net/~ysuenaga/JDK-8209790/webrev.00/
      >>>>>>>>>>>
      >>>>>>> It is P3 bug, but I want to fix it before JDK 13 RDP 1 if
     possible.
      >>>>>>>
      >>>>>>>
      >>>>>>> Thanks,
      >>>>>>>
      >>>>>>> Yasumasa
      >>>>>>>
      >>>>>>>
      >>>>>>> 2019年6月5日(水) 14:06 Yasumasa Suenaga<yasue...@gmail.com
     <mailto:yasue...@gmail.com>>:
      >>>>>>>> Hi Jc,
      >>>>>>>>
      >>>>>>>> Thank you for your comment!
      >>>>>>>> I updated a webrev:
      >>>>>>>>
      >>>>>>>>
http://cr.openjdk.java.net/~ysuenaga/JDK-8209790/webrev.01/
      >>>>>>>>
      >>>>>>>>>     - In runTests; if DebugdUtils implemented
Closeable, you
      >>>>>>>>> could just do a try-with-resources instead of the
finally
      >>>>>>>>> clause...
      >>>>>>>> I created DebugdUtils for convenience class for attach -
     detach
      >>>>>>>> mechanism of debug server.
      >>>>>>>> IMHO it is prefer "detach" to "close" in this case.
      >>>>>>>>
      >>>>>>>>
      >>>>>>>> Thanks,
      >>>>>>>>
      >>>>>>>> Yasumasa
      >>>>>>>>
      >>>>>>>>
      >>>>>>>> 2019年6月5日(水) 11:34 Jean Christophe
     Beyler<jcbey...@google.com <mailto:jcbey...@google.com>>:
      >>>>>>>>
      >>>>>>>>> Hi Yasumasa,
      >>>>>>>>>
      >>>>>>>>> I'm not an official reviewer but I don't see an issue
     with the
      >>>>>>>>> CSR (except that this seems to be bringing a fork in the
     tools
      >>>>>>>>> with some handling remote and others not).
      >>>>>>>>>
      >>>>>>>>> However, this code is really repetitive and this is
not the
      >>>>>>>>> place to do a big refactor probably but we could do a
few
     nits
      >>>>>>>>> perhaps:
      >>>>>>>>>     - Instead of every tool calling commonHelp with an
      >>>>>>>>> additional flag you could divide into commonHelp and
      >>>>>>>>> commonHelpWithRemote for the tools and they both call
the
      >>>>>>>>> current commonHelp with that boolean; so that when we
are
      >>>>>>>>> looking at the tool code we know what we are
getting... So
      >>>>>>>>> something like:
      >>>>>>>>>
      >>>>>>>>> private static boolean commonHelp(String mode, boolean
      >>>>>>>>> canConnectToRemote) {
      >>>>>>>>>      ..
      >>>>>>>>>   }
      >>>>>>>>>
      >>>>>>>>> private static boolean commonHelp(String mode) {
      >>>>>>>>>     return commonHelp(mode, false);
      >>>>>>>>> }
      >>>>>>>>>
      >>>>>>>>> private static boolean commonHelpWithRemote(String
mode) {
      >>>>>>>>>     return commonHelp(mode, false);
      >>>>>>>>> }
      >>>>>>>>>
      >>>>>>>>> and that way the tools that change are just going from:
      >>>>>>>>> -        return commonHelp("jmap");
      >>>>>>>>> +        return commonHelpWithRemote("jmap");
      >>>>>>>>>
      >>>>>>>>> - In the same vein, instead of passing null to the
      >>>>>>>>> buildAttachArgs; you could make a variable null:
      >>>>>>>>>
      >>>>>>>>> -        buildAttachArgs(newArgs, pid, exe, core, true);
      >>>>>>>>> +        String noRemote = null;
      >>>>>>>>> +        buildAttachArgs(newArgs, pid, exe, core,
noRemote,
      >>>>>>>>> true);
      >>>>>>>>>
      >>>>>>>>>
      >>>>>>>>>
-http://cr.openjdk.java.net/~ysuenaga/JDK-8209790/webrev.00/test/hotspot/jtreg/serviceability/sa/sadebugd/DebugdUtils.java.html

      >>>>>>>>>
      >>>>>>>>>     Nit: you have empty lines at l64 and l73
      >>>>>>>>>
      >>>>>>>>>
-http://cr.openjdk.java.net/~ysuenaga/JDK-8209790/webrev.00/test/hotspot/jtreg/serviceability/sa/sadebugd/DebugdConnectTest.java.html

      >>>>>>>>>
      >>>>>>>>>      Nit : you have an empty line at l110
      >>>>>>>>>
      >>>>>>>>>     - In runTests; if DebugdUtils implemented
Closeable, you
      >>>>>>>>> could just do a try-with-resources instead of the
finally
      >>>>>>>>> clause...
      >>>>>>>>>
      >>>>>>>>> All of these are details, I just thought I'd mention
them :)
      >>>>>>>>> Jc
      >>>>>>>>>
      >>>>>>>>> On Tue, Jun 4, 2019 at 6:44 PM Yasumasa
      >>>>>>>>> Suenaga<yasue...@gmail.com
<mailto:yasue...@gmail.com>>     wrote:
      >>>>>>>>>> PING: Could you review them?
      >>>>>>>>>>
      >>>>>>>>>>> JBS:https://bugs.openjdk.java.net/browse/JDK-8209790
      >>>>>>>>>>> CSR:https://bugs.openjdk.java.net/browse/JDK-8224979
      >>>>>>>>>>>
     webrev:http://cr.openjdk.java.net/~ysuenaga/JDK-8209790/webrev.00/
      >>>>>>>>>>>
      >>>>>>>>>> CSR status is provisional. So I need reviewers both
CSR and
      >>>>>>>>>> webrev.
      >>>>>>>>>>
      >>>>>>>>>>
      >>>>>>>>>> Thanks,
      >>>>>>>>>>
      >>>>>>>>>> Yasumasa
      >>>>>>>>>>
      >>>>>>>>>>
      >>>>>>>>>> 2019年5月29日(水) 22:37 Yasumasa
     Suenaga<yasue...@gmail.com <mailto:yasue...@gmail.com>>:
      >>>>>>>>>>> Hi all,
      >>>>>>>>>>>
      >>>>>>>>>>> Please review this change:
      >>>>>>>>>>>
      >>>>>>>>>>> JBS:https://bugs.openjdk.java.net/browse/JDK-8209790
      >>>>>>>>>>> CSR:https://bugs.openjdk.java.net/browse/JDK-8224979
      >>>>>>>>>>>
     webrev:http://cr.openjdk.java.net/~ysuenaga/JDK-8209790/webrev.00/
      >>>>>>>>>>>
      >>>>>>>>>>>
      >>>>>>>>>>> In JDK 8 or earlier, some tools (jstack, jmap,
jinfo) can
      >>>>>>>>>>> connect to
      >>>>>>>>>>> debug server (jsadebugd). However it has not done
so since
      >>>>>>>>>>> JDK 9 because
      >>>>>>>>>>> jhsdb cannot accept the attach request to debug
server.
      >>>>>>>>>>> So I want to introduce new option `--remote` to
connect to
      >>>>>>>>>>> debug server.
      >>>>>>>>>>>
      >>>>>>>>>>> I created CSR for this issue. So please review it
together.
      >>>>>>>>>>>
      >>>>>>>>>>>
      >>>>>>>>>>> Thanks,
      >>>>>>>>>>>
      >>>>>>>>>>> Yasumasa
      >>>>>>>>>
      >>>>>>>>> --
      >>>>>>>>>
      >>>>>>>>> Thanks,
      >>>>>>>>> Jc
      >>>>>
      >>>
      >

Re: PING: RFR: 8209790: SA tools not providing option to connect to debug server

Reply via email to