RFR: JDK-8164913: JVMTI.agent_load dcmd should show useful error message
David Holmes
david.holmes at oracle.com
Fri Sep 9 05:46:55 UTC 2016
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.
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