RFR: JDK-8164913: JVMTI.agent_load dcmd should show useful error message
serguei.spitsyn at oracle.com
serguei.spitsyn at oracle.com
Thu Sep 8 17:37:08 UTC 2016
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