RFR: JDK-8164913: JVMTI.agent_load dcmd should show useful error message

[Yasumasa Suenaga]

Hi Serguei,

I agree to you to reduce the risk at the RDP1.
However I think we should fix jvmtiExport.cpp .

Currently, jvmtiExport.cpp always returns JNI_OK even if Agent_OnAttach() is failed.
I think it is useful to obtain the reason of attach failure.

To reduce the risk, we can change the format of return code as below:

http://cr.openjdk.java.net/~ysuenaga/JDK-8164913/webrev.02/hotspot/src/share/vm/prims/jvmtiExport.cpp.udiff.html
------------------
+      st->print_cr("return code: %d", result); <- revert (st->print_cr("%d", result))
------------------


HotSpotVirtualMachine checks the result code from the stream.
So it works fine if we revert this change.

What do you think about it?


Thanks,

Yasumasa


On 2016/09/08 19:09, serguei.spitsyn at oracle.com wrote:
> Hi Yasumasa,
>
> We are expected to reduce the risk at the RDP1, not to extend.
> So that I'd suggest to reduce you fix instead of extending.
> Is it possible to achieve your JVMTI.agent_load dcmd goal without
> additional changes in the JvmtiExport::load_agent_library and
> the HotSpotVirtualMachine.java ?
> Or at least, could we minimize the impact?
>
> I do not like the update in the HotSpotVirtualMachine.java
> as it does not look as necessary to fix the bug 8164913.
>
> Thanks,
> Serguei
>
>
> On 9/7/16 20:47, 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/
>>       jdk: http://cr.openjdk.java.net/~ysuenaga/JDK-8164913/webrev.02/jdk/
>>
>>
>> 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
>>>>>>>>
>>>>
>>>
>