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