RFR: JDK-8164913: JVMTI.agent_load dcmd should show useful error message
Yasumasa Suenaga
yasuenag at gmail.com
Fri Sep 9 09:20:58 UTC 2016
2016/09/09 16:02 "serguei.spitsyn at oracle.com" <serguei.spitsyn at 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.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
>>>>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>
>>>>>>
>>>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20160909/bc21d0ab/attachment-0001.html>
More information about the serviceability-dev
mailing list