PING: RFR: 8209790: SA tools not providing option to connect to debug server
Yasumasa Suenaga
yasuenag at gmail.com
Sun Jun 16 14:22:51 UTC 2019
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/
>> IMHO refactoring should be worked on another issue.
>
> Agreed.
I filed it to JBS:
https://bugs.openjdk.java.net/browse/JDK-8226204
Thanks,
Yasumasa
On 2019/06/15 15:10, 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 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>:
>>>>> 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>:
>>>>>> 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>:
>>>>>>> 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> 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>:
>>>>>>>>> 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