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> 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)