Yasumasa, Looks good for me.
-Dmitry On 2016-01-19 16: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 >>>>>>>>>>> <yasue...@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) >>>>>>>>>> >>>>>>>> >>>>>>>> >>>>>> >>>>>> >>>> >>>> >> >> -- Dmitry Samersoff Oracle Java development team, Saint Petersburg, Russia * I would love to change the world, but they won't give me the sources.