PING: RFR: 8209790: SA tools not providing option to connect to debug server
David Holmes
david.holmes at oracle.com
Mon Jun 17 12:42:29 UTC 2019
Hi Yasumasa,
On 17/06/2019 6:50 pm, Yasumasa Suenaga wrote:
> Hi David,
>
> 8209790 is filed as a bug.
I don't agree this is a "bug" - sorry. For this to be a bug there must
be some specification of behaviour that the implementation is violating.
Is that the case? To me this is missing functionality which makes it an
enhancement.
> 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.
Just to clarify the process here, you don't want to set the "Fix
Version" to 13 or 14 in JBS before pushing. That will be set based on
the repo you push to. If you push to jdk/jdk it will be 14. If you push
to jdk/jdk13 it will be 13. Any fix pushed to jdk/jdk13 will be
"automatically" forward-ported to jdk/jdk and thus 14.
David
-----
> (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