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

  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.

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'.
---
 gdb/ChangeLog    |  6 ++++++
 gdb/linux-tdep.c | 19 ++++++++++++++++++-
 2 files changed, 24 insertions(+), 1 deletion(-)
  

On Tue, Nov 28, 2017 at 2:21 PM, Sergio Lopez <slp@redhat.com> wrote:
> +  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,
> +                          &setlist, &showlist);
>  }

A colleague spotted that I forgot to implement and use a different
"show" function for honor-dontdump-flag, wrongly reusing
show_use_coredump_filter.

I'm going to wait for feedback on the rest of the patchset. Will post
a v2 afterward.
  

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,
  

On 11/28/2017 03:42 PM, Sergio Durigan Junior wrote:

>> @@ -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?

Personally, I don't find the "coredump" issue that much of a
problem, given "dump" is there.  "honor-coredump-dontdump" looks
like a mouthful to me.

I think using "ignore" in the name would some more natural
in GDB than "honor".  I.e., "set ignore-dontdump-flag on/off".
That mean default is off/0 ( and the control variable could go
to .bss :-) )

It also avoids our UK friends cursing at bad spelling of "honour".  :-)

"set dump-excluded-mappings on/off" could work too?

I skimmed the series and didn't find a gdb/NEWS entry; we need
one for new commands.

> 
> 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,
Pedro Alves
  

On Tuesday, November 28 2017, Pedro Alves wrote:

> On 11/28/2017 03:42 PM, Sergio Durigan Junior wrote:
>
>>> @@ -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?
>
> Personally, I don't find the "coredump" issue that much of a
> problem, given "dump" is there.  "honor-coredump-dontdump" looks
> like a mouthful to me.

It is a mouthful, but at least it leaves no room for doubt.  The "dump"
in the name doesn't necessarily translate to "coredump", I think.  Or at
least that's what I would feel.

> I think using "ignore" in the name would some more natural
> in GDB than "honor".  I.e., "set ignore-dontdump-flag on/off".
> That mean default is off/0 ( and the control variable could go
> to .bss :-) )
>
> It also avoids our UK friends cursing at bad spelling of "honour".  :-)
>
> "set dump-excluded-mappings on/off" could work too?

This one is better IMO.
  

On Tue, Nov 28, 2017 at 4:42 PM, Sergio Durigan Junior
<sergiodj@redhat.com> wrote:
> On Tuesday, November 28 2017, Sergio Lopez wrote:
>> 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 is the only suggestion that I haven't applied because it would
break the coding style of the previous lines:

gdb/linux-tdep.c:
  89   };
  90
  91 /* Whether to take the /proc/PID/coredump_filter into account when
  92    generating a corefile.  */
  93
  94 static int use_coredump_filter = 1;
  95
  96 /* Whether the value of smaps_vmflags->exclude_coredump should be
  97    ignored, including mappings marked with the VM_DONTDUMP flag in
  98    the dump.  */
  99
 100 static int dump_excluded_mappings = 0;
 101
 102 /* This enum represents the signals' numbers on a generic architecture
  

On Wednesday, November 29 2017, Sergio Lopez wrote:

> On Tue, Nov 28, 2017 at 4:42 PM, Sergio Durigan Junior
> <sergiodj@redhat.com> wrote:
>> On Tuesday, November 28 2017, Sergio Lopez wrote:
>>> 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 is the only suggestion that I haven't applied because it would
> break the coding style of the previous lines:
>
> gdb/linux-tdep.c:
>   89   };
>   90
>   91 /* Whether to take the /proc/PID/coredump_filter into account when
>   92    generating a corefile.  */
>   93
>   94 static int use_coredump_filter = 1;
>   95
>   96 /* Whether the value of smaps_vmflags->exclude_coredump should be
>   97    ignored, including mappings marked with the VM_DONTDUMP flag in
>   98    the dump.  */
>   99
>  100 static int dump_excluded_mappings = 0;
>  101
>  102 /* This enum represents the signals' numbers on a generic architecture

The previous line is the one breaking our coding style.  Unfortunately
GDB is full of these inconsistencies, but please remove the empty line
in your patch anyway.

Thanks,
  

On Wed, Nov 29, 2017 at 8:39 PM, Sergio Durigan Junior
<sergiodj@redhat.com> wrote:
> The previous line is the one breaking our coding style.  Unfortunately
> GDB is full of these inconsistencies, but please remove the empty line
> in your patch anyway.

Ack, not a problem.

Is there something else you'd like to see fixed in v3? In case you
don't, do you prefer me to send the whole patchset as v3, or just
patch 1/4?
  

On Wednesday, November 29 2017, Sergio Lopez wrote:

> On Wed, Nov 29, 2017 at 8:39 PM, Sergio Durigan Junior
> <sergiodj@redhat.com> wrote:
>> The previous line is the one breaking our coding style.  Unfortunately
>> GDB is full of these inconsistencies, but please remove the empty line
>> in your patch anyway.
>
> Ack, not a problem.
>
> Is there something else you'd like to see fixed in v3? In case you
> don't, do you prefer me to send the whole patchset as v3, or just
> patch 1/4?

The only thing I'd like is a testcase for this feature.  This is
required since it is a regression.  If you need any help to write one,
please let me know.

Once you have a testcase, you could just send patch 1/4 again.

Thanks,