PING: RFR: 8209790: SA tools not providing option to connect to debug server
Yasumasa Suenaga
yasuenag at gmail.com
Thu Jul 4 22:55:23 UTC 2019
On 2019/07/05 6:44, David Holmes wrote:
> <trimming>
>
> On 5/07/2019 7:41 am, Chris Plummer wrote:
>> On 7/4/19 2:07 PM, David Holmes wrote:
>>> 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?
>
> The changes should be finalized before the CSR is finalized and approved. I think the CSR request would need to be re-opened, updated and resubmitted.
Ok, I will do that when the webrev is reviewed.
I will upload new one after the question of me is cleared.
Yasumasa
> David
> -----
>
>> 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