PING: RFR: 8209790: SA tools not providing option to connect to debug server
Yasumasa Suenaga
yasuenag at gmail.com
Tue Jun 25 23:35:57 UTC 2019
Thanks, Serguei!
Yasumasa
2019年6月25日(火) 17:35 serguei.spitsyn at oracle.com <serguei.spitsyn at 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 <yasuenag at gmail.com>:
> >> Hi David,
> >>
> >> On 2019/06/17 21:42, David Holmes wrote:
> >>> Hi Yasumasa,
> >>>
> >>> On 17/06/2019 6:50 pm, Yasumasa Suenaga wrote:
> >>>> Hi David,
> >>>>
> >>>> 8209790 is filed as a bug.
> >>> I don't agree this is a "bug" - sorry. For this to be a bug there must
> >>> be some specification of behaviour that the implementation is
> violating.
> >>> Is that the case? To me this is missing functionality which makes it an
> >>> enhancement.
> >> The feature for connecting to remote debug server has been provided JDK
> >> 8 or earlier. However it was missed since JDK 9. So I think we can
> >> handle it as a "bug".
> >> Anyway, I added jdk13-enhancement-request label to JBS. I'm waiting for
> >> the approval.
> >>
> >>
> >>>> According to [1], I think we can push the fix to jdk/jdk13. Does it
> >>>> correct?
> >>>>
> >>>> I'm not sure which version (13 or 14) should be set on JBS before
> >>>> pushing.
> >>> Just to clarify the process here, you don't want to set the "Fix
> >>> Version" to 13 or 14 in JBS before pushing. That will be set based on
> >>> the repo you push to. If you push to jdk/jdk it will be 14. If you push
> >>> to jdk/jdk13 it will be 13. Any fix pushed to jdk/jdk13 will be
> >>> "automatically" forward-ported to jdk/jdk and thus 14.
> >> Thanks! I got it.
> >>
> >>
> >> Yasumasa
> >>
> >>
> >>> David
> >>> -----
> >>>
> >>>> (Of course I will push it after the CSR is approved.)
> >>>>
> >>>>
> >>>> Thanks,
> >>>>
> >>>> Yasumasa
> >>>>
> >>>>
> >>>> [1]
> http://mail.openjdk.java.net/pipermail/jdk-dev/2019-June/003051.html
> >>>>
> >>>>
> >>>> On 2019/06/17 17:06, David Holmes wrote:
> >>>>> Hi Yasumasa,
> >>>>>
> >>>>> On 17/06/2019 8:52 am, Yasumasa Suenaga wrote:
> >>>>>> 2019年6月17日(月) 6:47 serguei.spitsyn at oracle.com
> >>>>>> <mailto:serguei.spitsyn at oracle.com> <serguei.spitsyn at oracle.com
> >>>>>> <mailto: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?
> >>>>> JDK 13 has now entered RDP1 so if you want this to go into 13 it will
> >>>>> need special approval:
> >>>>>
> >>>>> http://openjdk.java.net/jeps/3#Late-Enhancement-Request-Process
> >>>>>
> >>>>> Otherwise push to jdk/jdk and it will be for JDK 14.
> >>>>>
> >>>>> Cheers,
> >>>>> David
> >>>>>
> >>>>>> Thanks,
> >>>>>>
> >>>>>> Yasumasa
> >>>>>>
> >>>>>>
> >>>>>>
> >>>>>> Thanks,
> >>>>>> Serguei
> >>>>>>
> >>>>>>
> >>>>>> On 6/16/19 14:44, serguei.spitsyn at oracle.com
> >>>>>> <mailto: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
> >>>>>> <mailto: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
> >>>>>> <mailto: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
> >>>>>> <mailto: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
> >>>>>> <mailto: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 <mailto: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
> >>>>>> <mailto: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 <mailto: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/20190626/5eae50fc/attachment-0001.html>
More information about the serviceability-dev
mailing list