David, PS: It might be a good to check opt_len for some large enough value (like 2048) before allocation attempt and send back a message.
-Dmitry On 2016-01-20 08:03, David Holmes wrote: > On 20/01/2016 9:13 AM, Yasumasa Suenaga wrote: >> Hi David, >> >> ShouldNotReachHere( ) is called at NMTDcmd: >> http://hg.openjdk.java.net/jdk9/hs-rt/hotspot/file/8fcd5cba7938/src/share/vm/services/nmtDCmd.cpp#l156 >> > > Sure but that is more of a debugging check - we never expect to reach > that code. > > Unfortunately I do see some other implicit aborts due to allocation > failures, but I have to say this seems very wrong to me. > > David > ----- > >> If target VM is aborted, caller (e.g. jcmd) cannot receive response. I >> think that the caller show Exception. >> >> Thanks, >> >> Yasumasa >> >> 2016/01/20 7:37 "David Holmes" <david.hol...@oracle.com >> <mailto:david.hol...@oracle.com>>: >> >> On 19/01/2016 11:19 PM, 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(). >> >> >> That seems rather extreme - do other dcmd failures abort the VM? I >> would have expected some kind of error propagation back to the >> caller. >> >> Thanks, >> David >> >> 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 >> >> <mailto: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.
