RFR: JDK-8164913: JVMTI.agent_load dcmd should show useful error message
Yasumasa Suenaga
yasuenag at gmail.com
Thu Sep 8 22:48:35 UTC 2016
> 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/
>> 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.spitsyn at 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.spitsyn at 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.spitsyn at 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" <yasuenag at gmail.com
>>>>>>>>>>>> <mailto:yasuenag at 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
>>>>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>
>>>
>
More information about the serviceability-dev
mailing list