RFR: JDK-8164913: JVMTI.agent_load dcmd should show useful error message
Yasumasa Suenaga
yasuenag at gmail.com
Thu Sep 8 13:39:26 UTC 2016
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 .
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.
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