PING: RFR: 8209790: SA tools not providing option to connect to debug server
serguei.spitsyn at oracle.com
serguei.spitsyn at oracle.com
Sat Jun 15 06:10:14 UTC 2019
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