PING: RFR: 8209790: SA tools not providing option to connect to debug server
Chris Plummer
chris.plummer at oracle.com
Thu Jul 4 21:41:28 UTC 2019
On 7/4/19 2:07 PM, David Holmes wrote:
> Hi Chris,
>
> On 5/07/2019 5:24 am, Chris Plummer wrote:
>> Hi Yasumasa,
>>
>> On 7/4/19 5:30 AM, Yasumasa Suenaga wrote:
>>> Hi Chris,
>>>
>>> Thank you for your review.
>>>
>>> On 2019/07/04 8:07, Chris Plummer wrote:
>>>> Hi Yasumasa,
>>>>
>>>> Overall these changes look good, but I think there is a bit of
>>>> cleanup still needed.
>>>>
>>>> You've changed the indentation of the help to always have a few
>>>> spaces before the /t. If you are going to do this, then perhaps you
>>>> don't need the /t anymore.
>>>
>>> Ok, I will replace '\t' to whitespace.
>>>
>>>
>>>> There are 3 checks for "remote != null". Shouldn't they be "remote
>>>> != NO_REMOTE"?
>>>
>>> I will fix them.
>>>
>>>
>>>> The old jstack help output is a bit more informative than your output:
>>>>
>>>> jstack [-m] [-l] [server_id@]<remote server IP or hostname>
>>>> (to connect to a remote debug server)
>>>>
>>>> vs
>>>>
>>>> --connect <[id@]server> To operate on the remote debug server.
>>>>
>>>> More specifically, you replaced <remote server IP or hostname> with
>>>> "server". I think the former is a bit more self explanatory.
>>>
>>> Exactly, but it is too long to show on the console.
>>> So I used short word: server.
>>>
>>> If we can ignore long line, or can add newline (\n) on help message,
>>> I can change it. What do you think?
>> I don't see why not. Isn't there already help output that relies on
>> newlines?
>>>
>>>
>>>> Also, why say "to operate on" rather than "to connect to"? The
>>>> later seems more direct and correct.
>>>
>>> I will replace it to "to connect to".
>>>
>>>
>>>> I think you should also mention sadebugd in the jhsdb help output
>>>> for the -connect option, and not just say "debug server". Just
>>>> change it to "debug server (sadebugd)".
>>>
>>> I will add "(sadebugd)".
>>>
>>>
>>>> There are also online jhsdb doc updates that we need to make sure
>>>> are eventually taken care of properly.
>>>>
>>>> https://docs.oracle.com/en/java/javase/11/tools/jhsdb.html
>>>>
>>>> The CSR should trigger the doc update, but you need to make sure
>>>> the CSR contains all the needed details and instructions for the
>>>> doc writer. For example, here's what the jstack docs look like:
>>>>
>>>> https://docs.oracle.com/javase/7/docs/technotes/tools/share/jstack.html
>>>>
>>>>
>>>> *jstack* [ option ] [server-id@]remote-hostname-or-IP
>>>> ...
>>>> remote-hostname-or-IP
>>>> remote debug server's (see jsadebugd) hostname or IP address.
>>>>
>>>> It includes a reference to jsadebugd (and an actual URL link). This
>>>> was important for me just in doing this review, because I had to go
>>>> and figure out what "debug server" was all about, having never
>>>> dealt with it before. I found it by looking at the old jstack docs
>>>> and help output. Adding this to the jhsdb docs should be called
>>>> out in the CSR.
>>>
>>> I'm not an employee of Oracle, so I think I cannot change
>>> documents(s) which are provided by Oracle web sites.
>>> What should I do?
>> Since the CSR drives the documentation update, I was just asking that
>> the CSR be updated to include all information needed to properly
>> update the docs.
>
> I'm not aware of any automatic doc updating process driven by the CSR.
> If there is a doc task to be done then a specific task needs to be
> created for it. In this case it is the manpage for jhsdb that needs to
> be updated, which is now a dev task.
Ok, I thought I had heard that once. Must have been mistaken.
What about the question of making minor tweaks to the help output so it
is no longer the same as what is mentioned in the CSR. How does this
affect the CSR process?
Chris
>
> David
>
>>>
>>> Also CSR of this issue has been approved (JDK-8224979).
>>> Can we change it?
>>>
>> I'm not sure. I think that's a question for Joe Darcy. You would also
>> need to update it for the suggested help output changes above.
>>
>> thanks,
>>
>> Chris
>>>
>>> Thanks,
>>>
>>> Yasumasa
>>>
>>>
>>>> thanks,
>>>>
>>>> Chris
>>>>
>>>> On 7/1/19 3:26 PM, Yasumasa Suenaga wrote:
>>>>> PING: Could you review it?
>>>>>
>>>>>> http://cr.openjdk.java.net/~ysuenaga/JDK-8209790/webrev.03/
>>>>>
>>>>>
>>>>> Yasumasa
>>>>>
>>>>>
>>>>> On 2019/06/25 16:47, Yasumasa Suenaga wrote:
>>>>>> Hi,
>>>>>>
>>>>>> This enhancement has been retargeted to 14, and the CSR has been
>>>>>> approved.
>>>>>> I uploaded a webrev for 14. Could you review it?
>>>>>>
>>>>>> http://cr.openjdk.java.net/~ysuenaga/JDK-8209790/webrev.03/
>>>>>>
>>>>>> It includes the fix to rename `--remote` to `--connect` that is
>>>>>> suggested in CSR.
>>>>>> It passed tests on submit repo.
>>>>>>
>>>>>>
>>>>>> Thanks,
>>>>>>
>>>>>> Yasumasa
>>>>>>
>>>>>>
>>>>>> 2019年6月17日(月) 22:11 Yasumasa Suenaga <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
>>>>>>>>>>> >>>>>
>>>>>>>>>>> >>>
>>>>>>>>>>> >
>>>>>>>>>>>
>>>>
>>
More information about the serviceability-dev
mailing list