RFR: JDK-8164913: JVMTI.agent_load dcmd should show useful error message
serguei.spitsyn at oracle.com
serguei.spitsyn at oracle.com
Fri Sep 9 07:02:16 UTC 2016
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,
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
>>>>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>
>>>>>
>>>>>
>>>
More information about the serviceability-dev
mailing list