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

classic Classic list List threaded Threaded
52 messages Options
123
Reply | Threaded
Open this post in threaded view
|

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

Yasumasa Suenaga-4
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
Reply | Threaded
Open this post in threaded view
|

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

Yasumasa Suenaga-4
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 <[hidden email]>:

>
> 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
Reply | Threaded
Open this post in threaded view
|

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

Jean Christophe Beyler
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);


   Nit: you have empty lines at l64 and l73

    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 <[hidden email]> 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 <[hidden email]>:
>
> 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
Reply | Threaded
Open this post in threaded view
|

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

Yasumasa Suenaga-4
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 <[hidden email]>:

>
> 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 <[hidden email]> 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 <[hidden email]>:
>> >
>> > 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
Reply | Threaded
Open this post in threaded view
|

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

Yasumasa Suenaga-4
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 <[hidden email]>:

>
> 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 <[hidden email]>:
> >
> > 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 <[hidden email]> 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 <[hidden email]>:
> >> >
> >> > 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
Reply | Threaded
Open this post in threaded view
|

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

Yasumasa Suenaga-4
Sorry, new webrev is here:
  http://cr.openjdk.java.net/~ysuenaga/JDK-8209790/webrev.01/

Yasumasa

2019年6月10日(月) 11:27 Yasumasa Suenaga <[hidden email]>:

>
> 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 <[hidden email]>:
> >
> > 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 <[hidden email]>:
> > >
> > > 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 <[hidden email]> 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 <[hidden email]>:
> > >> >
> > >> > 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
Reply | Threaded
Open this post in threaded view
|

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

serguei.spitsyn@oracle.com
Hi Yasumasa,

I've added myself as a reviewer, so you can finalize it now.

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

Also just an observation that there is some room for option processing refactoring.
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 [hidden email]:
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 [hidden email]:
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 [hidden email]:
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 [hidden email] 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 [hidden email]:
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

Reply | Threaded
Open this post in threaded view
|

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

Yasumasa Suenaga-4
Hi Serguei,

Thank you for your comment!

On 2019/06/15 8:00, [hidden email] 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.


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

IMHO refactoring should be worked on another issue.


If you are OK in above, I will upload new webrev.


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<[hidden email]>:
>>> 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<[hidden email]>:
>>>> 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<[hidden email]>:
>>>>> 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<[hidden email]>  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<[hidden email]>:
>>>>>>> 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
>
Reply | Threaded
Open this post in threaded view
|

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

serguei.spitsyn@oracle.com
Hi Yasumasa,


On 6/14/19 21:11, Yasumasa Suenaga wrote:

> Hi Serguei,
>
> Thank you for your comment!
>
> On 2019/06/15 8:00, [hidden email] 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<[hidden email]>:
>>>> 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<[hidden email]>:
>>>>> 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<[hidden email]>:
>>>>>> 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<[hidden email]>  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<[hidden email]>:
>>>>>>>> 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
>>

Reply | Threaded
Open this post in threaded view
|

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

Yasumasa Suenaga-4
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/


 >> IMHO refactoring should be worked on another issue.
 >
 > Agreed.

I filed it to JBS:
   https://bugs.openjdk.java.net/browse/JDK-8226204


Thanks,

Yasumasa


On 2019/06/15 15:10, [hidden email] 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, [hidden email] 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<[hidden email]>:
>>>>> 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<[hidden email]>:
>>>>>> 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<[hidden email]>:
>>>>>>> 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<[hidden email]>  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<[hidden email]>:
>>>>>>>>> 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
>>>
>
Reply | Threaded
Open this post in threaded view
|

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

serguei.spitsyn@oracle.com
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, [hidden email] 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, [hidden email] 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<[hidden email]>:
>>>>>> 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<[hidden email]>:
>>>>>>> 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<[hidden email]>:
>>>>>>>> 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<[hidden email]>  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<[hidden email]>:
>>>>>>>>>> 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
>>>>
>>

Reply | Threaded
Open this post in threaded view
|

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

serguei.spitsyn@oracle.com
Forgot to tell...
This can be pushed only after the CSR is approved.

Thanks,
Serguei


On 6/16/19 14:44, [hidden email] 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, [hidden email] 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, [hidden email] 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<[hidden email]>:
>>>>>>> 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<[hidden email]>:
>>>>>>>> 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<[hidden email]>:
>>>>>>>>
>>>>>>>>> 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<[hidden email]>  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<[hidden email]>:
>>>>>>>>>>> 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
>>>>>
>>>
>

Reply | Threaded
Open this post in threaded view
|

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

Yasumasa Suenaga-4
2019年6月17日(月) 6:47 [hidden email] <[hidden email]>:
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?


Thanks,

Yasumasa



Thanks,
Serguei


On 6/16/19 14:44, [hidden email] 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, [hidden email] 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, [hidden email] 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<[hidden email]>:
>>>>>>> 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<[hidden email]>:
>>>>>>>> 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<[hidden email]>:
>>>>>>>>
>>>>>>>>> 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<[hidden email]>  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<[hidden email]>:
>>>>>>>>>>> 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
>>>>>
>>>
>

Reply | Threaded
Open this post in threaded view
|

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

David Holmes
Hi Yasumasa,

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

> 2019年6月17日(月) 6:47 [hidden email]
> <mailto:[hidden email]> <[hidden email]
> <mailto:[hidden email]>>:
>
>     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, [hidden email]
>     <mailto:[hidden email]> 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, [hidden email]
>     <mailto:[hidden email]> 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, [hidden email]
>     <mailto:[hidden email]> 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<[hidden email]
>     <mailto:[hidden email]>>:
>      >>>>>>> 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<[hidden email]
>     <mailto:[hidden email]>>:
>      >>>>>>>> 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<[hidden email] <mailto:[hidden email]>>:
>      >>>>>>>>
>      >>>>>>>>> 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<[hidden email] <mailto:[hidden email]>>
>     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<[hidden email] <mailto:[hidden email]>>:
>      >>>>>>>>>>> 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
>      >>>>>
>      >>>
>      >
>
Reply | Threaded
Open this post in threaded view
|

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

Yasumasa Suenaga-4
Hi David,

8209790 is filed as a bug.
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.
(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 [hidden email] <mailto:[hidden email]> <[hidden email] <mailto:[hidden email]>>:
>>
>>     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, [hidden email]
>>     <mailto:[hidden email]> 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, [hidden email]
>>     <mailto:[hidden email]> 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, [hidden email]
>>     <mailto:[hidden email]> 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<[hidden email]
>>     <mailto:[hidden email]>>:
>>      >>>>>>> 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<[hidden email]
>>     <mailto:[hidden email]>>:
>>      >>>>>>>> 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<[hidden email] <mailto:[hidden email]>>:
>>      >>>>>>>>
>>      >>>>>>>>> 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<[hidden email] <mailto:[hidden email]>>     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<[hidden email] <mailto:[hidden email]>>:
>>      >>>>>>>>>>> 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
>>      >>>>>
>>      >>>
>>      >
>>
Reply | Threaded
Open this post in threaded view
|

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

David Holmes
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.

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

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 [hidden email]
>>> <mailto:[hidden email]> <[hidden email]
>>> <mailto:[hidden email]>>:
>>>
>>>     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, [hidden email]
>>>     <mailto:[hidden email]> 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, [hidden email]
>>>     <mailto:[hidden email]> 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, [hidden email]
>>>     <mailto:[hidden email]> 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<[hidden email]
>>>     <mailto:[hidden email]>>:
>>>      >>>>>>> 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<[hidden email]
>>>     <mailto:[hidden email]>>:
>>>      >>>>>>>> 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<[hidden email] <mailto:[hidden email]>>:
>>>      >>>>>>>>
>>>      >>>>>>>>> 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<[hidden email]
>>> <mailto:[hidden email]>>     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<[hidden email] <mailto:[hidden email]>>:
>>>      >>>>>>>>>>> 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
>>>      >>>>>
>>>      >>>
>>>      >
>>>
Reply | Threaded
Open this post in threaded view
|

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

Yasumasa Suenaga-4
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 [hidden email]
>>>> <mailto:[hidden email]> <[hidden email]
>>>> <mailto:[hidden email]>>:
>>>>
>>>>     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, [hidden email]
>>>>     <mailto:[hidden email]> 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, [hidden email]
>>>>     <mailto:[hidden email]> 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, [hidden email]
>>>>     <mailto:[hidden email]> 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<[hidden email]
>>>>     <mailto:[hidden email]>>:
>>>>      >>>>>>> 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<[hidden email]
>>>>     <mailto:[hidden email]>>:
>>>>      >>>>>>>> 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<[hidden email] <mailto:[hidden email]>>:
>>>>      >>>>>>>>
>>>>      >>>>>>>>> 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<[hidden email]
>>>> <mailto:[hidden email]>>     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<[hidden email] <mailto:[hidden email]>>:
>>>>      >>>>>>>>>>> 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
>>>>      >>>>>
>>>>      >>>
>>>>      >
>>>>
Reply | Threaded
Open this post in threaded view
|

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

Yasumasa Suenaga-4
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 <[hidden email]>:

>
> 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 [hidden email]
> >>>> <mailto:[hidden email]> <[hidden email]
> >>>> <mailto:[hidden email]>>:
> >>>>
> >>>>     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, [hidden email]
> >>>>     <mailto:[hidden email]> 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, [hidden email]
> >>>>     <mailto:[hidden email]> 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, [hidden email]
> >>>>     <mailto:[hidden email]> 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<[hidden email]
> >>>>     <mailto:[hidden email]>>:
> >>>>      >>>>>>> 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<[hidden email]
> >>>>     <mailto:[hidden email]>>:
> >>>>      >>>>>>>> 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<[hidden email] <mailto:[hidden email]>>:
> >>>>      >>>>>>>>
> >>>>      >>>>>>>>> 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<[hidden email]
> >>>> <mailto:[hidden email]>>     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<[hidden email] <mailto:[hidden email]>>:
> >>>>      >>>>>>>>>>> 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
> >>>>      >>>>>
> >>>>      >>>
> >>>>      >
> >>>>
Reply | Threaded
Open this post in threaded view
|

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

serguei.spitsyn@oracle.com
Hi Yasumasa,

The fix looks good to me.

Thanks,
Serguei


On 6/25/19 00: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 <[hidden email]>:
>> 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 [hidden email]
>>>>>> <mailto:[hidden email]> <[hidden email]
>>>>>> <mailto:[hidden email]>>:
>>>>>>
>>>>>>      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, [hidden email]
>>>>>>      <mailto:[hidden email]> 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, [hidden email]
>>>>>>      <mailto:[hidden email]> 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, [hidden email]
>>>>>>      <mailto:[hidden email]> 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<[hidden email]
>>>>>>      <mailto:[hidden email]>>:
>>>>>>       >>>>>>> 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<[hidden email]
>>>>>>      <mailto:[hidden email]>>:
>>>>>>       >>>>>>>> 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<[hidden email] <mailto:[hidden email]>>:
>>>>>>       >>>>>>>>
>>>>>>       >>>>>>>>> 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<[hidden email]
>>>>>> <mailto:[hidden email]>>     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<[hidden email] <mailto:[hidden email]>>:
>>>>>>       >>>>>>>>>>> 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
>>>>>>       >>>>>
>>>>>>       >>>
>>>>>>       >
>>>>>>

Reply | Threaded
Open this post in threaded view
|

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

Yasumasa Suenaga-4
Thanks, Serguei!

Yasumasa


2019年6月25日(火) 17:35 [hidden email] <[hidden email]>:
Hi Yasumasa,

The fix looks good to me.

Thanks,
Serguei


On 6/25/19 00: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 <[hidden email]>:
>> 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 [hidden email]
>>>>>> <mailto:[hidden email]> <[hidden email]
>>>>>> <mailto:[hidden email]>>:
>>>>>>
>>>>>>      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, [hidden email]
>>>>>>      <mailto:[hidden email]> 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, [hidden email]
>>>>>>      <mailto:[hidden email]> 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, [hidden email]
>>>>>>      <mailto:[hidden email]> 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<[hidden email]
>>>>>>      <mailto:[hidden email]>>:
>>>>>>       >>>>>>> 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<[hidden email]
>>>>>>      <mailto:[hidden email]>>:
>>>>>>       >>>>>>>> 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<[hidden email] <mailto:[hidden email]>>:
>>>>>>       >>>>>>>>
>>>>>>       >>>>>>>>> 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<[hidden email]
>>>>>> <mailto:[hidden email]>>     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<[hidden email] <mailto:[hidden email]>>:
>>>>>>       >>>>>>>>>>> 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
>>>>>>       >>>>>
>>>>>>       >>>
>>>>>>       >
>>>>>>

123