PING: RFR: 8209790: SA tools not providing option to connect to debug server
Yasumasa Suenaga
yasuenag at gmail.com
Mon Jun 17 08:50:26 UTC 2019
Hi David,
8209790 is filed as a bug.
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.
(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