RFR: JDK-8059586: hs_err report should treat redirected core pattern.
David Holmes
david.holmes at oracle.com
Mon Dec 1 06:15:54 UTC 2014
Hi Yasumasa,
On 30/11/2014 1:44 AM, Yasumasa Suenaga wrote:
> Hi all,
>
>
> Thank you for checking my patch!
> I've uploaded new webrev:
> http://cr.openjdk.java.net/~ysuenaga/JDK-8059586/webrev.03/hotspot.patch
>
> David:
>> The change in:
>> src/os/aix/vm/os_aix.cpp
>> src/os/solaris/vm/os_solaris.cpp
>>
>> jio_snprintf(buffer, bufferSize, "%s/core or core.%d",
>> current_process_id());
>>
>> has no argument for the %s - presumably p was intended.
>
> I've fixed.
Thanks. The formatting needs fixing up though, the p should line up with
buffer.
I'm concerned by the changes in os_linux.cpp and os_posix.cpp to use
os::malloc. If this is being called from a signal handler there's a real
risk of deadlock if we try to use malloc/free. I know Thomas suggested
this (and sorry I didn't notice it then) but I don't think it is a good
idea for the crash handler.
Thanks,
David
>
> Staffan:
>> src/os/bsd/vm/os_linux.cpp:
>> Could we not simplify this to print a helpful message instead?
>
> Most of case in Linux, I think that core image name is "core.<pid>" .
> In other case which except pipe redirection, I guess that user defines it.
> Thus I print string in kernel.core_pattern directly.
>
>> src/os/bsd/vm/os_bsd.cpp:
>> On OS X cores are by default written to /cores/core.<pid>. This is
>> configureable with the kern.corefile sysctl variable, although it is
>> rare to do so.
>
> Thank you!
> I changed path to "/cores/core.<pid>" .
>
>
> Thomas:
>> - jio_snprintf() returns -1 on truncation. n+=written may walk
>> backwards. I would probably check for (written >= 0) and also, at the
>> start of the loop, for (n < sizeof(core_path)).
>> - code is used in error reporting. I would be hesitant to create
>> larger buffers on the stack. malloc may be better.
>
> I've fixed them.
>
>> - code does not detect truncation of core_path (unlikely but possible)
>
> Do you mean variable name?
> "core_path" in my patch stores /proc/sys/kernel/core_pattern .
> Length of kernel.core_pattern is defined 128 chars in Linux Kernel
> Documentation.
> https://www.kernel.org/doc/Documentation/sysctl/kernel.txt
>
> Thus length of core_path (129 chars) is enough.
>
>> - when reading /proc/sys/kernel/core_uses_pid, using fgetc instead of
>> fgets may be a tiny bit simpler.
>
> I changed to use fgetc() .
>
>
> Thanks,
>
> Yasumasa
>
>
> (2014/11/26 23:12), Thomas Stüfe wrote:
>> Hi Yasumasa,
>>
>> I am not a Reviewer. Barring the general decision of the real
>> reviewers, here are some thoughts:
>>
>> os_linux.cpp
>>
>> - jio_snprintf() returns -1 on truncation. n+=written may walk
>> backwards. I would probably check for (written >= 0) and also, at the
>> start of the loop, for (n < sizeof(core_path)).
>> - code is used in error reporting. I would be hesitant to create
>> larger buffers on the stack. malloc may be better.
>> - code does not detect truncation of core_path (unlikely but possible)
>>
>> the rest is more matter of taste:
>> - I would prefer sizeof(core_path) over PATH_MAX at all places where
>> you refer to the size of the buffer. So you could make the buffer very
>> small and test e.g. how your code behaves with truncation.
>> - when reading /proc/sys/kernel/core_uses_pid, using fgetc instead of
>> fgets may be a tiny bit simpler.
>>
>> Kind Regards, Thomas
>>
>>
>>
>> On Wed, Nov 26, 2014 at 4:54 AM, Yasumasa Suenaga <yasuenag at gmail.com
>> <mailto:yasuenag at gmail.com>> wrote:
>>
>> Hi Staffan,
>>
>> Thank you for reviewing!
>>
>> os_linux.cpp:
>> I want to print coredump location correctly to hs_err. So I want
>> to output
>> whether coredump is processed in other process or is written to file.
>> If os::get_core_path() should be more simply, I will print raw
>> string in
>> core_pattern.
>>
>> os_bsd.cpp:
>> I don't have OS X. So I cannot check it.
>> I am focusing Linux in this enhancement. Could you file it as another
>> enhancement if it need?
>>
>> Thanks,
>>
>> Yasumasa
>>
>> 2014/11/25 18:15 "Staffan Larsen" <staffan.larsen at oracle.com
>> <mailto:staffan.larsen at oracle.com>>:
>>
>> > src/os/bsd/vm/os_linux.cpp:
>> > I’m inclined to think this is too complicated and hard to test and
>> > maintain (and I see no tests in the webrev). Could we not
>> simplify this to
>> > print a helpful message instead? Something that prints the
>> core_pattern and
>> > perhaps some of the values that could be used for substitution,
>> but does
>> > not do the actual substitution? I think that would go a long
>> way but be a
>> > lot more maintainable.
>> >
>> > src/os/bsd/vm/os_bsd.cpp:
>> > On OS X cores are by default written to /cores/core.<pid>. This is
>> > configureable with the kern.corefile sysctl variable, although
>> it is rare
>> > to do so.
>> >
>> > /Staffan
>> >
>> > > On 24 nov 2014, at 14:21, Yasumasa Suenaga
>> <yasuenag at gmail.com <mailto:yasuenag at gmail.com>> wrote:
>> > >
>> > > Hi all,
>> > >
>> > > I've uploaded webrev for this issue about a month ago.
>> > > Could you review it and sponsor it?
>> > >
>> > >
>> > > Thanks,
>> > >
>> > > Yasumasa
>> > >
>> > >
>> > > On 10/15/2014 11:13 PM, Yasumasa Suenaga wrote:
>> > >> Hi David,
>> > >>
>> > >> I've uploaded new webrev:
>> > >> http://cr.openjdk.java.net/~ysuenaga/JDK-8059586/webrev.02/
>> > >>
>> > >>
>> > >>> I wasn't suggesting that you make such a change though
>> because it is
>> > large and disruptive.
>> > >>
>> > >>> Unfactoring check_or_create_dump is a step backwards in
>> terms of code
>> > sharing.
>> > >>
>> > >> I restored check_or_create_dump() to os_posix.cpp .
>> > >> And I changed get_core_path() to create message which
>> represents core
>> > dump path
>> > >> (including filename) in each OS.
>> > >>
>> > >>
>> > >>> Expanding the get_core_path in os_linux.cpp to handle the
>> core_pattern
>> > may be okay (but I don't know enough about it to validate
>> everything).
>> > >>
>> > >> I implemented all parameters in Linux kernel documentation:
>> > >> https://www.kernel.org/doc/Documentation/sysctl/kernel.txt
>> > >>
>> > >> So I think that parameters which are processed are enough.
>> > >>
>> > >>
>> > >> Thanks,
>> > >>
>> > >> Yasumasa
>> > >>
>> > >>
>> > >>
>> > >> (2014/10/15 9:41), David Holmes wrote:
>> > >>> On 14/10/2014 8:05 PM, Yasumasa Suenaga wrote:
>> > >>>> Hi David,
>> > >>>>
>> > >>>> Thank you for comments!
>> > >>>> I've uploaded new webrev. Could you review it again?
>> > >>>> http://cr.openjdk.java.net/~ysuenaga/JDK-8059586/webrev.01/
>> > >>>>
>> > >>>> I am an author of jdk9. So I cannot commit it.
>> > >>>> Could you be a sponsor for this enhancement?
>> > >>>>
>> > >>>>
>> > >>>>> In which case that should be handled by the linux specific
>> > >>>>> get_core_path() function.
>> > >>>>
>> > >>>> Agree.
>> > >>>> So I implemented it in os_linux.cpp .
>> > >>>> But part of format characters (%P: global pid, %s: signal,
>> %t dump
>> > time)
>> > >>>> are not processed
>> > >>>> in this function because I think these parameters are
>> difficult to
>> > >>>> handle in it.
>> > >>>>
>> > >>>> %P: I could not find API for this.
>> > >>>> %s: We have to change arguments of get_core_path() .
>> > >>>> %t: This parameter means timestamp of coredump. It is
>> decided in
>> > Kernel.
>> > >>>>
>> > >>>>
>> > >>>>> Fixing this means changing all the os_posix using
>> platforms. But your
>> > >>>>> patch is not about this part. :)
>> > >>>>
>> > >>>> I moved os::check_or_create_dump() to each OS
>> implementations (AIX,
>> > BSD,
>> > >>>> Solaris, Linux) .
>> > >>>> So I can write Linux specific code to
>> check_or_create_dump() .
>> > >>>> As a result, I could remove "#ifdef LINUX" from
>> os_posix.cpp :-)
>> > >>>
>> > >>> I wasn't suggesting that you make such a change though
>> because it is
>> > large and disruptive. The simple handling of the | part of
>> core_pattern was
>> > basically ok. Expanding the get_core_path in os_linux.cpp to
>> handle the
>> > core_pattern may be okay (but I don't know enough about it to
>> validate
>> > everything). Unfactoring check_or_create_dump is a step
>> backwards in terms
>> > of code sharing.
>> > >>>
>> > >>> Sorry this has grown too large for me to deal with right now.
>> > >>>
>> > >>> David
>> > >>> -----
>> > >>>
>> > >>>>
>> > >>>>> Though I'm unclear whether it both invokes the program
>> and creates a
>> > >>>>> core dump file; or just invokes the program?
>> > >>>>
>> > >>>> If '|' is set, Linux kernel will just redirect core image
>> to user
>> > process.
>> > >>>> Kernel documentation says as below:
>> > >>>> ------------
>> > >>>> . If the first character of the pattern is a '|', the
>> kernel will
>> > treat
>> > >>>> the rest of the pattern as a command to run. The core
>> dump will be
>> > >>>> written to the standard input of that program instead of
>> to a file.
>> > >>>> ------------
>> > >>>>
>> > >>>> And implementation of coredump (do_coredump()) follows to it.
>> > >>>>
>> >
>> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/fs/coredump.c
>>
>> > >>>>
>> > >>>>
>> > >>>> In case of ABRT, ABRT dumps core image to default location
>> > >>>> (<CWD>/core.<PID>)
>> > >>>> if user set unlimited to resource limit of core (ulimit -c) .
>> > >>>>
>> https://github.com/abrt/abrt/blob/master/src/hooks/abrt-hook-ccpp.c
>> > >>>>
>> > >>>>
>> > >>>>> A few style nits - you need spaces around keywords and
>> before braces
>> > >>>>> I also suggest saying "Core dumps may be processed with
>> ..." rather
>> > >>>>> than "treated".
>> > >>>>> And as you don't do anything in the non-redirect case I
>> suggest
>> > >>>>> collapsing this:
>> > >>>>
>> > >>>> I've fixed them.
>> > >>>>
>> > >>>>
>> > >>>> Thanks,
>> > >>>>
>> > >>>> Yasumasa
>> > >>>>
>> > >>>>
>> > >>>> (2014/10/13 9:41), David Holmes wrote:
>> > >>>>> Hi Yasumasa,
>> > >>>>>
>> > >>>>> On 7/10/2014 8:48 PM, Yasumasa Suenaga wrote:
>> > >>>>>> Hi David,
>> > >>>>>>
>> > >>>>>> Sorry for my English.
>> > >>>>>>
>> > >>>>>> I want to propose that JVM should create message
>> according to core
>> > >>>>>> pattern (/proc/sys/kernel/core_pattern) .
>> > >>>>>> So I filed it to JBS and created a patch.
>> > >>>>>
>> > >>>>> So I've had a quick look at this core_pattern business
>> and it seems
>> > to
>> > >>>>> me that there are two aspects to this.
>> > >>>>>
>> > >>>>> First, without the leading |, the entry in the
>> core_pattern file is a
>> > >>>>> naming pattern for the core file. In which case that
>> should be
>> > handled
>> > >>>>> by the linux specific get_core_path() function. Though
>> that in itself
>> > >>>>> can't fully report the expected name, as part of it is
>> provided in
>> > the
>> > >>>>> shared code in os::check_or_create_dump. Fixing this
>> means changing
>> > >>>>> all the os_posix using platforms. But your patch is not
>> about this
>> > >>>>> part. :)
>> > >>>>>
>> > >>>>> Second, with a leading | the core_pattern is actually the
>> name of a
>> > >>>>> program to execute when the program is about to core
>> dump, and that
>> > is
>> > >>>>> what you report with your patch. Though I'm unclear
>> whether it both
>> > >>>>> invokes the program and creates a core dump file; or just
>> invokes the
>> > >>>>> program?
>> > >>>>>
>> > >>>>> So with regards to this second part your patch seems
>> functionally ok.
>> > >>>>> I do dislike having a big chunk of linux specific code in
>> this
>> > "posix"
>> > >>>>> support file but ...
>> > >>>>>
>> > >>>>> A few style nits - you need spaces around keywords and
>> before braces
>> > eg:
>> > >>>>>
>> > >>>>> if(x){
>> > >>>>>
>> > >>>>> should be
>> > >>>>>
>> > >>>>> if (x) {
>> > >>>>>
>> > >>>>> I also suggest saying "Core dumps may be processed with
>> ..." rather
>> > >>>>> than "treated".
>> > >>>>>
>> > >>>>> And as you don't do anything in the non-redirect case I
>> suggest
>> > >>>>> collapsing this:
>> > >>>>>
>> > >>>>> 83 is_redirect = core_pattern[0] == '|';
>> > >>>>> 84 }
>> > >>>>> 85
>> > >>>>> 86 if(is_redirect){
>> > >>>>> 87 jio_snprintf(buffer, bufferSize,
>> > >>>>> 88 "Core dumps may be treated with
>> \"%s\"",
>> > >>>>> &core_pattern[1]);
>> > >>>>> 89 }
>> > >>>>>
>> > >>>>> to just
>> > >>>>>
>> > >>>>> 83 if (core_pattern[0] == '|') { // redirect
>> > >>>>> 84 jio_snprintf(buffer, bufferSize, "Core
>> dumps may be
>> > >>>>> processed with \"%s\"", &core_pattern[1]);
>> > >>>>> 85 }
>> > >>>>> 86 }
>> > >>>>>
>> > >>>>> Comments from other runtime folk appreciated.
>> > >>>>>
>> > >>>>> Thanks,
>> > >>>>> David
>> > >>>>>
>> > >>>>>> Thanks,
>> > >>>>>>
>> > >>>>>> Yasumasa
>> > >>>>>>
>> > >>>>>> 2014/10/07 15:43 "David Holmes" <david.holmes at oracle.com
>> <mailto:david.holmes at oracle.com>
>> > >>>>>> <mailto:david.holmes at oracle.com
>> <mailto:david.holmes at oracle.com>>>:
>> > >>>>>>
>> > >>>>>> Hi Yasumasa,
>> > >>>>>>
>> > >>>>>> I'm sorry but I don't understand what you are
>> proposing. When you
>> > >>>>>> say
>> > >>>>>> "treat" do you mean "create"? Otherwise what do you
>> mean by
>> > >>>>>> "treated"?
>> > >>>>>>
>> > >>>>>> Thanks,
>> > >>>>>> David
>> > >>>>>>
>> > >>>>>> On 2/10/2014 8:38 AM, Yasumasa Suenaga wrote:
>> > >>>>>> > I'm in Hackergarten @ JavaOne :-)
>> > >>>>>> >
>> > >>>>>> >
>> > >>>>>> > Hi all,
>> > >>>>>> >
>> > >>>>>> > I would like to enhance the messages in hs_err
>> report.
>> > >>>>>> > Modern Linux kernel can treat core dump with user
>> process
>> > >>>>>> (e.g. ABRT)
>> > >>>>>> > However, hs_err report cannot detect it.
>> > >>>>>> >
>> > >>>>>> > I think that hs_err report should output messages
>> as below:
>> > >>>>>> > -------------
>> > >>>>>> > Failed to write core dump. Core dumps may be
>> treated with
>> > >>>>>> "/usr/sbin/chroot /proc/%P/root
>> /usr/libexec/abrt-hook-ccpp %s
>> > %c %p
>> > >>>>>> %u %g %t e"
>> > >>>>>> > -------------
>> > >>>>>> >
>> > >>>>>> > I've uploaded webrev of this enhancement.
>> > >>>>>> > Could you review it?
>> > >>>>>> >
>> > >>>>>> >
>> http://cr.openjdk.java.net/~ysuenaga/JDK-8059586/webrev.00/
>> > >>>>>> >
>> > >>>>>> > This patch works fine on Fedora20 x86_64.
>> > >>>>>> >
>> > >>>>>> >
>> > >>>>>> >
>> > >>>>>> > Thanks,
>> > >>>>>> >
>> > >>>>>> > Yasumasa
>> > >>>>>> >
>> > >>>>>>
>> >
>> >
>>
>>
More information about the hotspot-runtime-dev
mailing list