Re: [PATCH 1/4] Implement 'set honor-dontdump-flag' command

Sergio Durigan Junior
2017-11-28T15:42:26Z

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