This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH 1/4] Implement 'set honor-dontdump-flag' command
- From: Sergio Durigan Junior <sergiodj at redhat dot com>
- To: Sergio Lopez <slp at redhat dot com>
- Cc: gdb-patches at sourceware dot org
- Date: Tue, 28 Nov 2017 10:42:13 -0500
- Subject: Re: [PATCH 1/4] Implement 'set honor-dontdump-flag' command
- Authentication-results: sourceware.org; auth=none
- References: <20171128132148.31802-1-slp@redhat.com> <20171128132148.31802-2-slp@redhat.com>
On Tuesday, November 28 2017, Sergio Lopez wrote:
> Commit df8411da087dc05481926f4c4a82deabc5bc3859 implemented support for
> checking /proc/PID/coredump_filter, and also changed gcore behavior to
> unconditionally honor the VM_DONTDUMP flag, preventing sections marked
> as such for being dumped into the core file.
>
> This patch adds support for the 'set honor-dontdump-flag' command for
> instructing gdb to ignore the VM_DONTDUMP flag. Combined with 'set
> use-coredump-filter', this allows the user to restore the old behavior,
> dumping all sections (except the ones marked as IO) unconditionally.
Thanks for the patch. A few comments below.
> ChangeLog
This should say "gdb/ChangeLog:".
> 2017-11-28 Sergio Lopez <slp@redhat.com>
>
> * linux-tdep.c (honor_dontdump_flag): New variable.
> (dump_mapping_p): Use honor_dontdump_flag variable.
> (_initialize_linux_tdep): New command 'set honor-dontdump-flag'.
Not sure if it was your MUA, but ChangeLog lines should be indented
using TABs.
> ---
> gdb/ChangeLog | 6 ++++++
> gdb/linux-tdep.c | 19 ++++++++++++++++++-
> 2 files changed, 24 insertions(+), 1 deletion(-)
>
> diff --git a/gdb/ChangeLog b/gdb/ChangeLog
> index ebb969998c..ce68fee776 100644
> --- a/gdb/ChangeLog
> +++ b/gdb/ChangeLog
> @@ -1,3 +1,9 @@
> +2017-11-28 Sergio Lopez <slp@redhat.com>
> +
> + * linux-tdep.c (honor_dontdump_flag): New variable.
> + (dump_mapping_p): Use honor_dontdump_flag variable.
> + (_initialize_linux_tdep): New command 'set honor-dontdump-flag'.
Here they're fine. We don't usually include diffs to ChangeLogs in the
commit because they can cause conflicts when applying the patch, but
it's up to you.
> +
> 2017-11-27 Tom Tromey <tom@tromey.com>
>
> * Makefile.in (REMOTE_OBS): Remove.
> diff --git a/gdb/linux-tdep.c b/gdb/linux-tdep.c
> index 24237b8d39..5f4a1cdad1 100644
> --- a/gdb/linux-tdep.c
> +++ b/gdb/linux-tdep.c
> @@ -93,6 +93,11 @@ struct smaps_vmflags
>
> static int use_coredump_filter = 1;
>
> +/* Whether to honor the VM_DONTDUMP flag in /proc/PID/smaps when
> + generating a corefile. */
> +
> +static int honor_dontdump_flag = 1;
No empty line between command and definition of variable.
> +
> /* This enum represents the signals' numbers on a generic architecture
> running the Linux kernel. The definition of "generic" comes from
> the file <include/uapi/asm-generic/signal.h>, from the Linux kernel
> @@ -655,7 +660,7 @@ dump_mapping_p (filter_flags filterflags, const struct smaps_vmflags *v,
> return 0;
>
> /* Check if we should exclude this mapping. */
> - if (v->exclude_coredump)
> + if (honor_dontdump_flag && v->exclude_coredump)
> return 0;
>
> /* Update our notion of whether this mapping is shared or
> @@ -2517,4 +2522,16 @@ of /proc/PID/coredump_filter when generating the corefile. For more information
> about this file, refer to the manpage of core(5)."),
> NULL, show_use_coredump_filter,
> &setlist, &showlist);
> +
> + add_setshow_boolean_cmd ("honor-dontdump-flag", class_files,
> + &honor_dontdump_flag, _("\
> +Set whether gcore should honor the VM_DONTDUMP flag."),
> + _("\
> +Show whether gcore should honor the VM_DONTDUMP flag."),
> + _("\
> +Use this command to set whether gcore should honor the VM_DONTDUMP\n\
> +flag from /proc/PID/smaps when generating the corefile. For more information\n\
> +about this file, refer to the manpage of proc(5) and core(5)."),
> + NULL, show_use_coredump_filter,
You've already spotted the mistake of using 'show_use_coredump_filter'
here.
> + &setlist, &showlist);
I'm not sure "honor-dontdump-flag" is a good name for this setting.
There's no indication that it relates to coredumps, and I think it
should. A name like "honor-coredump-dontdump-flag" is a bit repetitive,
but IMHO is better than just "honor-dontdump-flag". WDYT?
To be fair, my first thought was to suggest adding a new "set coredump"
super command, which would encompass "set coredump use-coredump-filter"
and "set coredump honor-dontdump-flag", but I wouldn't like to introduce
this change so close to a release.
Otherwise, the patch looks OK to me, but I'm not a global maintainer.
Thanks,
--
Sergio
GPG key ID: 237A 54B1 0287 28BF 00EF 31F4 D0EB 7628 65FC 5E36
Please send encrypted e-mail if possible
http://sergiodj.net/