PING: RFR: 8209790: SA tools not providing option to connect to debug server
Yasumasa Suenaga
yasuenag at gmail.com
Sat Jun 15 04:11:54 UTC 2019
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.
> 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.)
IMHO refactoring should be worked on another issue.
If you are OK in above, I will upload new webrev.
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