> On 22 jan. 2016, at 04:21, Yasumasa Suenaga <yasue...@gmail.com> wrote:
>
> Hi,
>
> I think that we can check malloc error as below:
> --------------
> int opt_len = strlen(_libpath.value()) + strlen(_option.value()) + 2;
> char *opt = (char *)os::malloc(opt_len, mtInternal);
> if (opt == NULL) {
> // 4096 comes from PATH_MAX at Linux.
> if (opt_len > 4096) {
> output()->print_cr("JVMTI agent attach failed: "
> "Could not allocate %d bytes for argument.",
> opt_len);
> } else {
> // VM might not work because memory exhausted.
> vm_exit_out_of_memory(opt_len, OOM_MALLOC_ERROR,
> "JVMTIAgentLoadDCmd::execute");
> }
> }
> --------------
>
> If you think that this code should NOT be available in dcmd, I will remove
> vm_exit_out_of_memory() from it.
Yes, please remove it and use output()->print() to signal an error instead.
/Staffan
>
>
> Thanks,
>
> Yasumasa
>
>
> 2016-01-20 18:06 GMT+09:00 Yasumasa Suenaga <yasue...@gmail.com
> <mailto:yasue...@gmail.com>>:
> I agree to Dmitry.
>
> Most case of malloc error, native memory is exhausted.
> Thus I think process (or system) is illegal state. It should be shut down.
> If do not so, malloc failure might be occurred another point.
>
> However, I think that it is very difficult to set the threshold.
> If it can be clear, I will make a new webrev.
>
> Thanks,
>
> Yasumasa
>
> 2016/01/20 17:03 "Dmitry Samersoff" <dmitry.samers...@oracle.com
> <mailto:dmitry.samers...@oracle.com>>:
>
> 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
> >>
> >> <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>
> >> <mailto: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/
> >> <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
> >> <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
> >>
> >> <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
> >> <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/
> >> <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>
> >>
> >> <mailto: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/
> >> <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
> >> <http://icedtea.classpath.org/wiki/HeapStats>
> >> [2]
> >>
> >> https://github.com/YaSuenag/jlivepatcher
> >> <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.
>
