PING: RFR: 8209790: SA tools not providing option to connect to debug server
Chris Plummer
chris.plummer at oracle.com
Thu Jul 4 19:24:17 UTC 2019
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.
>
> 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