2016/09/09 16:02 "serguei.spit...@oracle.com" <serguei.spit...@oracle.com>: > > On 9/8/16 22:46, David Holmes wrote: >> >> On 9/09/2016 8:48 AM, Yasumasa Suenaga wrote: >>>> >>>> Could you, please, send a full webrev for this last suggestion? >>> >>> >>> I uploaded a webrev. Could you review it? >>> >>> http://cr.openjdk.java.net/~ysuenaga/JDK-8164913/webrev.03/ >> >> >> This minimalist change looks okay to me. > > > Dmitry was initially for this approach. > I'm pushing the fix now.
Thanks! Yasumasa > Thanks, > Serguei > > >> >> Thanks, >> David >> ----- >> >>> >>>>> We still cannot know the reason of attach failure. If jvmtiExport.cpp >>>>> will be changed in above, I will file it as other issue on JBS. >>>> >>>> >>>> Agreed, it is better to separate this issue. >>> >>> >>> I've filed: >>> >>> https://bugs.openjdk.java.net/browse/JDK-8165736 >>> >>> After JDK-8164913, I'll create a patch. >>> And I will send review request after jdk10 repos is opened. >>> >>> >>> Thanks, >>> >>> Yasumasa >>> >>> >>> On 2016/09/09 2:37, serguei.spit...@oracle.com wrote: >>>> >>>> Hi Yasumasa, >>>> >>>> On 9/8/16 06:39, Yasumasa Suenaga wrote: >>>>> >>>>> Hi Dmitry, >>>>> >>>>> for jvmtiExport.cpp , can we change as below? >>>>> ---------------------- >>>>> --- a/src/share/vm/prims/jvmtiExport.cpp Wed Sep 07 23:17:24 >>>>> 2016 +0200 >>>>> +++ b/src/share/vm/prims/jvmtiExport.cpp Thu Sep 08 22:34:01 >>>>> 2016 +0900 >>>>> @@ -2407,9 +2407,7 @@ >>>>> delete agent_lib; >>>>> } >>>>> >>>>> - // Agent_OnAttach executed so completion status is JNI_OK >>>>> st->print_cr("%d", result); >>>>> - result = JNI_OK; >>>>> } >>>>> } >>>>> return result; >>>>> ---------------------- >>>>> >>>>> At least, we can get error code if agent attach is failed. >>>>> If it is acceptable, I remove the change for HotSpotVirtualMacine.java . >>>> >>>> >>>> This would be nice. >>>> Could you, please, send a full webrev for this last suggestion? >>>> >>>>> >>>>> We still cannot know the reason of attach failure. If jvmtiExport.cpp >>>>> will be changed in above, I will file it as other issue on JBS. >>>> >>>> >>>> Agreed, it is better to separate this issue. >>>> >>>> >>>> Thanks, >>>> Serguei >>>> >>>>> >>>>> >>>>> Thanks, >>>>> >>>>> Yasumasa >>>>> >>>>> >>>>> On 2016/09/08 21:37, Dmitry Samersoff wrote: >>>>>> >>>>>> Yasumasa, >>>>>> >>>>>> jvmtiExport.cpp: >>>>>> >>>>>> I'm reluctant to change printing drastically at this stage of jdk9. >>>>>> So it might be better to refactor the code to keep only one line in >>>>>> output and print either >>>>>> st->print_cr("return code: %d", result); >>>>>> or >>>>>> st->print_cr("%s was not loaded because of %s", ebuf); >>>>>> >>>>>> >>>>>> Pending exception situation is less clean for me. We continue to load >>>>>> agent ever if an exception happens. I would recommend to leave the code >>>>>> as is and address this issue separately. >>>>>> >>>>>> >>>>>> diagnosticCommand.cpp: >>>>>> No comments >>>>>> >>>>>> HotSpotVirtualMachine.java: >>>>>> >>>>>> 68 Agent_OnAttach failed: " + result >>>>>> We know that result is null here. >>>>>> >>>>>> 71 We don't need to use regex here. >>>>>> >>>>>> The only case where agent is loaded correctly returns exactly >>>>>> >>>>>> "return code: 0" >>>>>> >>>>>> so we can check for presence of this string to return OK and throw an >>>>>> exception with a message read from agent in all other case. >>>>>> >>>>>> -Dmitry >>>>>> >>>>>> >>>>>> On 2016-09-08 14:28, Yasumasa Suenaga wrote: >>>>>>> >>>>>>> Hi David, >>>>>>> >>>>>>>> This one is tricky to get indentation reasonable but at the moment it >>>>>>>> seems inconsistent maybe something like: >>>>>>> >>>>>>> >>>>>>> Thanks! >>>>>>> I will fix it after discussion on [1]. >>>>>>> >>>>>>> >>>>>>>> You know result is null here. >>>>>>>> >>>>>>>> 74 int retCode = >>>>>>>> Integer.parseInt(matcher.group(1)); >>>>>>>> >>>>>>>> This can throw NumberFormatException which is not declared for this >>>>>>>> method. The original code wraps this in an IOException. >>>>>>> >>>>>>> >>>>>>> >>>>>>> My patch uses regex, and checks the match before getting the value: >>>>>>> ----------------- >>>>>>> + Matcher matcher = Pattern.compile("^return code: >>>>>>> (\\d+)$") >>>>>>> + .matcher(result); >>>>>>> + if (matcher.matches()) { >>>>>>> + int retCode = Integer.parseInt(matcher.group(1)); >>>>>>> + if (retCode != 0) { >>>>>>> + throw new AgentInitializationException( >>>>>>> + "Agent_OnAttach >>>>>>> failed", >>>>>>> retCode); >>>>>>> + } >>>>>>> + } >>>>>>> ----------------- >>>>>>> >>>>>>> So I think matcher.group(1) will not return null. >>>>>>> >>>>>>> >>>>>>> Thanks, >>>>>>> >>>>>>> Yasumasa >>>>>>> >>>>>>> >>>>>>> [1] >>>>>>> http://mail.openjdk.java.net/pipermail/serviceability-dev/2016-September/020385.html >>>>>>> >>>>>>> >>>>>>> >>>>>>> >>>>>>> On 2016/09/08 19:18, David Holmes wrote: >>>>>>>> >>>>>>>> On 8/09/2016 1:47 PM, Yasumasa Suenaga wrote: >>>>>>>>> >>>>>>>>> Hi all, >>>>>>>>> >>>>>>>>> This changes has been approved. But it was not passed JPRT. >>>>>>>>> It is caused by checking return value in >>>>>>>>> HotSpotVirtualMachine#loadAgentLibrary(). >>>>>>>>> >>>>>>>>> I've fixed it and uploaded webrev. This changes has been passed >>>>>>>>> JPRT. >>>>>>>>> (Thanks David!) >>>>>>>>> Could you review again? >>>>>>>>> >>>>>>>>> hotspot: >>>>>>>>> http://cr.openjdk.java.net/~ysuenaga/JDK-8164913/webrev.02/hotspot/ >>>>>>>> >>>>>>>> >>>>>>>> Looks ok. >>>>>>>> >>>>>>>>> jdk: >>>>>>>>> http://cr.openjdk.java.net/~ysuenaga/JDK-8164913/webrev.02/jdk/ >>>>>>>> >>>>>>>> >>>>>>>> src/jdk.attach/share/classes/sun/tools/attach/HotSpotVirtualMachine.java >>>>>>>> >>>>>>>> >>>>>>>> 63 try(BufferedReader reader = new BufferedReader(new >>>>>>>> InputStreamReader( >>>>>>>> 64 execute("load", agentLibrary, >>>>>>>> 65 >>>>>>>> Boolean.toString(isAbsolute), options)))) { >>>>>>>> >>>>>>>> This one is tricky to get indentation reasonable but at the moment it >>>>>>>> seems inconsistent maybe something like: >>>>>>>> >>>>>>>> try (BufferedReader reader = >>>>>>>> new BufferedReader( >>>>>>>> new InputStreamReader( >>>>>>>> execute("load", >>>>>>>> agentLibrary, >>>>>>>> >>>>>>>> Boolean.toString(isAbsolute), options) >>>>>>>> ))) { >>>>>>>> >>>>>>>> I can't get the above to display correctly in my mail client but >>>>>>>> hopefully you get the idea. Also note space after 'try'. >>>>>>>> >>>>>>>> 67 if (result == null) { >>>>>>>> 68 throw new AgentInitializationException( >>>>>>>> 69 "Agent_OnAttach failed: " + >>>>>>>> result); >>>>>>>> >>>>>>>> You know result is null here. >>>>>>>> >>>>>>>> 74 int retCode = >>>>>>>> Integer.parseInt(matcher.group(1)); >>>>>>>> >>>>>>>> This can throw NumberFormatException which is not declared for this >>>>>>>> method. The original code wraps this in an IOException. >>>>>>>> >>>>>>>> Thanks, >>>>>>>> David >>>>>>>> >>>>>>>>> >>>>>>>>> Thanks, >>>>>>>>> >>>>>>>>> Yasumasa >>>>>>>>> >>>>>>>>> >>>>>>>>> On 2016/09/07 16:47, serguei.spit...@oracle.com wrote: >>>>>>>>>> >>>>>>>>>> On 9/7/16 00:45, Dmitry Samersoff wrote: >>>>>>>>>>> >>>>>>>>>>> Serguei, >>>>>>>>>>> >>>>>>>>>>> I'm OK with the fix and OK to be listed as a reviewer. >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> Thanks, Dmitry! >>>>>>>>>> Serguei >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> -Dmitry >>>>>>>>>>> >>>>>>>>>>> On 2016-09-07 07:08, serguei.spit...@oracle.com wrote: >>>>>>>>>>>> >>>>>>>>>>>> Hi Dmitry, >>>>>>>>>>>> >>>>>>>>>>>> As a sponsor I'm going to push this fix if you are Ok with the >>>>>>>>>>>> fix. >>>>>>>>>>>> Please, let me know if you still have any concerns. >>>>>>>>>>>> Also, confirm if you are Ok to be in the list of reviewers. >>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>>> On 9/5/16 06:46, Dmitry Samersoff wrote: >>>>>>>>>>>>> >>>>>>>>>>>>> Yasumasa, >>>>>>>>>>>>> >>>>>>>>>>>>> I'll look closely to the fix. >>>>>>>>>>>>> >>>>>>>>>>>>> Please, notice: >>>>>>>>>>>>> >>>>>>>>>>>>> 1. We typically avoid printing attach error messages on >>>>>>>>>>>>> the target VM side. >>>>>>>>>>>> >>>>>>>>>>>> Some message was already printed: >>>>>>>>>>>> >>>>>>>>>>>> 2410 // Agent_OnAttach executed so completion status is JNI_OK >>>>>>>>>>>> 2411 st->print_cr("%d", result); >>>>>>>>>>>> 2412 result = JNI_OK; >>>>>>>>>>>> >>>>>>>>>>>> It is just a replacement. :) >>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>>>> 2. We are in RDP1 for jdk9. >>>>>>>>>>>>> >>>>>>>>>>>>> >>>>>>>>>>>>> http://mail.openjdk.java.net/pipermail/jdk9-dev/2016-August/004777.html >>>>>>>>>>>>> >>>>>>>>>>>>> >>>>>>>>>>>>> >>>>>>>>>>>> I've set the priority to P3, so it can be pushed to the jdk9/hs. >>>>>>>>>>>> >>>>>>>>>>>> Thanks, >>>>>>>>>>>> Serguei >>>>>>>>>>>> >>>>>>>>>>>>> -Dmitry >>>>>>>>>>>>> >>>>>>>>>>>>> On 2016-09-05 16:25, Yasumasa Suenaga wrote: >>>>>>>>>>>>>> >>>>>>>>>>>>>> PING: Could you review and sponsor it? >>>>>>>>>>>>>> >>>>>>>>>>>>>>> http://cr.openjdk.java.net/~ysuenaga/JDK-8164913/webrev.01/ >>>>>>>>>>>>>> >>>>>>>>>>>>>> Thanks, >>>>>>>>>>>>>> >>>>>>>>>>>>>> Yasumasa >>>>>>>>>>>>>> >>>>>>>>>>>>>> >>>>>>>>>>>>>> On 2016/09/01 12:47, Yasumasa Suenaga wrote: >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> Hi all, >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> I think RDP1 has been started. >>>>>>>>>>>>>>> Cannot I fix this? >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> This problem is that jcmd shows incorrect status when JVMTI >>>>>>>>>>>>>>> agent >>>>>>>>>>>>>>> cannot be attached. >>>>>>>>>>>>>>> I think this problem should be fixed in 9 GA. >>>>>>>>>>>>>>> The users who want.to <http://want.to> attach JVMTI agent >>>>>>>>>>>>>>> want to >>>>>>>>>>>>>>> know >>>>>>>>>>>>>>> whether it succeed. >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> Yasumasa >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> 2016/08/29 15:42 "Yasumasa Suenaga" <yasue...@gmail.com >>>>>>>>>>>>>>> <mailto:yasue...@gmail.com>>: >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> This comment no longer matches the code and should be >>>>>>>>>>>>>>> deleted: >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> 2412 // Agent_OnAttach executed so completion >>>>>>>>>>>>>>> status is >>>>>>>>>>>>>>> JNI_OK >>>>>>>>>>>>>>> 2413 st->print_cr("return code: %d", result); >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> Thanks David! >>>>>>>>>>>>>>> I removed it in new webrev. >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> http://cr.openjdk.java.net/~ysuenaga/JDK-8164913/webrev.01/ >>>>>>>>>>>>>>> <http://cr.openjdk.java.net/~ysuenaga/JDK-8164913/webrev.01/> >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> Yasumasa >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> On 2016/08/29 12:59, David Holmes wrote: >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> Hi Yasumasa, >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> On 28/08/2016 10:47 PM, Yasumasa Suenaga wrote: >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> Hi all, >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> If we try to attach invalid JVMTI agent via >>>>>>>>>>>>>>> JVMTI.agent_load dcmd, we >>>>>>>>>>>>>>> will get >>>>>>>>>>>>>>> "Command executed successfully". However, it >>>>>>>>>>>>>>> implies >>>>>>>>>>>>>>> error in >>>>>>>>>>>>>>> JVMTIAgentLoadDCmd. >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> This message is from JCmd.java when jcmd does not >>>>>>>>>>>>>>> receive >>>>>>>>>>>>>>> output from >>>>>>>>>>>>>>> target VM. >>>>>>>>>>>>>>> So we should send error message from >>>>>>>>>>>>>>> JVMTIAgentLoadDCmd. >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> I uploaded a webrev for it. Could you review it? >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> http://cr.openjdk.java.net/~ysuenaga/JDK-8164913/webrev.00/ >>>>>>>>>>>>>>> <http://cr.openjdk.java.net/~ysuenaga/JDK-8164913/webrev.00/> >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> This seems reasonable. >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> src/share/vm/prims/jvmtiExport.cpp >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> This comment no longer matches the code and should be >>>>>>>>>>>>>>> deleted: >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> 2412 // Agent_OnAttach executed so completion >>>>>>>>>>>>>>> status is >>>>>>>>>>>>>>> JNI_OK >>>>>>>>>>>>>>> 2413 st->print_cr("return code: %d", result); >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> Thanks, >>>>>>>>>>>>>>> David >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> I cannot access JPRT. >>>>>>>>>>>>>>> So I need a sponsor. >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> Thanks, >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> Yasumasa >>>>>>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>> >>>>>> >>>>>> >>>> >
