[prev in list] [next in list] [prev in thread] [next in thread]
List: openjdk-serviceability-dev
Subject: Re: PING: RFR: 8209790: SA tools not providing option to connect to debug server
From: Yasumasa Suenaga <yasuenag () gmail ! com>
Date: 2019-06-25 23:35:57
Message-ID: CAGFVN2Amqk5hHenUhX2LF45_Z7qexYK-zRFU1+MY7CJdHZ+NSg () mail ! gmail ! com
[Download RAW message or body]
Thanks, Serguei!
Yasumasa
2019年6月25日(火) 17:35 serguei.spitsyn@oracle.com <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 <yasuenag@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@oracle.com
> > > > > > > <mailto:serguei.spitsyn@oracle.com> <serguei.spitsyn@oracle.com
> > > > > > > <mailto:serguei.spitsyn@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@oracle.com
> > > > > > > <mailto:serguei.spitsyn@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@oracle.com
> > > > > > > <mailto:serguei.spitsyn@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@oracle.com
> > > > > > > <mailto:serguei.spitsyn@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@gmail.com
> > > > > > > <mailto:yasuenag@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@gmail.com
> > > > > > > <mailto:yasuenag@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@google.com <mailto:jcbeyler@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@gmail.com
> > > > > > > <mailto:yasuenag@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@gmail.com <mailto:yasuenag@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
> > > > > > > > > > > >
> > > > > > > > > >
> > > > > > > >
> > > > > > >
>
>
[Attachment #3 (text/html)]
<div dir="auto">Thanks, Serguei!<div dir="auto"><br></div><div \
dir="auto">Yasumasa</div><div dir="auto"><br></div></div><br><div \
class="gmail_quote"><div dir="ltr" class="gmail_attr">2019年6月25日(火) 17:35 <a \
href="mailto:serguei.spitsyn@oracle.com">serguei.spitsyn@oracle.com</a> <<a \
href="mailto:serguei.spitsyn@oracle.com">serguei.spitsyn@oracle.com</a>>:<br></div><blockquote \
class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc \
solid;padding-left:1ex">Hi Yasumasa,<br> <br>
The fix looks good to me.<br>
<br>
Thanks,<br>
Serguei<br>
<br>
<br>
On 6/25/19 00:47, Yasumasa Suenaga wrote:<br>
> Hi,<br>
><br>
> This enhancement has been retargeted to 14, and the CSR has been approved.<br>
> I uploaded a webrev for 14. Could you review it?<br>
><br>
> <a href="http://cr.openjdk.java.net/~ysuenaga/JDK-8209790/webrev.03/" \
rel="noreferrer noreferrer" \
target="_blank">http://cr.openjdk.java.net/~ysuenaga/JDK-8209790/webrev.03/</a><br> \
><br> > It includes the fix to rename `--remote` to `--connect` that is<br>
> suggested in CSR.<br>
> It passed tests on submit repo.<br>
><br>
><br>
> Thanks,<br>
><br>
> Yasumasa<br>
><br>
><br>
> 2019年6月17日(月) 22:11 Yasumasa Suenaga <<a \
href="mailto:yasuenag@gmail.com" target="_blank" \
rel="noreferrer">yasuenag@gmail.com</a>>:<br> >> Hi David,<br>
>><br>
>> On 2019/06/17 21:42, David Holmes wrote:<br>
>>> Hi Yasumasa,<br>
>>><br>
>>> On 17/06/2019 6:50 pm, Yasumasa Suenaga wrote:<br>
>>>> Hi David,<br>
>>>><br>
>>>> 8209790 is filed as a bug.<br>
>>> I don't agree this is a "bug" - sorry. For this to be a \
bug there must<br> >>> be some specification of behaviour that the \
implementation is violating.<br> >>> Is that the case? To me this is missing \
functionality which makes it an<br> >>> enhancement.<br>
>> The feature for connecting to remote debug server has been provided JDK<br>
>> 8 or earlier. However it was missed since JDK 9. So I think we can<br>
>> handle it as a "bug".<br>
>> Anyway, I added jdk13-enhancement-request label to JBS. I'm waiting \
for<br> >> the approval.<br>
>><br>
>><br>
>>>> According to [1], I think we can push the fix to jdk/jdk13. Does \
it<br> >>>> correct?<br>
>>>><br>
>>>> I'm not sure which version (13 or 14) should be set on JBS \
before<br> >>>> pushing.<br>
>>> Just to clarify the process here, you don't want to set the \
"Fix<br> >>> Version" to 13 or 14 in JBS before pushing. That will \
be set based on<br> >>> the repo you push to. If you push to jdk/jdk it will \
be 14. If you push<br> >>> to jdk/jdk13 it will be 13. Any fix pushed to \
jdk/jdk13 will be<br> >>> "automatically" forward-ported to \
jdk/jdk and thus 14.<br> >> Thanks! I got it.<br>
>><br>
>><br>
>> Yasumasa<br>
>><br>
>><br>
>>> David<br>
>>> -----<br>
>>><br>
>>>> (Of course I will push it after the CSR is approved.)<br>
>>>><br>
>>>><br>
>>>> Thanks,<br>
>>>><br>
>>>> Yasumasa<br>
>>>><br>
>>>><br>
>>>> [1] <a \
href="http://mail.openjdk.java.net/pipermail/jdk-dev/2019-June/003051.html" \
rel="noreferrer noreferrer" \
target="_blank">http://mail.openjdk.java.net/pipermail/jdk-dev/2019-June/003051.html</a><br>
>>>><br>
>>>><br>
>>>> On 2019/06/17 17:06, David Holmes wrote:<br>
>>>>> Hi Yasumasa,<br>
>>>>><br>
>>>>> On 17/06/2019 8:52 am, Yasumasa Suenaga wrote:<br>
>>>>>> 2019年6月17日(月) 6:47 <a \
href="mailto:serguei.spitsyn@oracle.com" target="_blank" \
rel="noreferrer">serguei.spitsyn@oracle.com</a><br> >>>>>> \
<mailto:<a href="mailto:serguei.spitsyn@oracle.com" target="_blank" \
rel="noreferrer">serguei.spitsyn@oracle.com</a>> <<a \
href="mailto:serguei.spitsyn@oracle.com" target="_blank" \
rel="noreferrer">serguei.spitsyn@oracle.com</a><br> >>>>>> \
<mailto:<a href="mailto:serguei.spitsyn@oracle.com" target="_blank" \
rel="noreferrer">serguei.spitsyn@oracle.com</a>>>:<br> \
>>>>>><br> >>>>>> Forgot to tell...<br>
>>>>>> This can be pushed only after the CSR is \
approved.<br> >>>>>><br>
>>>>>><br>
>>>>>> Sure!<br>
>>>>>> And I will push it when I get second reviewer.<br>
>>>>>><br>
>>>>>> BTW should I push this change to jdk/jdk? or push to \
jdk/jdk13<br> >>>>>> manually?<br>
>>>>> JDK 13 has now entered RDP1 so if you want this to go into 13 it \
will<br> >>>>> need special approval:<br>
>>>>><br>
>>>>> <a \
href="http://openjdk.java.net/jeps/3#Late-Enhancement-Request-Process" \
rel="noreferrer noreferrer" \
target="_blank">http://openjdk.java.net/jeps/3#Late-Enhancement-Request-Process</a><br>
>>>>><br>
>>>>> Otherwise push to jdk/jdk and it will be for JDK 14.<br>
>>>>><br>
>>>>> Cheers,<br>
>>>>> David<br>
>>>>><br>
>>>>>> Thanks,<br>
>>>>>><br>
>>>>>> Yasumasa<br>
>>>>>><br>
>>>>>><br>
>>>>>><br>
>>>>>> Thanks,<br>
>>>>>> Serguei<br>
>>>>>><br>
>>>>>><br>
>>>>>> On 6/16/19 14:44, <a \
href="mailto:serguei.spitsyn@oracle.com" target="_blank" \
rel="noreferrer">serguei.spitsyn@oracle.com</a><br> >>>>>> \
<mailto:<a href="mailto:serguei.spitsyn@oracle.com" target="_blank" \
rel="noreferrer">serguei.spitsyn@oracle.com</a>> wrote:<br> \
>>>>>> > Hi Yasumasa,<br> >>>>>> \
><br> >>>>>> ><br>
>>>>>> > On 6/16/19 07:22, Yasumasa Suenaga wrote:<br>
>>>>>> >> Hi Serguei,<br>
>>>>>> >><br>
>>>>>> >> >>> One minor suggestion is to \
use the final field NO_REMOTE<br> >>>>>> instead<br>
>>>>>> >> >>> of null for initialization \
of the local variable "remote".<br> >>>>>> \
>><br> >>>>>> >> I fixed it on new webrev. \
Could you check again?<br> >>>>>> >> <a \
href="http://cr.openjdk.java.net/~ysuenaga/JDK-8209790/webrev.02/" rel="noreferrer \
noreferrer" target="_blank">http://cr.openjdk.java.net/~ysuenaga/JDK-8209790/webrev.02/</a><br>
>>>>>> ><br>
>>>>>> ><br>
>>>>>> > It looks good.<br>
>>>>>> > Thanks you for the update!<br>
>>>>>> ><br>
>>>>>> >><br>
>>>>>> >> >> IMHO refactoring should be \
worked on another issue.<br> >>>>>> >> ><br>
>>>>>> >> > Agreed.<br>
>>>>>> >><br>
>>>>>> >> I filed it to JBS:<br>
>>>>>> >> <a \
href="https://bugs.openjdk.java.net/browse/JDK-8226204" rel="noreferrer noreferrer" \
target="_blank">https://bugs.openjdk.java.net/browse/JDK-8226204</a><br> \
>>>>>> ><br> >>>>>> > \
Thank you for filing the enhancement!<br> >>>>>> ><br>
>>>>>> > Thanks.<br>
>>>>>> > Serguei<br>
>>>>>> ><br>
>>>>>> ><br>
>>>>>> >> Thanks,<br>
>>>>>> >><br>
>>>>>> >> Yasumasa<br>
>>>>>> >><br>
>>>>>> >><br>
>>>>>> >> On 2019/06/15 15:10, <a \
href="mailto:serguei.spitsyn@oracle.com" target="_blank" \
rel="noreferrer">serguei.spitsyn@oracle.com</a><br> >>>>>> \
<mailto:<a href="mailto:serguei.spitsyn@oracle.com" target="_blank" \
rel="noreferrer">serguei.spitsyn@oracle.com</a>> wrote:<br> \
>>>>>> >>> Hi Yasumasa,<br> \
>>>>>> >>><br> >>>>>> \
>>><br> >>>>>> >>> On 6/14/19 21:11, \
Yasumasa Suenaga wrote:<br> >>>>>> >>>> Hi \
Serguei,<br> >>>>>> >>>><br>
>>>>>> >>>> Thank you for your comment!<br>
>>>>>> >>>><br>
>>>>>> >>>> On 2019/06/15 8:00, <a \
href="mailto:serguei.spitsyn@oracle.com" target="_blank" \
rel="noreferrer">serguei.spitsyn@oracle.com</a><br> >>>>>> \
<mailto:<a href="mailto:serguei.spitsyn@oracle.com" target="_blank" \
rel="noreferrer">serguei.spitsyn@oracle.com</a>> wrote:<br> \
>>>>>> >>>>> Hi Yasumasa,<br> \
>>>>>> >>>>><br> >>>>>> \
>>>>> I've added myself as a reviewer, so you can finalize it \
now.<br> >>>>>> >>>><br>
>>>>>> >>>> I moved CSR to Finalized, and \
added a comment for your<br> >>>>>> question.<br>
>>>>>> >>><br>
>>>>>> >>> Okay, thanks!<br>
>>>>>> >>><br>
>>>>>> >>><br>
>>>>>> >>>>> The fix looks pretty good to \
me.<br> >>>>>> >>>>><br>
>>>>>> >>>>> One minor suggestion is to \
use the final field NO_REMOTE<br> >>>>>> instead<br>
>>>>>> >>>>> of null for initialization of \
the local variable "remote".<br> >>>>>> \
>>>><br> >>>>>> >>>> I will fix \
that.<br> >>>>>> >>>><br>
>>>>>> >>>><br>
>>>>>> >>>>> Also just an observation that \
there is some room for option<br> >>>>>> \
>>>>> processing refactoring.<br> >>>>>> \
>>>><br> >>>>>> >>>> Your \
suggestion handles all options in one parser method.<br> >>>>>> \
>>>> I concern it might be complex for option validation.<br> \
>>>>>> >>>> (e.g. `jmap -heap` is allowed, \
but `jstack -heap` is not<br> >>>>>> allowed.)<br>
>>>>>> >>><br>
>>>>>> >>> This concern is not valid as the list \
allowed options allowed<br> >>>>>> for each<br>
>>>>>> >>> jhsdb sub-command is controlled with \
the longOpts argument.<br> >>>>>> >>><br>
>>>>>> >>> jmap has:<br>
>>>>>> >>> String[] longOpts = \
{"exe=", "core=", "pid=",<br> >>>>>> \
"remote=",<br> >>>>>> >>> \
"heap", "binaryheap", "dumpfile=", \
"histo",<br> >>>>>> >>> \
"clstats", "finalizerinfo"};<br> >>>>>> \
>>><br> >>>>>> >>> but jstack has:<br>
>>>>>> >>> String[] longOpts = \
{"exe=", "core=", "pid=",<br> >>>>>> \
"remote=",<br> >>>>>> >>> \
"mixed", "locks"};<br> >>>>>> \
>>><br> >>>>>> >>>> IMHO refactoring \
should be worked on another issue.<br> >>>>>> \
>>><br> >>>>>> >>> Agreed.<br>
>>>>>> >>><br>
>>>>>> >>><br>
>>>>>> >>>> If you are OK in above, I will \
upload new webrev.<br> >>>>>> >>><br>
>>>>>> >>> Yes, I'm Okay with it.<br>
>>>>>> >>><br>
>>>>>> >>><br>
>>>>>> >>> Thanks,<br>
>>>>>> >>> Serguei<br>
>>>>>> >>><br>
>>>>>> >>><br>
>>>>>> >>>> Thanks,<br>
>>>>>> >>>><br>
>>>>>> >>>> Yasumasa<br>
>>>>>> >>>><br>
>>>>>> >>>><br>
>>>>>> >>>>> All the jhsdb sub-commands do \
execute similar loops like<br> >>>>>> this:<br>
>>>>>> >>>>> while((s = sg.next(null, \
longOpts)) != null) {<br> >>>>>> >>>>> \
. . .<br> >>>>>> >>>>> }<br>
>>>>>> >>>>><br>
>>>>>> >>>>> It can be moved into a \
separate method instead.<br> >>>>>> >>>>> \
The longOpts can passed in arguments.<br> >>>>>> \
>>>>><br> >>>>>> >>>>> It \
can be something like this:<br> >>>>>> \
>>>>><br> >>>>>> >>>>> \
private ArrayList<String> processOptions(final String[]<br> \
>>>>>> oldArgs,<br> >>>>>> \
>>>>> \
final String[]<br> >>>>>> >>>>> \
longOpts,<br> >>>>>> >>>>> \
boolean<br> >>>>>> allowEmpty) {<br>
>>>>>> >>>>> SAGetopt sg = \
new SAGetopt(oldArgs);<br> >>>>>> >>>>> \
ArrayList<String> newArgs = new ArrayList();<br> >>>>>> \
>>>>><br> >>>>>> >>>>> \
String pid = null;<br> >>>>>> >>>>> \
String exe = null;<br> >>>>>> >>>>> \
String core = null;<br> >>>>>> >>>>> \
String s = null;<br> >>>>>> >>>>> \
String dumpfile = null;<br> >>>>>> >>>>> \
String remote = NO_REMOTE;<br> >>>>>> \
>>>>> boolean requestHeapdump = false;<br> \
>>>>>> >>>>><br> >>>>>> \
>>>>> while((s = sg.next(null, longOpts)) != null) {<br> \
>>>>>> >>>>> if \
(s.equals("exe")) {<br> >>>>>> \
>>>>> exe = sg.getOptarg();<br> \
>>>>>> >>>>> \
continue;<br> >>>>>> >>>>> \
}<br> >>>>>> >>>>> if \
(s.equals("core")) {<br> >>>>>> \
>>>>> core = sg.getOptarg();<br> \
>>>>>> >>>>> \
continue;<br> >>>>>> >>>>> \
}<br> >>>>>> >>>>> if \
(s.equals("pid")) {<br> >>>>>> \
>>>>> pid = sg.getOptarg();<br> \
>>>>>> >>>>> \
continue;<br> >>>>>> >>>>> \
}<br> >>>>>> >>>>> if \
(s.equals("remote")) {<br> >>>>>> \
>>>>> remote = sg.getOptarg();<br> \
>>>>>> >>>>> \
continue;<br> >>>>>> >>>>> \
}<br> >>>>>> >>>>> if \
(s.equals("mixed")) {<br> >>>>>> \
>>>>> newArgs.add("-m");<br> \
>>>>>> >>>>> \
continue;<br> >>>>>> >>>>> \
}<br> >>>>>> >>>>> if \
(s.equals("locks")) {<br> >>>>>> \
>>>>> newArgs.add("-l");<br> \
>>>>>> >>>>> \
continue;<br> >>>>>> >>>>> \
}<br> >>>>>> >>>>> if \
(s.equals("heap")) {<br> >>>>>> \
>>>>> newArgs.add("-heap");<br> \
>>>>>> >>>>> \
continue;<br> >>>>>> >>>>> \
}<br> >>>>>> >>>>> if \
(s.equals("binaryheap")) {<br> >>>>>> \
>>>>> requestHeapdump = true;<br> \
>>>>>> >>>>> \
continue;<br> >>>>>> >>>>> \
}<br> >>>>>> >>>>> if \
(s.equals("dumpfile")) {<br> >>>>>> \
>>>>> dumpfile = sg.getOptarg();<br> \
>>>>>> >>>>> \
continue;<br> >>>>>> >>>>> \
}<br> >>>>>> >>>>> if \
(s.equals("histo")) {<br> >>>>>> \
>>>>> newArgs.add("-histo");<br> \
>>>>>> >>>>> \
continue;<br> >>>>>> >>>>> \
}<br> >>>>>> >>>>> if \
(s.equals("clstats")) {<br> >>>>>> \
>>>>> newArgs.add("-clstats");<br> \
>>>>>> >>>>> \
continue;<br> >>>>>> >>>>> \
}<br> >>>>>> >>>>> if \
(s.equals("finalizerinfo")) {<br> >>>>>> \
>>>>> \
newArgs.add("-finalizerinfo");<br> >>>>>> \
>>>>> continue;<br> >>>>>> \
>>>>> }<br> >>>>>> \
>>>>> if (s.equals("flags")) {<br> \
>>>>>> >>>>> \
newArgs.add("-flags");<br> >>>>>> \
>>>>> continue;<br> >>>>>> \
>>>>> }<br> >>>>>> \
>>>>> if (s.equals("sysprops")) {<br> \
>>>>>> >>>>> \
newArgs.add("-sysprops");<br> >>>>>> \
>>>>> continue;<br> >>>>>> \
>>>>> }<br> >>>>>> \
>>>>> if (s.equals("serverid")) {<br> \
>>>>>> >>>>> \
String serverid = sg.getOptarg();<br> >>>>>> \
>>>>> if (serverid != null) {<br> \
>>>>>> >>>>> \
newArgs.add(serverid);<br> >>>>>> >>>>> \
}<br> >>>>>> >>>>> \
continue;<br> >>>>>> >>>>> \
}<br> >>>>>> >>>>> }<br>
>>>>>> >>>>><br>
>>>>>> >>>>> if \
(!requestHeapdump && (dumpfile != null)) {<br> >>>>>> \
>>>>> throw new \
IllegalArgumentException("Unexpected<br> >>>>>> \
>>>>> argument dumpfile");<br> >>>>>> \
>>>>> }<br> >>>>>> \
>>>>> if (requestHeapdump) {<br> \
>>>>>> >>>>> if \
(dumpfile == null) {<br> >>>>>> >>>>> \
newArgs.add("-heap:format=b");<br> >>>>>> \
>>>>> } else {<br> >>>>>> \
>>>>> \
newArgs.add("-heap:format=b,file=" +<br> >>>>>> \
dumpfile);<br> >>>>>> >>>>> \
}<br> >>>>>> >>>>> }<br>
>>>>>> >>>>> \
buildAttachArgs(newArgs, pid, exe, core, remote,<br> >>>>>> \
>>>>> allowEmpty);<br> >>>>>> \
>>>>><br> >>>>>> >>>>> \
return newArgs;<br> >>>>>> >>>>> \
}<br> >>>>>> >>>>><br>
>>>>>> >>>>> private static void \
runCLHSDB(String[] oldArgs) {<br> >>>>>> \
>>>>> String[] longOpts = {"exe=", \
"core=", "pid="};<br> >>>>>> \
>>>>> ArrayList<String> newArgs = \
processOptions(oldArgs,<br> >>>>>> >>>>> \
longOpts, true);<br> >>>>>> >>>>><br>
>>>>>> >>>>> \
CLHSDB.main(newArgs.toArray(new<br> >>>>>> \
String[newArgs.size()]));<br> >>>>>> >>>>> \
}<br> >>>>>> >>>>><br>
>>>>>> >>>>> private static void \
runHSDB(String[] oldArgs) {<br> >>>>>> \
>>>>> String[] longOpts = {"exe=", \
"core=", "pid="};<br> >>>>>> \
>>>>> ArrayList<String> newArgs =<br> \
>>>>>> processOptions(oldArgs,<br> >>>>>> \
>>>>> longOpts, true);<br> >>>>>> \
>>>>><br> >>>>>> >>>>> \
HSDB.main(newArgs.toArray(new<br> >>>>>> \
String[newArgs.size()]));<br> >>>>>> >>>>> \
}<br> >>>>>> >>>>><br>
>>>>>> >>>>> private static void \
runJSTACK(String[] oldArgs) {<br> >>>>>> \
>>>>> String[] longOpts = {"exe=", \
"core=", "pid=",<br> >>>>>> \
"remote=",<br> >>>>>> >>>>> \
"mixed", "locks"};<br> >>>>>> \
>>>>> ArrayList<String> newArgs = \
processOptions(oldArgs,<br> >>>>>> >>>>> \
longOpts, false);<br> >>>>>> >>>>><br>
>>>>>> >>>>> JStack jstack = \
new JStack(false, false);<br> >>>>>> >>>>> \
jstack.runWithArgs(newArgs.toArray(new<br> >>>>>> \
>>>>> String[newArgs.size()]));<br> >>>>>> \
>>>>> }<br> >>>>>> \
>>>>><br> >>>>>> >>>>> \
private static void runJMAP(String[] oldArgs) {<br> >>>>>> \
>>>>> String[] longOpts = {"exe=", \
"core=", "pid=",<br> >>>>>> \
"remote=",<br> >>>>>> >>>>> \
"heap", "binaryheap", "dumpfile=", \
"histo",<br> >>>>>> >>>>> \
"clstats", "finalizerinfo"};<br> >>>>>> \
>>>>><br> >>>>>> >>>>> \
ArrayList<String> newArgs = processOptions(oldArgs,<br> \
>>>>>> >>>>> longOpts, false);<br> \
>>>>>> >>>>><br> >>>>>> \
>>>>> JMap.main(newArgs.toArray(new<br> \
>>>>>> String[newArgs.size()]));<br> >>>>>> \
>>>>> }<br> >>>>>> \
>>>>><br> >>>>>> >>>>> \
private static void runJINFO(String[] oldArgs) {<br> >>>>>> \
>>>>> String[] longOpts = {"exe=", \
"core=", "pid=",<br> >>>>>> \
"remote=",<br> >>>>>> >>>>> \
"flags", "sysprops"};<br> >>>>>> \
>>>>> ArrayList<String> newArgs = \
processOptions(oldArgs,<br> >>>>>> >>>>> \
longOpts, false);<br> >>>>>> >>>>><br>
>>>>>> >>>>> \
JInfo.main(newArgs.toArray(new<br> >>>>>> \
String[newArgs.size()]));<br> >>>>>> >>>>> \
}<br> >>>>>> >>>>><br>
>>>>>> >>>>> private static void \
runJSNAP(String[] oldArgs) {<br> >>>>>> \
>>>>> String[] longOpts = {"exe=", \
"core=", "pid=",<br> >>>>>> \
"remote=",<br> >>>>>> >>>>> \
"all"};<br> >>>>>> >>>>> \
ArrayList<String> newArgs = processOptions(oldArgs,<br> \
>>>>>> >>>>> longOpts, false);<br> \
>>>>>> >>>>> \
JSnap.main(newArgs.toArray(new<br> >>>>>> \
String[newArgs.size()]));<br> >>>>>> >>>>> \
}<br> >>>>>> >>>>><br>
>>>>>> >>>>> private static void \
runDEBUGD(String[] oldArgs) {<br> >>>>>> \
>>>>> // By default SA agent classes prefer Windows<br> \
>>>>>> process<br> >>>>>> \
>>>>> debugger<br> >>>>>> \
>>>>> // to windbg debugger. SA expects special<br> \
>>>>>> properties to<br> >>>>>> \
>>>>> be set<br> >>>>>> \
>>>>> // to choose other debuggers. We will set \
those<br> >>>>>> here<br>
>>>>>> before<br>
>>>>>> >>>>> // attaching to \
SA agent.<br> >>>>>> >>>>><br>
>>>>>> \
System.setProperty("sun.jvm.hotspot.debugger.useWindbgDebugger",<br> \
>>>>>> >>>>> "true");<br> \
>>>>>> >>>>><br> >>>>>> \
>>>>> String[] longOpts = {"exe=", \
"core=", "pid=",<br> >>>>>> \
"serverid="};<br> >>>>>> >>>>> \
ArrayList<String> newArgs = processOptions(oldArgs,<br> \
>>>>>> >>>>> longOpts, false);<br> \
>>>>>> >>>>><br> >>>>>> \
>>>>> // delegate to the actual SA debug server.<br> \
>>>>>> >>>>> \
sun.jvm.hotspot.DebugServer.main(newArgs.toArray(new<br> >>>>>> \
>>>>> String[newArgs.size()]));<br> >>>>>> \
>>>>> }<br> >>>>>> \
>>>>><br> >>>>>> >>>>><br>
>>>>>> >>>>> Please, let me know what do \
you think.<br> >>>>>> >>>>><br>
>>>>>> >>>>> Thanks,<br>
>>>>>> >>>>> Serguei<br>
>>>>>> >>>>><br>
>>>>>> >>>>><br>
>>>>>> >>>>> On 6/9/19 7:29 PM, Yasumasa \
Suenaga wrote:<br> >>>>>> >>>>>> Sorry, \
new webrev is here:<br> >>>>>> >>>>>> \
<a href="http://cr.openjdk.java.net/~ysuenaga/JDK-8209790/webrev.01/" rel="noreferrer \
noreferrer" target="_blank">http://cr.openjdk.java.net/~ysuenaga/JDK-8209790/webrev.01/</a><br>
>>>>>> >>>>>><br>
>>>>>> >>>>>> Yasumasa<br>
>>>>>> >>>>>><br>
>>>>>> >>>>>> 2019年6月10日(月) \
11:27 Yasumasa Suenaga<<a href="mailto:yasuenag@gmail.com" target="_blank" \
rel="noreferrer">yasuenag@gmail.com</a><br> >>>>>> \
<mailto:<a href="mailto:yasuenag@gmail.com" target="_blank" \
rel="noreferrer">yasuenag@gmail.com</a>>>:<br> >>>>>> \
>>>>>>> PING: Could you review them?<br> \
>>>>>> >>>>>>><br> \
>>>>>> >>>>>>>>>>> \
JBS:<a href="https://bugs.openjdk.java.net/browse/JDK-8209790" rel="noreferrer \
noreferrer" target="_blank">https://bugs.openjdk.java.net/browse/JDK-8209790</a><br> \
>>>>>> >>>>>>>>>>> \
CSR:<a href="https://bugs.openjdk.java.net/browse/JDK-8224979" rel="noreferrer \
noreferrer" target="_blank">https://bugs.openjdk.java.net/browse/JDK-8224979</a><br> \
>>>>>> >>>>>>>>>>><br> \
>>>>>> webrev:<a \
href="http://cr.openjdk.java.net/~ysuenaga/JDK-8209790/webrev.00/" rel="noreferrer \
noreferrer" target="_blank">http://cr.openjdk.java.net/~ysuenaga/JDK-8209790/webrev.00/</a><br>
>>>>>> >>>>>>>>>>><br>
>>>>>> >>>>>>> It is P3 bug, but I \
want to fix it before JDK 13 RDP 1 if<br> >>>>>> \
possible.<br> >>>>>> >>>>>>><br>
>>>>>> >>>>>>><br>
>>>>>> >>>>>>> Thanks,<br>
>>>>>> >>>>>>><br>
>>>>>> >>>>>>> Yasumasa<br>
>>>>>> >>>>>>><br>
>>>>>> >>>>>>><br>
>>>>>> >>>>>>> 2019年6月5日(水) \
14:06 Yasumasa Suenaga<<a href="mailto:yasuenag@gmail.com" target="_blank" \
rel="noreferrer">yasuenag@gmail.com</a><br> >>>>>> \
<mailto:<a href="mailto:yasuenag@gmail.com" target="_blank" \
rel="noreferrer">yasuenag@gmail.com</a>>>:<br> >>>>>> \
>>>>>>>> Hi Jc,<br> >>>>>> \
>>>>>>>><br> >>>>>> \
>>>>>>>> Thank you for your comment!<br> \
>>>>>> >>>>>>>> I updated a \
webrev:<br> >>>>>> >>>>>>>><br>
>>>>>> >>>>>>>><br>
>>>>>> <a \
href="http://cr.openjdk.java.net/~ysuenaga/JDK-8209790/webrev.01/" rel="noreferrer \
noreferrer" target="_blank">http://cr.openjdk.java.net/~ysuenaga/JDK-8209790/webrev.01/</a><br>
>>>>>> >>>>>>>><br>
>>>>>> >>>>>>>>> - In \
runTests; if DebugdUtils implemented<br> >>>>>> Closeable, you<br>
>>>>>> >>>>>>>>> could just do \
a try-with-resources instead of the<br> >>>>>> finally<br>
>>>>>> >>>>>>>>> clause...<br>
>>>>>> >>>>>>>> I created \
DebugdUtils for convenience class for attach -<br> >>>>>> \
detach<br> >>>>>> >>>>>>>> \
mechanism of debug server.<br> >>>>>> \
>>>>>>>> IMHO it is prefer "detach" to \
"close" in this case.<br> >>>>>> \
>>>>>>>><br> >>>>>> \
>>>>>>>><br> >>>>>> \
>>>>>>>> Thanks,<br> >>>>>> \
>>>>>>>><br> >>>>>> \
>>>>>>>> Yasumasa<br> >>>>>> \
>>>>>>>><br> >>>>>> \
>>>>>>>><br> >>>>>> \
>>>>>>>> 2019年6月5日(水) 11:34 Jean Christophe<br> \
>>>>>> Beyler<<a href="mailto:jcbeyler@google.com" \
target="_blank" rel="noreferrer">jcbeyler@google.com</a> <mailto:<a \
href="mailto:jcbeyler@google.com" target="_blank" \
rel="noreferrer">jcbeyler@google.com</a>>>:<br> >>>>>> \
>>>>>>>><br> >>>>>> \
>>>>>>>>> Hi Yasumasa,<br> >>>>>> \
>>>>>>>>><br> >>>>>> \
>>>>>>>>> I'm not an official reviewer but I don't \
see an issue<br> >>>>>> with the<br>
>>>>>> >>>>>>>>> CSR (except \
that this seems to be bringing a fork in the<br> >>>>>> \
tools<br> >>>>>> >>>>>>>>> \
with some handling remote and others not).<br> >>>>>> \
>>>>>>>>><br> >>>>>> \
>>>>>>>>> However, this code is really repetitive and this \
is<br> >>>>>> not the<br>
>>>>>> >>>>>>>>> place to do a \
big refactor probably but we could do a<br> >>>>>> few<br>
>>>>>> nits<br>
>>>>>> >>>>>>>>> perhaps:<br>
>>>>>> >>>>>>>>> - \
Instead of every tool calling commonHelp with an<br> >>>>>> \
>>>>>>>>> additional flag you could divide into commonHelp \
and<br> >>>>>> >>>>>>>>> \
commonHelpWithRemote for the tools and they both call<br> >>>>>> \
the<br> >>>>>> >>>>>>>>> \
current commonHelp with that boolean; so that when we<br> >>>>>> \
are<br> >>>>>> >>>>>>>>> \
looking at the tool code we know what we are<br> >>>>>> getting... \
So<br> >>>>>> >>>>>>>>> \
something like:<br> >>>>>> \
>>>>>>>>><br> >>>>>> \
>>>>>>>>> private static boolean commonHelp(String mode, \
boolean<br> >>>>>> >>>>>>>>> \
canConnectToRemote) {<br> >>>>>> \
>>>>>>>>> ..<br> >>>>>> \
>>>>>>>>> }<br> >>>>>> \
>>>>>>>>><br> >>>>>> \
>>>>>>>>> private static boolean commonHelp(String mode) \
{<br> >>>>>> >>>>>>>>> \
return commonHelp(mode, false);<br> >>>>>> \
>>>>>>>>> }<br> >>>>>> \
>>>>>>>>><br> >>>>>> \
>>>>>>>>> private static boolean \
commonHelpWithRemote(String<br> >>>>>> mode) {<br>
>>>>>> >>>>>>>>> return \
commonHelp(mode, false);<br> >>>>>> \
>>>>>>>>> }<br> >>>>>> \
>>>>>>>>><br> >>>>>> \
>>>>>>>>> and that way the tools that change are just \
going from:<br> >>>>>> \
>>>>>>>>> - return \
commonHelp("jmap");<br> >>>>>> \
>>>>>>>>> + return \
commonHelpWithRemote("jmap");<br> >>>>>> \
>>>>>>>>><br> >>>>>> \
>>>>>>>>> - In the same vein, instead of passing null to \
the<br> >>>>>> >>>>>>>>> \
buildAttachArgs; you could make a variable null:<br> >>>>>> \
>>>>>>>>><br> >>>>>> \
>>>>>>>>> - buildAttachArgs(newArgs, pid, exe, \
core, true);<br> >>>>>> \
>>>>>>>>> + String noRemote = null;<br> \
>>>>>> >>>>>>>>> + \
buildAttachArgs(newArgs, pid, exe, core,<br> >>>>>> noRemote,<br>
>>>>>> >>>>>>>>> true);<br>
>>>>>> >>>>>>>>><br>
>>>>>> >>>>>>>>><br>
>>>>>> >>>>>>>>><br>
>>>>>> -<a \
href="http://cr.openjdk.java.net/~ysuenaga/JDK-8209790/webrev.00/test/hotspot/jtreg/serviceability/sa/sadebugd/DebugdUtils.java.html" \
rel="noreferrer noreferrer" \
target="_blank">http://cr.openjdk.java.net/~ysuenaga/JDK-8209790/webrev.00/test/hotspot/jtreg/serviceability/sa/sadebugd/DebugdUtils.java.html</a><br>
>>>>>><br>
>>>>>><br>
>>>>>> >>>>>>>>><br>
>>>>>> >>>>>>>>> Nit: \
you have empty lines at l64 and l73<br> >>>>>> \
>>>>>>>>><br> >>>>>> \
>>>>>>>>><br> >>>>>> -<a \
href="http://cr.openjdk.java.net/~ysuenaga/JDK-8209790/webrev.00/test/hotspot/jtreg/serviceability/sa/sadebugd/DebugdConnectTest.java.html" \
rel="noreferrer noreferrer" \
target="_blank">http://cr.openjdk.java.net/~ysuenaga/JDK-8209790/webrev.00/test/hotspot/jtreg/serviceability/sa/sadebugd/DebugdConnectTest.java.html</a><br>
>>>>>><br>
>>>>>><br>
>>>>>> >>>>>>>>><br>
>>>>>> >>>>>>>>> Nit : \
you have an empty line at l110<br> >>>>>> \
>>>>>>>>><br> >>>>>> \
>>>>>>>>> - In runTests; if DebugdUtils \
implemented<br> >>>>>> Closeable, you<br>
>>>>>> >>>>>>>>> could just do \
a try-with-resources instead of the<br> >>>>>> finally<br>
>>>>>> >>>>>>>>> clause...<br>
>>>>>> >>>>>>>>><br>
>>>>>> >>>>>>>>> All of these \
are details, I just thought I'd mention<br> >>>>>> them :)<br>
>>>>>> >>>>>>>>> Jc<br>
>>>>>> >>>>>>>>><br>
>>>>>> >>>>>>>>> On Tue, Jun \
4, 2019 at 6:44 PM Yasumasa<br> >>>>>> \
>>>>>>>>> Suenaga<<a href="mailto:yasuenag@gmail.com" \
target="_blank" rel="noreferrer">yasuenag@gmail.com</a><br> >>>>>> \
<mailto:<a href="mailto:yasuenag@gmail.com" target="_blank" \
rel="noreferrer">yasuenag@gmail.com</a>>> wrote:<br> \
>>>>>> >>>>>>>>>> PING: \
Could you review them?<br> >>>>>> \
>>>>>>>>>><br> >>>>>> \
>>>>>>>>>>> JBS:<a \
href="https://bugs.openjdk.java.net/browse/JDK-8209790" rel="noreferrer noreferrer" \
target="_blank">https://bugs.openjdk.java.net/browse/JDK-8209790</a><br> \
>>>>>> >>>>>>>>>>> \
CSR:<a href="https://bugs.openjdk.java.net/browse/JDK-8224979" rel="noreferrer \
noreferrer" target="_blank">https://bugs.openjdk.java.net/browse/JDK-8224979</a><br> \
>>>>>> >>>>>>>>>>><br> \
>>>>>> webrev:<a \
href="http://cr.openjdk.java.net/~ysuenaga/JDK-8209790/webrev.00/" rel="noreferrer \
noreferrer" target="_blank">http://cr.openjdk.java.net/~ysuenaga/JDK-8209790/webrev.00/</a><br>
>>>>>> >>>>>>>>>>><br>
>>>>>> >>>>>>>>>> CSR \
status is provisional. So I need reviewers both<br> >>>>>> CSR \
and<br> >>>>>> >>>>>>>>>> \
webrev.<br> >>>>>> \
>>>>>>>>>><br> >>>>>> \
>>>>>>>>>><br> >>>>>> \
>>>>>>>>>> Thanks,<br> >>>>>> \
>>>>>>>>>><br> >>>>>> \
>>>>>>>>>> Yasumasa<br> >>>>>> \
>>>>>>>>>><br> >>>>>> \
>>>>>>>>>><br> >>>>>> \
>>>>>>>>>> 2019年5月29日(水) 22:37 Yasumasa<br> \
>>>>>> Suenaga<<a href="mailto:yasuenag@gmail.com" \
target="_blank" rel="noreferrer">yasuenag@gmail.com</a> <mailto:<a \
href="mailto:yasuenag@gmail.com" target="_blank" \
rel="noreferrer">yasuenag@gmail.com</a>>>:<br> >>>>>> \
>>>>>>>>>>> Hi all,<br> >>>>>> \
>>>>>>>>>>><br> >>>>>> \
>>>>>>>>>>> Please review this change:<br> \
>>>>>> >>>>>>>>>>><br> \
>>>>>> >>>>>>>>>>> \
JBS:<a href="https://bugs.openjdk.java.net/browse/JDK-8209790" rel="noreferrer \
noreferrer" target="_blank">https://bugs.openjdk.java.net/browse/JDK-8209790</a><br> \
>>>>>> >>>>>>>>>>> \
CSR:<a href="https://bugs.openjdk.java.net/browse/JDK-8224979" rel="noreferrer \
noreferrer" target="_blank">https://bugs.openjdk.java.net/browse/JDK-8224979</a><br> \
>>>>>> >>>>>>>>>>><br> \
>>>>>> webrev:<a \
href="http://cr.openjdk.java.net/~ysuenaga/JDK-8209790/webrev.00/" rel="noreferrer \
noreferrer" target="_blank">http://cr.openjdk.java.net/~ysuenaga/JDK-8209790/webrev.00/</a><br>
>>>>>> >>>>>>>>>>><br>
>>>>>> >>>>>>>>>>><br>
>>>>>> >>>>>>>>>>> In \
JDK 8 or earlier, some tools (jstack, jmap,<br> >>>>>> jinfo) \
can<br> >>>>>> \
>>>>>>>>>>> connect to<br> >>>>>> \
>>>>>>>>>>> debug server (jsadebugd). However it has \
not done<br> >>>>>> so since<br>
>>>>>> >>>>>>>>>>> JDK 9 \
because<br> >>>>>> \
>>>>>>>>>>> jhsdb cannot accept the attach request \
to debug<br> >>>>>> server.<br>
>>>>>> >>>>>>>>>>> So I \
want to introduce new option `--remote` to<br> >>>>>> connect \
to<br> >>>>>> \
>>>>>>>>>>> debug server.<br> \
>>>>>> >>>>>>>>>>><br> \
>>>>>> >>>>>>>>>>> I \
created CSR for this issue. So please review it<br> >>>>>> \
together.<br> >>>>>> \
>>>>>>>>>>><br> >>>>>> \
>>>>>>>>>>><br> >>>>>> \
>>>>>>>>>>> Thanks,<br> >>>>>> \
>>>>>>>>>>><br> >>>>>> \
>>>>>>>>>>> Yasumasa<br> >>>>>> \
>>>>>>>>><br> >>>>>> \
>>>>>>>>> --<br> >>>>>> \
>>>>>>>>><br> >>>>>> \
>>>>>>>>> Thanks,<br> >>>>>> \
>>>>>>>>> Jc<br> >>>>>> \
>>>>><br> >>>>>> >>><br>
>>>>>> ><br>
>>>>>><br>
<br>
</blockquote></div>
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic