Hi Serguei,
Thank you for your comment.
I will fix them before creating changeset.
Thanks,
Yasumasa
2016/01/20 4:04 "serguei.spit...@oracle.com" <serguei.spit...@oracle.com>:
> 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 <yasue...@gmail.com>
> <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)
>
>
