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.
Thanks,
Yasumasa
2016-01-20 18:06 GMT+09:00 Yasumasa Suenaga <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>:
>
> 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.
>>
>
