RFR: JDK-8147388: Add diagnostic commands to attach JVMTI agent.

Tue Jan 19 19:04:32 UTC 2016

Hi Yasumasa,

This looks pretty good.
Thank you for the enhancement!

A couple of minor comments.

src/share/vm/prims/jvmtiExport.cp

+jint JvmtiExport::load_agent_library(const char *agent, const char 
*absParam,
+ const char *options, outputStream* st) {

The indent needs to be corrected.

test/serviceability/dcmd/jvmti/LoadAgentDcmdTest.java

The indent is not consistent:

   50             throw new RuntimeException(
   51                     "System property 'test.jdk' not set. " +
  . . .
   61             throw new FileNotFoundException(
   62                                   "Could not find " + libpath.toAbsolutePath());

I do not need to see another webrev if you resolve the indent comments. 
Thanks, Serguei On 1/19/16 05:19, Yasumasa Suenaga wrote:
> Hi, I uploaded a new webrev:   
> http://cr.openjdk.java.net/~ysuenaga/JDK-8147388/webrev.03/ It is 
> malloc/free version. If NULL returns from malloc(), it calls 
> vm_exit_out_of_memory(). All test about this changes work fine. Please 
> review. Thanks, Yasumasa On 2016/01/19 22:07, Dmitry Samersoff wrote:
>> David,
>>> Anytime we start to use a language feature for the first time we 
>>> need to be extra diligent to make sure there are no unintended 
>>> side-effects and that all our supported compilers (and probably a 
>>> few others used in the community) do the right thing. A bit of 
>>> googling seems to indicate that variable length arrays are part of 
>>> C99 but are not allowed in C++ - though gcc has an extension that 
>>> does allow them. 
>> I hear your concern. Moreover I revisit my advice and think it's not 
>> a good idea to do a stack allocation based on unverified user input. 
>> Yasumasa, sorry for leading in wrong direction. Lets use malloc/free 
>> in this case. Nevertheless, I would like to start broader evaluation 
>> of possible use on-stack allocation (either alloca or VLA) - hotspot 
>> may benefit of it in many places. -Dmitry On 2016-01-19 02:06, David 
>> Holmes wrote:
>>> On 19/01/2016 7:26 AM, Dmitry Samersoff wrote:
>>>> David, On 2016-01-18 23:47, David Holmes wrote:
>>>>> On 18/01/2016 11:20 PM, Dmitry Samersoff wrote:
>>>>>> Yasumasa,
>>>>>>> Can we use VLA (Variable Length Arrays) ? 
>>>>>> Apple LLVM version 6.1.0 (clang-602.0.53) (based on LLVM 
>>>>>> 3.6.0svn) Target: x86_64-apple-darwin14.5.0 Thread model: posix 
>>>>>> Compiles it just fine. 
>>>>> Are we using variable length arrays anywhere else in the VM yet? 
>>>> Probably not. But personally, I see no reason to don't use it for 
>>>> simple cases like this one. 
>>> Anytime we start to use a language feature for the first time we 
>>> need to be extra diligent to make sure there are no unintended 
>>> side-effects and that all our supported compilers (and probably a 
>>> few others used in the community) do the right thing. A bit of 
>>> googling seems to indicate that variable length arrays are part of 
>>> C99 but are not allowed in C++ - though gcc has an extension that 
>>> does allow them. This reports they are not available in Visual 
>>> Studio C++: https://msdn.microsoft.com/en-us/library/zb1574zs.aspx 
>>> David -----
>>>>> What are the implications for allocation and in particular 
>>>>> allocation failure? 
>>>> This allocation just reserves some space on the stack[1], so it can 
>>>> cause stack overflow if we attempt to allocate two much bytes. 
>>>> 1. Listing fragment (extra labels are removed) 3                    
>>>> .Ltext0: 5                    .LC0: 14:test.cxx      **** void 
>>>> testme(int n) { 15:test.cxx      **** char m[n]; 25 0000 
>>>> 4863FF                movslq  %edi, %rdi 28 0003 
>>>> 55                    pushq   %rbp 37 0004 BE000000 movl    $.LC0, 
>>>> %esi 41 0009 4883C70F              addq    $15, %rdi 46 000d 
>>>> 31C0                  xorl    %eax, %eax 50 000f 
>>>> 4883E7F0              andq    $-16, %rdi 54 0013 4889E5 movq    
>>>> %rsp, %rbp 59 0016 4829FC                subq    %rdi, %rsp 64 0019 
>>>> BF010000              movl    $1, %edi 65 001e 4889E2 movq    %rsp, 
>>>> %rdx 66 0021 E8000000              call __printf_chk 
>>>> 16:test.cxx      ****   printf("%s", m); 17:test.cxx **** } -Dmitry
>>>>> David -----
>>>>>> -Dmitry On 2016-01-18 16:09, Yasumasa Suenaga wrote:
>>>>>>> Hi Dmitry,
>>>>>>>> 1. It might be better to have one jcmd to run both java and 
>>>>>>>> native java agent. If agent library name ends with ".jar" we 
>>>>>>>> can assume it's java agent. 
>>>>>>> Okay, I'll try it.
>>>>>>>> if (_libpath.value() == NULL) { error ... } 
>>>>>>> I will add it. However, I note you that _libpath is given 
>>>>>>> mandatory flag. 
>>>>>>> http://mail.openjdk.java.net/pipermail/serviceability-dev/2016-January/018661.html 
>>>>>>>
>> char options[option_len];
>>>>>>> Can we use VLA (Variable Length Arrays) ? I'm worried that 
>>>>>>> several C++ compiler cannot compile this code. 
>>>>>>> http://clang.llvm.org/compatibility.html#vla Thanks, Yasumasa On 
>>>>>>> 2016/01/18 19:38, Dmitry Samersoff wrote:
>>>>>>>> Yasumasa, 1. It might be better to have one jcmd to run both 
>>>>>>>> java and native java agent. If agent library name ends with 
>>>>>>>> ".jar" we can assume it's java agent. 2. Please get rid of 
>>>>>>>> malloc/free and check _libpath.value() for NULL at ll. 295 and 
>>>>>>>> below. if (_libpath.value() == NULL) { error ... } if 
>>>>>>>> (_option.value() == NULL) { 
>>>>>>>> JvmtiExport::load_agent_library("instrument", "false", 
>>>>>>>> _libpath.value(), output()); return; } size_t option_len = \ 
>>>>>>>> strlen(_libpath.value()) + strlen(_option.value()) + 1; char 
>>>>>>>> options[option_len]; .... -Dmitry On 2016-01-15 16:33, Yasumasa 
>>>>>>>> Suenaga wrote:
>>>>>>>>> Hi, I added permissions and tests in new webrev: 
>>>>>>>>> http://cr.openjdk.java.net/~ysuenaga/JDK-8147388/webrev.01/ 
>> Two tests (LoadAgentDcmdTest, LoadJavaAgentDcmdTest) work fine.
>>>>>>>>> Thanks, Yasumasa On 2016/01/15 17:20, Staffan Larsen wrote:
>>>>>>>>>> This is a good improvement overall. The new diagnostic 
>>>>>>>>>> commands need to have proper permissions set: static const 
>>>>>>>>>> JavaPermission permission() { JavaPermission p = 
>>>>>>>>>> {"java.lang.management.ManagementPermission", “control", 
>>>>>>>>>> NULL}; return p; } And as David said: tests! See 
>>>>>>>>>> hotspot/test/serviceability/dcmd/jvmti. Thanks, /Staffan
>>>>>>>>>>> On 14 jan. 2016, at 15:00, Yasumasa Suenaga 
>>>>>>>>>>> <yasuenag at gmail.com> wrote: Hi all, We can use Attach API to 
>>>>>>>>>>> attach JVMTI agent to live process. However, we have to 
>>>>>>>>>>> write Java code for it. If we can attach JVMTI agents 
>>>>>>>>>>> through jcmd, it is very useful. So I want to add two new 
>>>>>>>>>>> diagnostic commands: * JVMTI.agent_load: Load JVMTI native 
>>>>>>>>>>> agent. * JVMTI.javaagent_load: Load JVMTI java agent. I 
>>>>>>>>>>> maintain two JVMTI agents - HeapStats [1] and JLivePatcher 
>>>>>>>>>>> [2]. [1] is native agent, [2] is java agent. They provide a 
>>>>>>>>>>> program for attaching to live process. I guess that various 
>>>>>>>>>>> JVMTI agents provide own attach mechanism like them. I think 
>>>>>>>>>>> that we should provide general way to attach. I've uploaded 
>>>>>>>>>>> webrev. Could you review it? 
>>>>>>>>>>> http://cr.openjdk.java.net/~ysuenaga/JDK-8147388/webrev.00/ 
>> I'm jdk9 committer, however I cannot access JPRT.
>>>>>>>>>>> So I need a sponsor. Thanks, Yasumasa [1] 
>>>>>>>>>>> http://icedtea.classpath.org/wiki/HeapStats [2] 
>>>>>>>>>>> https://github.com/YaSuenag/jlivepatcher  (in Japanese) 
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20160119/01e6aa53/attachment.html>