PING: RFR: 8209790: SA tools not providing option to connect to debug server
serguei.spitsyn at oracle.com
serguei.spitsyn at oracle.com
Fri Jun 14 23:00:36 UTC 2019
Hi Yasumasa,
I've added myself as a reviewer, so you can finalize it now.
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".
Also just an observation that there is some room for option processing
refactoring.
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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20190614/dd5fd0f4/attachment-0001.html>
More information about the serviceability-dev
mailing list