Re: [PATCH v2] Improve corefile generation by using /proc/PID/coredump_filter (PR corefile/16902)

[Sergio Durigan Junior <sergiodj@redhat.com>]

On Friday, March 13 2015, Pedro Alves wrote:

Thanks for the review, and sorry for taking a bit of time to respond.  I
had to dig deeper into this patch to properly answer some of your
questions.

> On 03/12/2015 09:39 PM, Sergio Durigan Junior wrote:
>> gdb/ChangeLog:
>> 2015-03-12  Sergio Durigan Junior  <sergiodj@redhat.com>
>> 	    Jan Kratochvil  <jan.kratochvil@redhat.com>
>> 	    Oleg Nesterov  <oleg@redhat.com>
>> 
>> 	PR corefiles/16092
>> 	* common/common-defs.h (enum memory_mapping_state): New enum.
>> 	* defs.h (find_memory_region_ftype): Remove 'int modified'
>> 	parameter, replacing by 'enum memory_mapping_state state'.
>> 	* gcore.c (gcore_create_callback): Likewise.  Change 'if/else'
>> 	statements and improve the logic of deciding when to ignore a
>> 	memory mapping.
>> 	(objfile_find_memory_regions): Passing
>> 	'MEMORY_MAPPING_UNKNOWN_STATE' or 'MEMORY_MAPPING_MODIFIED' when
>> 	needed to 'func' callback, instead of saying the memory mapping
>> 	was modified even without knowing it.
>> 	* gnu-nat.c (gnu_find_memory_regions): Likewise.
>> 	* linux-tdep.c: Include 'gdbcmd.h' and 'gdb_regex.h'.
>> 	New enum identifying the various options of the coredump_filter
>> 	file.
>> 	(struct smaps_vmflags): New struct.
>> 	(use_coredump_filter): New variable.
>> 	(decode_vmflags): New function.
>> 	(mapping_is_anonymous_p): Likewise.
>> 	(dump_mapping_p): Likewise.
>> 	(linux_find_memory_region_ftype): Remove 'int modified' parameter,
>> 	replacing by 'enum memory_mapping_state state'.
>> 	(linux_find_memory_regions_full): New variables
>> 	'coredumpfilter_name', 'coredumpfilterdata', 'pid',
>> 	'filterflags'.  Read /proc/<PID>/smaps file; improve parsing of
>> 	its information.  Implement memory mapping filtering based on its
>> 	contents.
>> 	(linux_find_memory_regions_thunk): Remove 'int modified'
>> 	parameter, replacing by 'enum memory_mapping_state state'.
>> 	(linux_make_mappings_callback): Likewise.
>> 	(find_mapping_size): Likewise.
>> 	(show_use_coredump_filter): New function.
>> 	(_initialize_linux_tdep): New command 'set use-coredump-filter'.
>> 	* procfs.c (find_memory_regions_callback): Passing
>> 	'MEMORY_MAPPING_UNKNOWN_STATE' when needed to 'func' callback,
>> 	instead of saying the memory mapping was modified even without
>> 	knowing it.
>> 
>> gdb/doc/ChangeLog:
>> 2015-03-12  Sergio Durigan Junior  <sergiodj@redhat.com>
>> 
>> 	PR corefiles/16092
>> 	* gdb.texinfo (gcore): Mention new command 'set
>> 	use-coredump-filter'.
>> 	(set use-coredump-filter): Document new command.
>> 
>> gdb/testsuite/ChangeLog:
>> 2015-03-12  Sergio Durigan Junior  <sergiodj@redhat.com>
>> 
>> 	PR corefiles/16092
>> 	* gdb.base/coredump-filter.c: New file.
>> 	* gdb.base/coredump-filter.exp: Likewise.
>> 
>> 
>> diff --git a/gdb/common/common-defs.h b/gdb/common/common-defs.h
>> index 62d9de5..01b05f5 100644
>> --- a/gdb/common/common-defs.h
>> +++ b/gdb/common/common-defs.h
>> @@ -60,4 +60,14 @@
>>  # define EXTERN_C_POP
>>  #endif
>>  
>> +/* Enum used to inform the state of a memory mapping.  This is used in
>> +   functions implementing find_memory_region_ftype.  */
>
> Why isn't this enum defined next to find_memory_region_ftype?

Bad judgement, sorry.  I thought it would make sense to put this into
common/ because it could be used by gdbserver.  But then again, it is
not (yet).  Moved to defs.h.

>> +
>> +enum memory_mapping_state
>> +  {
>> +    MEMORY_MAPPING_MODIFIED,
>> +    MEMORY_MAPPING_UNMODIFIED,
>> +    MEMORY_MAPPING_UNKNOWN_STATE,
>> +  };
>> +
>>  #endif /* COMMON_DEFS_H */
>> diff --git a/gdb/defs.h b/gdb/defs.h
>> index 72512f6..4829b62 100644
>> --- a/gdb/defs.h
>> +++ b/gdb/defs.h
>> @@ -338,7 +338,8 @@ extern void init_source_path (void);
>>  
>>  typedef int (*find_memory_region_ftype) (CORE_ADDR addr, unsigned long size,
>>  					 int read, int write, int exec,
>> -					 int modified, void *data);
>> +					 enum memory_mapping_state state,
>> +					 void *data);
>>  
>>  /* * Possible lvalue types.  Like enum language, this should be in
>>     value.h, but needs to be here for the same reason.  */
>> diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
>> index 9e71642..092bc93 100644
>> --- a/gdb/doc/gdb.texinfo
>> +++ b/gdb/doc/gdb.texinfo
>> @@ -10952,6 +10952,67 @@ specified, the file name defaults to @file{core.@var{pid}}, where
>>  
>>  Note that this command is implemented only for some systems (as of
>>  this writing, @sc{gnu}/Linux, FreeBSD, Solaris, and S390).
>> +
>> +On @sc{gnu}/Linux, this command can take into account the value of the
>> +file @file{/proc/@var{pid}/coredump_filter} when generating the core
>> +dump (@pxref{set use-coredump-filter}).
>> +
>> +@kindex set use-coredump-filter
>> +@anchor{set use-coredump-filter}
>> +@item set use-coredump-filter on
>> +@itemx set use-coredump-filter off
>> +Enable or disable the use of the file
>> +@file{/proc/@var{pid}/coredump_filter} when generating core dump
>> +files.  This file is used by the Linux kernel to decide what types of
>> +memory mappings will be dumped or ignored when generating a core dump
>> +file.
>> +
>> +To make use of this feature, you have to write in the
>> +@file{/proc/@var{pid}/coredump_filter} file a value, in hexadecimal,
>> +which is a bit mask representing the memory mapping types.  If a bit
>> +is set in the bit mask, then the memory mappings of the corresponding
>> +types will be dumped; otherwise, they will be ignored.  The bits in
>> +this bit mask have the following meanings:
>> +
>> +@table @code
>> +@item bit 0
>> +Dump anonymous private mappings.
>> +@item bit 1
>> +Dump anonymous shared mappings.
>> +@item bit 2
>> +Dump file-backed private mappings.
>> +@item bit 3
>> +Dump file-backed shared mappings.
>> +@item bit 4
>> +(since Linux 2.6.24)
>> +Dump ELF headers. (@value{GDBN} does not take this bit into account)
>> +@item bit 5
>> +(since Linux 2.6.28)
>> +Dump private huge pages.
>> +@item bit 6
>> +(since Linux 2.6.28)
>> +Dump shared huge pages.
>> +@end table
>> +
>> +For example, supposing that the @code{pid} of the program being
>> +debugging is @code{1234}, if you wanted to dump everything except the
>> +anonymous private and the file-backed shared mappings, you would do:
>> +
>> +@smallexample
>> +$ echo 0x76 > /proc/1234/coredump_filter
>> +@end smallexample
>> +
>> +For more documentation about how to use the @file{coredump_filter}
>> +file, see the manpage of @code{proc(5)}.
>> +
>> +By default, this option is @code{on}.  If this option is turned
>> +@code{off}, @value{GDBN} will not read the @file{coredump_filter}
>> +file, but it uses the same default value as the Linux kernel in order
>
> "will not read (...) uses".  I think the grammar isn't correct
> that way.
>
> I think preferred is to use present in both cases, thus:
>
>   "(...) turned @code{off}, @value{GDBN} does not the read the"
>
> and no "it", in "but uses the same".  Suggest "and" instead of "but":
>
> By default, this option is @code{on}.  If this option is turned
> @code{off}, @value{GDBN} does not read the @file{coredump_filter}
> file and instead uses the same default value as the Linux kernel in order
> ...

Fixed.

>> +to decide which pages will be dumped in the core dump file.  This
>> +value currently is @code{0x33}, which means that the bits @code{0}
>
> "is currently", I think.   Also, "which means that bits" would sound
> more natural to me, though take it with a grain of salt.

Fixed.

>> +(anonymous private mappings), @code{1} (anonymous shared mappings) and
>> +@code{4} (ELF headers) are active.  This will cause these memory
>> +mappings to be dumped automatically.
>>  @end table
>>  
>>  @node Character Sets
>> diff --git a/gdb/gcore.c b/gdb/gcore.c
>> index 44b9d0c..89d8285 100644
>> --- a/gdb/gcore.c
>> +++ b/gdb/gcore.c
>> @@ -415,27 +415,22 @@ make_output_phdrs (bfd *obfd, asection *osec, void *ignored)
>>  
>>  static int
>>  gcore_create_callback (CORE_ADDR vaddr, unsigned long size, int read,
>> -		       int write, int exec, int modified, void *data)
>> +		       int write, int exec, enum memory_mapping_state state,
>> +		       void *data)
>>  {
>>    bfd *obfd = data;
>>    asection *osec;
>>    flagword flags = SEC_ALLOC | SEC_HAS_CONTENTS | SEC_LOAD;
>>  
>> -  /* If the memory segment has no permissions set, ignore it, otherwise
>> -     when we later try to access it for read/write, we'll get an error
>> -     or jam the kernel.  */
>> -  if (read == 0 && write == 0 && exec == 0 && modified == 0)
>> -    {
>> -      if (info_verbose)
>> -        {
>> -          fprintf_filtered (gdb_stdout, "Ignore segment, %s bytes at %s\n",
>> -                            plongest (size), paddress (target_gdbarch (), vaddr));
>> -        }
>> -
>> -      return 0;
>> -    }
>> -
>> -  if (write == 0 && modified == 0 && !solib_keep_data_in_core (vaddr, size))
>> +  /* If the memory segment has no read permission set, or if it has
>> +     been marked as unmodified, then we have to generate a segment
>> +     header for it, but without contents (i.e., FileSiz = 0),
>> +     otherwise when we later try to access it for read/write, we'll
>> +     get an error or jam the kernel.  */
>> +  if (read == 0 || state == MEMORY_MAPPING_UNMODIFIED)
>> +    flags &= ~(SEC_LOAD | SEC_HAS_CONTENTS);
>
> I'm feeling dense and I'm understanding this change / comment.  :-/
> Why didn't we need to do this before, and we need to do it now?

I'm glad you asked, because that made me review the decision to hack
this part of the code, which led me to the conclusion that it needed to
be fixed/improved.

First of all, I would like to apologize for not having splited this
patch into smaller hunks; I am now fairly convinced that this would have
helped in the review process.

This specific part of the code started to be changed when I wanted to
simplify the original condition to ignore a memory mapping:

  if (read == 0 && write == 0 && exec == 0 && modified == 0)

The simplification was initially:

  if (read == 0)

The rationale is that if the mapping ccannot be read, then this is
enough for ignoring it.  However, after some discussions with Jan, he
suggested that instead of returning immediately from the function, GDB
should actually mark the memory region as '~(SEC_LOAD |
SEC_HAS_CONTENTS)', which basically means that it would create a segment
header for the mapping in the corefile, but mark its FileSiz as 0 (IOW,
when this corefile is opened, GDB would not try to read the contents of
this memory region).

I found this to be a good idea, and implemented this part.
Additionally, when I introduced 'enum memory_mapping_state' and tweaked
the code to provide exactly whether the mapping was MODIFIED, UNMODIFIED
or UNKNOWN, I also decided to generate a segment header for the mappings
that were marked as UNMODIFIED (in the patch, this state actually means
that the mapping should not be dumped at all, either because it was
marked as VM_DONTDUMP or because the user chose to ignore it via the
coredump_filter mechanism).  This was a bad call, because the purpose of
this patch is to bring GDB closer to the Linux kernel when it comes to
generating coredump files, and the Linux kernel does *not* generate
anything (not even a segment header) for the mappings that should be
ignored.

To summarize: I decided to change this part of the code, and make GDB
actually ignore (i.e., return 0) mappings marked as UNMODIFIED.  After
all, as explained above, this is what Linux does.

>> +  else if (write == 0 && state == MEMORY_MAPPING_UNKNOWN_STATE
>> +	   && !solib_keep_data_in_core (vaddr, size))
>>      {
>>        /* See if this region of memory lies inside a known file on disk.
>>  	 If so, we can avoid copying its contents by clearing SEC_LOAD.  */
>> @@ -528,7 +523,8 @@ objfile_find_memory_regions (struct target_ops *self,
>>  			 1, /* All sections will be readable.  */
>>  			 (flags & SEC_READONLY) == 0, /* Writable.  */
>>  			 (flags & SEC_CODE) != 0, /* Executable.  */
>> -			 1, /* MODIFIED is unknown, pass it as true.  */
>> +			 MEMORY_MAPPING_UNKNOWN_STATE, /* MODIFIED is
>> +							 unknown.  */
>>  			 obfd);
>>  	  if (ret != 0)
>>  	    return ret;
>> @@ -541,7 +537,7 @@ objfile_find_memory_regions (struct target_ops *self,
>>  	     1, /* Stack section will be readable.  */
>>  	     1, /* Stack section will be writable.  */
>>  	     0, /* Stack section will not be executable.  */
>> -	     1, /* Stack section will be modified.  */
>> +	     MEMORY_MAPPING_MODIFIED, /* Stack section will be modified.  */
>>  	     obfd);
>>  
>>    /* Make a heap segment.  */
>> @@ -550,7 +546,7 @@ objfile_find_memory_regions (struct target_ops *self,
>>  	     1, /* Heap section will be readable.  */
>>  	     1, /* Heap section will be writable.  */
>>  	     0, /* Heap section will not be executable.  */
>> -	     1, /* Heap section will be modified.  */
>> +	     MEMORY_MAPPING_MODIFIED, /* Heap section will be modified.  */
>>  	     obfd);
>>  
>>    return 0;
>> diff --git a/gdb/gnu-nat.c b/gdb/gnu-nat.c
>> index d830773..60612a7 100644
>> --- a/gdb/gnu-nat.c
>> +++ b/gdb/gnu-nat.c
>> @@ -2611,7 +2611,7 @@ gnu_find_memory_regions (struct target_ops *self,
>>  		     last_protection & VM_PROT_READ,
>>  		     last_protection & VM_PROT_WRITE,
>>  		     last_protection & VM_PROT_EXECUTE,
>> -		     1, /* MODIFIED is unknown, pass it as true.  */
>> +		     MEMORY_MAPPING_UNKNOWN_STATE, /* MODIFIED is unknown.  */
>>  		     data);
>>  	  last_region_address = region_address;
>>  	  last_region_end = region_address += region_length;
>> @@ -2625,7 +2625,7 @@ gnu_find_memory_regions (struct target_ops *self,
>>  	     last_protection & VM_PROT_READ,
>>  	     last_protection & VM_PROT_WRITE,
>>  	     last_protection & VM_PROT_EXECUTE,
>> -	     1, /* MODIFIED is unknown, pass it as true.  */
>> +	     MEMORY_MAPPING_UNKNOWN_STATE, /* MODIFIED is unknown.  */
>>  	     data);
>>  
>>    return 0;
>> diff --git a/gdb/linux-tdep.c b/gdb/linux-tdep.c
>> index ea0d4cd..ae11a2e 100644
>> --- a/gdb/linux-tdep.c
>> +++ b/gdb/linux-tdep.c
>> @@ -35,9 +35,58 @@
>>  #include "observer.h"
>>  #include "objfiles.h"
>>  #include "infcall.h"
>> +#include "gdbcmd.h"
>> +#include "gdb_regex.h"
>>  
>>  #include <ctype.h>
>>  
>> +/* This enum represents the values that the user can choose when
>> +   informing the Linux kernel about which memory mappings will be
>> +   dumped in a corefile.  They are described in the file
>> +   Documentation/filesystems/proc.txt, inside the Linux kernel
>> +   tree.  */
>> +
>> +enum
>> +  {
>> +    COREFILTER_ANON_PRIVATE = 1 << 0,
>> +    COREFILTER_ANON_SHARED = 1 << 1,
>> +    COREFILTER_MAPPED_PRIVATE = 1 << 2,
>> +    COREFILTER_MAPPED_SHARED = 1 << 3,
>> +    COREFILTER_ELF_HEADERS = 1 << 4,
>> +    COREFILTER_HUGETLB_PRIVATE = 1 << 5,
>> +    COREFILTER_HUGETLB_SHARED = 1 << 6,
>> +  };
>> +
>> +struct smaps_vmflags
>
> Missing intro comment.

Fixed.

>> +  {
>> +    /* Zero if this structure has not been initialized yet.  It
>> +       probably means that the Linux kernel being used does not emit
>> +       the "VmFlags:" field on "/proc/PID/smaps".  */
>> +
>> +    unsigned int initialized_p : 1;
>> +
>> +    /* Memory mapped I/O area (VM_IO, "io").  */
>> +
>> +    unsigned int io_page : 1;
>> +
>> +    /* Area uses huge TLB pages (VM_HUGETLB, "ht").  */
>> +
>> +    unsigned int uses_huge_tlb : 1;
>> +
>> +    /* Do not include this memory region on the coredump (VM_DONTDUMP, "dd").  */
>> +
>> +    unsigned int exclude_coredump : 1;
>> +
>> +    /* Is this a MAP_SHARED mapping (VM_SHARED, "sh").  */
>> +
>> +    unsigned int shared_mapping : 1;
>> +  };
>> +
>> +/* Whether to take the /proc/PID/coredump_filter into account when
>> +   generating a corefile.  */
>> +
>> +static int use_coredump_filter = 1;
>> +
>>  /* 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
>> @@ -381,6 +430,159 @@ read_mapping (const char *line,
>>    *filename = p;
>>  }
>>  
>> +/* Helper function to decode the "VmFlags" field in /proc/PID/smaps.
>> +
>> +   This function was based on the documentation found on
>> +   <Documentation/filesystems/proc.txt>, on the Linux kernel.
>> +
>> +   Linux kernels before commit
>> +   834f82e2aa9a8ede94b17b656329f850c1471514 do not have this field on
>> +   smaps.  */
>> +
>> +static void
>> +decode_vmflags (char *p, struct smaps_vmflags *v)
>
> Can 'p' be made const ?

strtok expects char *, so not really.

>> +{
>> +  char *saveptr;
>> +  char *s;
>
> Likewise.

Likewise for saveptr.  But I've made 's' const.

>> +
>> +  v->initialized_p = 1;
>> +  p = skip_to_space (p);
>> +  p = skip_spaces (p);
>> +
>> +  for (s = strtok_r (p, " ", &saveptr);
>> +       s != NULL;
>> +       s = strtok_r (NULL, " ", &saveptr))
>> +    {
>> +      if (strcmp (s, "io") == 0)
>> +	v->io_page = 1;
>> +      else if (strcmp (s, "ht") == 0)
>> +	v->uses_huge_tlb = 1;
>> +      else if (strcmp (s, "dd") == 0)
>> +	v->exclude_coredump = 1;
>> +      else if (strcmp (s, "sh") == 0)
>> +	v->shared_mapping = 1;
>> +    }
>> +}
>> +
>> +/* Return 1 if the memory mapping is anonymous, 0 otherwise.
>> +
>> +   FILENAME is the name of the file present in the first line of the
>> +   memory mapping, in the "/proc/PID/smaps" output.  For example, if
>> +   the first line is:
>> +
>> +   7fd0ca877000-7fd0d0da0000 r--p 00000000 fd:02 2100770   /path/to/file
>> +
>> +   Then FILENAME will be "/path/to/file".  */
>> +
>> +static int
>> +mapping_is_anonymous_p (const char *filename)
>
> As Oleg mentioned, these aren't really anonymous.  Is there a different
> term we can use, or improve the comment?

Well, for GDB those *are* anonymous mappings (in the sense of
MAP_ANONYMOUS).  In fact, I am not sure why Oleg said these mappings are
not anonymous; they are not file-backed either.  Maybe he means that we
shouldn't say only "anonymous" without specifying whether the mapping is
shared or private...  Anyway, I will have to discuss with him before I
can (a) understand why we can't say "anonymous" here, and (b) suggest a
better term, if there is one.

>> +{
>> +  static regex_t dev_zero_regex, shmem_file_regex, file_deleted_regex;
>> +  static int init_regex_p = 0;
>> +
>> +  if (!init_regex_p)
>> +    {
>> +      struct cleanup *c = make_cleanup (null_cleanup, NULL);
>> +
>> +      init_regex_p = 1;
>> +      compile_rx_or_error (&dev_zero_regex, "^/dev/zero\\( (deleted)\\)\\?$",
>> +			   _("Could not compile regex to match /dev/zero "
>> +			     "filename"));
>> +      compile_rx_or_error (&shmem_file_regex,
>> +			   "^/\\?SYSV[0-9a-fA-F]\\{8\\}\\( (deleted)\\)\\?$",
>> +			   _("Could not compile regex to match shmem "
>> +			     "filenames"));
>
> Could you add some comment about what these regexes above are for?

Done.

>> +      /* FILE_DELETED_REGEX is a heuristic we use to try to mimic the
>> +	 Linux kernel's 'n_link == 0' code, which is responsible to
>> +	 decide if it is dealing with a 'MAP_SHARED | MAP_ANONYMOUS'
>> +	 mapping.  In other words, if FILE_DELETED_REGEX matches, it
>> +	 does not necessarily mean that we are dealing with an
>> +	 anonymous shared mapping.  However, there is no easy way to
>> +	 detect this currently, so this is the best approximation we
>> +	 have.
>> +
>> +	 As a result, GDB will dump readonly pages of deleted
>> +	 executables when using the default value of coredump_filter
>> +	 (0x33), while the Linux kernel will not dump those pages.
>> +	 But we can live with that.  */
>> +      compile_rx_or_error (&file_deleted_regex, " (deleted)$",
>> +			   _("Could not compile regex to match "
>> +			     "'<file> (deleted)'"));
>> +      /* We will never release these regexes, so just discard the
>> +	 cleanups.  */
>> +      discard_cleanups (c);
>> +    }
>> +
>
> Above, on error, init_regex_p is left set to the same value as
> if no error was thrown.  Seems like then the second time
> this function is called we'll reach here with invalid compiled
> regexes:
>
>> +  if (*filename == '\0'
>> +      || regexec (&dev_zero_regex, filename, 0, NULL, 0) == 0
>> +      || regexec (&shmem_file_regex, filename, 0, NULL, 0) == 0
>> +      || regexec (&file_deleted_regex, filename, 0, NULL, 0) == 0)
>> +    return 1;
>
> I think it'd be safer if the top did:
>
> if (init_regex_p == -1)
>   return 1; // assume anonymous ?
>
> if (init_regex_p == 0)
>   {
>     init_regex_p = -1; /* assume error */
>
>     compile_rx_or_error ();
>
>     init_regex_p = 1; /* success! */
> }

I improved this code.  Instead of just assuming that the mapping is
anonymous, I decided to try to look (using strstr) for the string
"(deleted)" in the filename.  I think this is the least we can do to try
to guess what kind of mapping we're dealing with.

>> +
>> +  return 0;
>> +}
>> +
>> +/* Return 0 if the memory mapping (which is related to FILTERFLAGS, V,
>> +   MAYBE_PRIVATE_P, and MAPPING_ANONYMOUS_P) should not be dumped, or
>> +   greater than 0 if it should.  */
>> +
>> +static int
>> +dump_mapping_p (unsigned int filterflags, const struct smaps_vmflags *v,
>> +		int maybe_private_p, int mapping_anon_p, const char *filename)
>> +{
>> +  /* Initially, we trust in what we received from outside.  This value
>> +     may not be very precise (i.e., it was probably gathered from the
>> +     permission line in the /proc/PID/smaps list, which actually
>> +     refers to VM_MAYSHARE, and not VM_SHARED), but it is what we have
>> +     for now.  */
>> +  int private_p = maybe_private_p;
>> +
>> +  /* We always dump vDSO and vsyscall mappings.  */
>
> Add comment on why this is special cased?
>
> In the v1 patch intro you said:
>
>  "now also respects the MADV_DONTDUMP flag and does not dump the memory
>  mapping marked as so, and won't try to dump "[vsyscall]" or "[vdso]"
>  mappings as before (just like the Linux kernel)."
>
> Was that incorrect then?

Yeah, this was a thinko, Jan corrected right after I sent the message.
What I meant was that GDB *will* dump those mappings.

>> +  if (strcmp ("[vdso]", filename) == 0
>> +      || strcmp ("[vsyscall]", filename) == 0)
>> +    return 1;
>> +
>> +  if (v->initialized_p)
>> +    {
>> +      /* We never dump I/O mappings.  */
>> +      if (v->io_page)
>> +	return 0;
>> +
>> +      /* Check if we should exclude this mapping.  */
>> +      if (v->exclude_coredump)
>> +	return 0;
>> +
>> +      /* Updating our notion of whether this mapping is shared or
>
> s/Updating/Update/

Fixed.

>> +	 private based on a trustworthy value.  */
>> +      private_p = !v->shared_mapping;
>> +
>> +      /* HugeTLB checking.  */
>> +      if (v->uses_huge_tlb)
>> +	{
>> +	  if ((private_p && (filterflags & COREFILTER_HUGETLB_PRIVATE))
>> +	      || (!private_p && (filterflags & COREFILTER_HUGETLB_SHARED)))
>> +	    return 1;
>> +
>> +	  return 0;
>> +	}
>> +    }
>> +
>> +  if (private_p)
>> +    {
>> +      if (mapping_anon_p)
>> +	return (filterflags & COREFILTER_ANON_PRIVATE) != 0;
>> +      else
>> +	return (filterflags & COREFILTER_MAPPED_PRIVATE) != 0;
>> +    }
>> +  else
>> +    {
>> +      if (mapping_anon_p)
>> +	return (filterflags & COREFILTER_ANON_SHARED) != 0;
>> +      else
>> +	return (filterflags & COREFILTER_MAPPED_SHARED) != 0;
>> +    }
>> +}
>> +
>>  /* Implement the "info proc" command.  */
>>  
>>  static void
>> @@ -807,7 +1009,8 @@ linux_core_info_proc (struct gdbarch *gdbarch, const char *args,
>>  typedef int linux_find_memory_region_ftype (ULONGEST vaddr, ULONGEST size,
>>  					    ULONGEST offset, ULONGEST inode,
>>  					    int read, int write,
>> -					    int exec, int modified,
>> +					    int exec,
>> +					    enum memory_mapping_state state,
>>  					    const char *filename,
>>  					    void *data);
>>  
>> @@ -819,48 +1022,84 @@ linux_find_memory_regions_full (struct gdbarch *gdbarch,
>>  				void *obfd)
>>  {
>>    char mapsfilename[100];
>> -  char *data;
>> +  char coredumpfilter_name[100];
>> +  char *data, *coredumpfilterdata;
>> +  pid_t pid;
>> +  /* Default dump behavior of coredump_filter (0x33), according to
>> +     Documentation/filesystems/proc.txt from the Linux kernel
>> +     tree.  */
>> +  unsigned int filterflags = (COREFILTER_ANON_PRIVATE
>> +			      | COREFILTER_ANON_SHARED
>> +			      | COREFILTER_ELF_HEADERS
>> +			      | COREFILTER_HUGETLB_PRIVATE);
>>  
>>    /* We need to know the real target PID to access /proc.  */
>>    if (current_inferior ()->fake_pid_p)
>>      return 1;
>>  
>> -  xsnprintf (mapsfilename, sizeof mapsfilename,
>> -	     "/proc/%d/smaps", current_inferior ()->pid);
>> +  pid = current_inferior ()->pid;
>> +
>> +  if (use_coredump_filter)
>> +    {
>> +      xsnprintf (coredumpfilter_name, sizeof (coredumpfilter_name),
>> +		 "/proc/%d/coredump_filter", pid);
>> +      coredumpfilterdata = target_fileio_read_stralloc (coredumpfilter_name);
>> +      if (coredumpfilterdata != NULL)
>> +	{
>> +	  sscanf (coredumpfilterdata, "%x", &filterflags);
>> +	  xfree (coredumpfilterdata);
>> +	}
>> +    }
>> +
>> +  xsnprintf (mapsfilename, sizeof mapsfilename, "/proc/%d/smaps", pid);
>>    data = target_fileio_read_stralloc (mapsfilename);
>>    if (data == NULL)
>>      {
>>        /* Older Linux kernels did not support /proc/PID/smaps.  */
>> -      xsnprintf (mapsfilename, sizeof mapsfilename,
>> -		 "/proc/%d/maps", current_inferior ()->pid);
>> +      xsnprintf (mapsfilename, sizeof mapsfilename, "/proc/%d/maps", pid);
>>        data = target_fileio_read_stralloc (mapsfilename);
>>      }
>> -  if (data)
>> +
>> +  if (data != NULL)
>>      {
>>        struct cleanup *cleanup = make_cleanup (xfree, data);
>> -      char *line;
>> +      char *line, *t;
>>  
>> -      line = strtok (data, "\n");
>> -      while (line)
>> +      line = strtok_r (data, "\n", &t);
>> +      while (line != NULL)
>>  	{
>>  	  ULONGEST addr, endaddr, offset, inode;
>>  	  const char *permissions, *device, *filename;
>> +	  struct smaps_vmflags v;
>>  	  size_t permissions_len, device_len;
>> -	  int read, write, exec;
>> -	  int modified = 0, has_anonymous = 0;
>> +	  int read, write, exec, private;
>> +	  enum memory_mapping_state state;
>> +	  int has_anonymous = 0;
>> +	  int mapping_anon_p;
>>  
>> +	  memset (&v, 0, sizeof (v));
>>  	  read_mapping (line, &addr, &endaddr, &permissions, &permissions_len,
>>  			&offset, &device, &device_len, &inode, &filename);
>> +	  mapping_anon_p = mapping_is_anonymous_p (filename);
>>  
>>  	  /* Decode permissions.  */
>>  	  read = (memchr (permissions, 'r', permissions_len) != 0);
>>  	  write = (memchr (permissions, 'w', permissions_len) != 0);
>>  	  exec = (memchr (permissions, 'x', permissions_len) != 0);
>> +	  /* 'private' here actually means VM_MAYSHARE, and not
>> +	     VM_SHARED.  In order to know if a mapping is really
>> +	     private or not, we must check the flag "sh" in the
>> +	     VmFlags field.  This is done by decode_vmflags.  However,
>> +	     if we are using an old Linux kernel, we will not have the
>
> It's best to avoid "old", "new", etc.  New will get old soon too.
> Do we have some version string/number to put here instead?
> Likewise other places.

Yeah.  Fixed.

>> +	     VmFlags there.  In this case, there is really no way to
>> +	     know if we are dealing with VM_SHARED, so we just assume
>> +	     that VM_MAYSHARE is enough.  */
>> +	  private = memchr (permissions, 'p', permissions_len) != 0;
>>  
>>  	  /* Try to detect if region was modified by parsing smaps counters.  */
>> -	  for (line = strtok (NULL, "\n");
>> -	       line && line[0] >= 'A' && line[0] <= 'Z';
>> -	       line = strtok (NULL, "\n"))
>> +	  for (line = strtok_r (NULL, "\n", &t);
>> +	       line != NULL && line[0] >= 'A' && line[0] <= 'Z';
>> +	       line = strtok_r (NULL, "\n", &t))
>>  	    {
>>  	      char keyword[64 + 1];
>>  
>> @@ -869,11 +1108,17 @@ linux_find_memory_regions_full (struct gdbarch *gdbarch,
>>  		  warning (_("Error parsing {s,}maps file '%s'"), mapsfilename);
>>  		  break;
>>  		}
>> +
>>  	      if (strcmp (keyword, "Anonymous:") == 0)
>> -		has_anonymous = 1;
>> -	      if (strcmp (keyword, "Shared_Dirty:") == 0
>> -		  || strcmp (keyword, "Private_Dirty:") == 0
>> -		  || strcmp (keyword, "Swap:") == 0
>> +		{
>> +		  /* Older Linux kernels did not support the
>> +		     "Anonymous:" counter.  Check it here.  */
>> +		  has_anonymous = 1;
>> +		}
>> +	      else if (strcmp (keyword, "VmFlags:") == 0)
>> +		decode_vmflags (line, &v);
>> +
>> +	      if (strcmp (keyword, "AnonHugePages:") == 0
>>  		  || strcmp (keyword, "Anonymous:") == 0)
>>  		{
>>  		  unsigned long number;
>> @@ -884,19 +1129,43 @@ linux_find_memory_regions_full (struct gdbarch *gdbarch,
>>  			       mapsfilename);
>>  		      break;
>>  		    }
>> -		  if (number != 0)
>> -		    modified = 1;
>> +		  if (number > 0)
>> +		    {
>> +		      /* Even if we are dealing with a file-backed
>> +			 mapping, if it contains anonymous pages we
>> +			 consider it to be an anonymous mapping,
>> +			 because this is what the Linux kernel does:
>> +
>> +			 // Dump segments that have been written to.
>> +			 if (vma->anon_vma && FILTER(ANON_PRIVATE))
>> +			 	goto whole;
>> +		      */
>> +		      mapping_anon_p = 1;
>> +		    }
>>  		}
>>  	    }
>>  
>> -	  /* Older Linux kernels did not support the "Anonymous:" counter.
>> -	     If it is missing, we can't be sure - dump all the pages.  */
>> -	  if (!has_anonymous)
>> -	    modified = 1;
>> +	  /* If a mapping should not be dumped we still should create
>> +	     a segment for it, just without SEC_LOAD (see
>> +	     gcore_create_callback).  */
>
> I saw gcore_create_callback, but I ended up still clueless.  :-)

I hope my explanation above helped you in this.  I will also try to
improve the comments on the code.

>> +	  if (has_anonymous)
>> +	    {
>> +	      if (dump_mapping_p (filterflags, &v, private, mapping_anon_p,
>> +				  filename))
>> +		state = MEMORY_MAPPING_MODIFIED;
>> +	      else
>> +		state = MEMORY_MAPPING_UNMODIFIED;
>> +	    }
>> +	  else
>> +	    {
>> +	      /* Older Linux kernels did not support the "Anonymous:" counter.
>> +		 If it is missing, we can't be sure - dump all the pages.  */
>> +	      state = MEMORY_MAPPING_UNKNOWN_STATE;
>> +	    }
>>  
>>  	  /* Invoke the callback function to create the corefile segment.  */
>>  	  func (addr, endaddr - addr, offset, inode,
>> -		read, write, exec, modified, filename, obfd);
>> +		read, write, exec, state, filename, obfd);
>>  	}
>>  
>>        do_cleanups (cleanup);
>> @@ -926,12 +1195,13 @@ struct linux_find_memory_regions_data
>>  static int
>>  linux_find_memory_regions_thunk (ULONGEST vaddr, ULONGEST size,
>>  				 ULONGEST offset, ULONGEST inode,
>> -				 int read, int write, int exec, int modified,
>> +				 int read, int write, int exec,
>> +				 enum memory_mapping_state state,
>
> read/write, etc. are also state.  "enum memory_mapping_state state"
> doesn't really indicate immediately what it's about.  How about
> using "modified_state" for the variable name?  (here and elsewhere).
> I think the end result will be more readable.

Right, fixed.

>>  				 const char *filename, void *arg)
>>  {
>>    struct linux_find_memory_regions_data *data = arg;
>>  
>> -  return data->func (vaddr, size, read, write, exec, modified, data->obfd);
>> +  return data->func (vaddr, size, read, write, exec, state, data->obfd);
>>  }
>>  
>>  /* A variant of linux_find_memory_regions_full that is suitable as the
>> @@ -1074,7 +1344,8 @@ static linux_find_memory_region_ftype linux_make_mappings_callback;
>>  static int
>>  linux_make_mappings_callback (ULONGEST vaddr, ULONGEST size,
>>  			      ULONGEST offset, ULONGEST inode,
>> -			      int read, int write, int exec, int modified,
>> +			      int read, int write, int exec,
>> +			      enum memory_mapping_state state,
>>  			      const char *filename, void *data)
>>  {
>>    struct linux_make_mappings_data *map_data = data;
>> @@ -1872,7 +2143,8 @@ linux_gdb_signal_to_target (struct gdbarch *gdbarch,
>>  
>>  static int
>>  find_mapping_size (CORE_ADDR vaddr, unsigned long size,
>> -		   int read, int write, int exec, int modified,
>> +		   int read, int write, int exec,
>> +		   enum memory_mapping_state state,
>>  		   void *data)
>>  {
>>    struct mem_range *range = data;
>> @@ -1972,6 +2244,17 @@ linux_infcall_mmap (CORE_ADDR size, unsigned prot)
>>    return retval;
>>  }
>>  
>> +/* Display whether the gcore command is using the
>> +   /proc/PID/coredump_filter file.  */
>> +
>> +static void
>> +show_use_coredump_filter (struct ui_file *file, int from_tty,
>> +			  struct cmd_list_element *c, const char *value)
>> +{
>> +  fprintf_filtered (file, _("Use of /proc/PID/coredump_filter file to generate"
>> +			    " corefiles is %s.\n"), value);
>> +}
>> +
>>  /* To be called from the various GDB_OSABI_LINUX handlers for the
>>     various GNU/Linux architectures and machine types.  */
>>  
>> @@ -2008,4 +2291,16 @@ _initialize_linux_tdep (void)
>>    /* Observers used to invalidate the cache when needed.  */
>>    observer_attach_inferior_exit (invalidate_linux_cache_inf);
>>    observer_attach_inferior_appeared (invalidate_linux_cache_inf);
>> +
>> +  add_setshow_boolean_cmd ("use-coredump-filter", class_files,
>> +			   &use_coredump_filter, _("\
>> +Set whether gcore should consider /proc/PID/coredump_filter."),
>> +			   _("\
>> +Show whether gcore should consider /proc/PID/coredump_filter."),
>> +			   _("\
>> +Use this command to set whether gcore should consider the contents\n\
>> +of /proc/PID/coredump_filter when generating the corefile.  For more information\n\
>> +about this file, refer to the manpage of core(5)."),
>> +			   NULL, show_use_coredump_filter,
>> +			   &setlist, &showlist);
>>  }
>> diff --git a/gdb/procfs.c b/gdb/procfs.c
>> index b62539f..d074dd3 100644
>> --- a/gdb/procfs.c
>> +++ b/gdb/procfs.c
>> @@ -4967,7 +4967,7 @@ find_memory_regions_callback (struct prmap *map,
>>  		  (map->pr_mflags & MA_READ) != 0,
>>  		  (map->pr_mflags & MA_WRITE) != 0,
>>  		  (map->pr_mflags & MA_EXEC) != 0,
>> -		  1, /* MODIFIED is unknown, pass it as true.  */
>> +		  MEMORY_MAPPING_UNKNOWN_STATE, /* MODIFIED is unknown.  */
>>  		  data);
>>  }
>>  
>> diff --git a/gdb/testsuite/gdb.base/coredump-filter.c b/gdb/testsuite/gdb.base/coredump-filter.c
>> new file mode 100644
>> index 0000000..192c469
>> --- /dev/null
>> +++ b/gdb/testsuite/gdb.base/coredump-filter.c
>> @@ -0,0 +1,61 @@
>> +/* Copyright 2015 Free Software Foundation, Inc.
>> +
>> +   This file is part of GDB.
>> +
>> +   This program is free software; you can redistribute it and/or modify
>> +   it under the terms of the GNU General Public License as published by
>> +   the Free Software Foundation; either version 3 of the License, or
>> +   (at your option) any later version.
>> +
>> +   This program is distributed in the hope that it will be useful,
>> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
>> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> +   GNU General Public License for more details.
>> +
>> +   You should have received a copy of the GNU General Public License
>> +   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
>> +
>> +#define _GNU_SOURCE
>> +#include <stdlib.h>
>> +#include <assert.h>
>> +#include <unistd.h>
>> +#include <stdio.h>
>> +#include <sys/mman.h>
>> +#include <errno.h>
>> +#include <string.h>
>> +
>> +static void *
>> +do_mmap (void *addr, size_t size, int prot, int flags, int fd, off_t offset)
>> +{
>> +  void *ret = mmap (addr, size, prot, flags, fd, offset);
>> +
>> +  assert (ret != NULL);
>> +  return ret;
>> +}
>> +
>> +int
>> +main (int argc, char *argv[])
>> +{
>> +  const size_t size = 10;
>> +  const int default_prot = PROT_READ | PROT_WRITE;
>> +  char *private_anon, *shared_anon;
>> +  char *dont_dump;
>> +  int i;
>> +
>> +  private_anon = do_mmap (NULL, size, default_prot,
>> +			  MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
>> +  memset (private_anon, 0x11, size);
>> +
>> +  shared_anon = do_mmap (NULL, size, default_prot,
>> +			 MAP_SHARED | MAP_ANONYMOUS, -1, 0);
>> +  memset (shared_anon, 0x22, size);
>> +
>> +  dont_dump = do_mmap (NULL, size, default_prot,
>> +		       MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
>> +  memset (dont_dump, 0x55, size);
>> +  i = madvise (dont_dump, size, MADV_DONTDUMP);
>> +  assert_perror (errno);
>> +  assert (i == 0);
>> +
>> +  return 0; /* break-here */
>> +}
>> diff --git a/gdb/testsuite/gdb.base/coredump-filter.exp b/gdb/testsuite/gdb.base/coredump-filter.exp
>> new file mode 100644
>> index 0000000..c7ae91d
>> --- /dev/null
>> +++ b/gdb/testsuite/gdb.base/coredump-filter.exp
>> @@ -0,0 +1,129 @@
>> +# Copyright 2015 Free Software Foundation, Inc.
>> +
>> +# This program is free software; you can redistribute it and/or modify
>> +# it under the terms of the GNU General Public License as published by
>> +# the Free Software Foundation; either version 3 of the License, or
>> +# (at your option) any later version.
>> +#
>> +# This program is distributed in the hope that it will be useful,
>> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
>> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> +# GNU General Public License for more details.
>> +#
>> +# You should have received a copy of the GNU General Public License
>> +# along with this program.  If not, see <http://www.gnu.org/licenses/>.
>> +
>> +standard_testfile
>> +
>> +if { [prepare_for_testing "failed to prepare" $testfile $srcfile debug] } {
>> +    untested $testfile.exp
>> +    return -1
>> +}
>> +
>> +if { ![runto_main] } {
>> +    untested $testfile.exp
>> +    return -1
>> +}
>> +
>> +gdb_breakpoint [gdb_get_line_number "break-here"]
>> +gdb_continue_to_breakpoint "break-here" ".* break-here .*"
>> +
>> +proc do_save_core { filter_flag core ipid } {
>> +    verbose -log "writing $filter_flag to /proc/$ipid/coredump_filter"
>> +    if { [catch {open /proc/$ipid/coredump_filter w} fileid] } {
>
> This is opening the /proc file on the build machine, but it
> should be the file on the target machine.  Can you use
> "remote_file target" for this?
>
> If not, perhaps something around:
>
>  remote_exec target "echo $filter_flag > /proc/$ipid/coredump_filter"
>
> ?

Fixed.  I chose to use the second method.

>> +	untested $testfile.exp
>> +	return -1
>> +    }
>> +
>> +    # Set coredump_filter to the value we want
>> +    puts $fileid $filter_flag
>> +    close $fileid
>> +
>> +    # Generate a corefile
>> +    gdb_gcore_cmd "$core" "save corefile $core"
>> +}
>> +
>> +proc do_load_and_test_core { core var working_var working_value } {
>> +    global hex decimal addr
>> +
>> +    set core_loaded [gdb_core_cmd "$core" "load $core"]
>> +    if { $core_loaded == -1 } {
>> +	fail "loading $core"
>> +	return
>> +    }
>> +
>> +    # Use 'int' as any variants of 'char' try to read the target bytes.
>
> I don't understand this comment.

This is not really needed, but I can expand on the comment.

Jan suggested that I use the "*(unsigned int *)" form to access the
variable's contents because it is clearer this way that I am interested
in the contents, rather in the address itself.  It is just to make the
code clearer, but I will explain better.

>> +    gdb_test "print *(unsigned int *) $addr($var)" "\(\\\$$decimal = <error: \)?Cannot access memory at address $hex\(>\)?" \
>> +	"printing $var when core is loaded (should not work)"
>> +    gdb_test "print/x *(unsigned int *) $addr($working_var)" " = $working_value.*" \
>> +	"print/x *$working_var ( = $working_value)"
>> +}
>> +
>> +set non_private_anon_core [standard_output_file non-private-anon.gcore]
>> +set non_shared_anon_core [standard_output_file non-shared-anon.gcore]
>> +set dont_dump_core [standard_output_file dont-dump.gcore]
>> +
>> +# We will generate a few corefiles
>
> Missing period.

Fixed.

>> +#
>> +# This list is composed by sub-lists, and their elements are (in
>> +# order):
>> +#
>> +# - name of the test
>> +# - hexadecimal value to be put in the /proc/PID/coredump_filter file
>> +# - name of the variable that contains the name of the corefile to be
>> +#   generated (including the initial $).
>> +# - name of the variable in the C source code that points to the
>> +#   memory mapping that will NOT be present in the corefile.
>> +# - name of a variable in the C source code that points to a memory
>> +#   mapping that WILL be present in the corefile
>> +# - corresponding value expected for the above variable
>> +
>> +set all_corefiles { { "non-Private-Anonymous" "0x7e" \
>> +			  $non_private_anon_core \
>> +			  "private_anon" \
>> +			  "shared_anon" "0x22" }
>> +    { "non-Shared-Anonymous" "0x7d" \
>> +	  $non_shared_anon_core "shared_anon" \
>> +	  "private_anon" "0x11" }
>> +    { "DoNotDump" "0x33" \
>> +	  $dont_dump_core "dont_dump" \
>> +	  "shared_anon" "0x22" } }
>> +
>> +set core_supported [gdb_gcore_cmd "$non_private_anon_core" "save a corefile"]
>> +if { !$core_supported } {
>> +    untested $testfile.exp
>
> https://sourceware.org/gdb/wiki/GDBTestcaseCookbook#A.22untested.22_calls

Fixed.

>> +    return -1
>> +}
>> +
>> +# Getting the inferior's PID
>> +gdb_test_multiple "info inferiors" "getting inferior pid" {
>> +    -re "process \($decimal\).*\r\n$gdb_prompt $" {
>> +	set infpid $expect_out(1,string)
>> +    }
>> +}
>
> Don't leave infpid undefined on gdb_test_multiple failure.
> Set it upfront:
>
>   set infpid ""
>   gdb_test_multiple "info inferiors" "getting inferior pid" {
>     ...

Fixed.

>
>> +
>> +foreach item $all_corefiles {
>> +    foreach name [list [lindex $item 3] [lindex $item 4]] {
>> +	set test "print/x $name"
>> +	gdb_test_multiple $test $test {
>> +	    -re " = \($hex\)\r\n$gdb_prompt $" {
>> +		set addr($name) $expect_out(1,string)
>
> I'm probably being dense, but I can't see where is addr
> ever used?

'addr' is an associative array that maps variable names to addresses.
This is needed because sometimes (depending on which pags we
dump/ignore), the stack is not dumped and GDB would not be able to find
the variable names; therefore, this dependency is removed here.

>> +	    }
>> +	}
>> +    }
>> +}
>> +
>> +foreach item $all_corefiles {
>> +    with_test_prefix "saving corefile for [lindex $item 0]" {
>> +	do_save_core [lindex $item 1] [subst [lindex $item 2]] $infpid
>> +    }
>> +}
>> +
>> +clean_restart $testfile
>> +
>> +foreach item $all_corefiles {
>> +    with_test_prefix "loading and testing corefile for [lindex $item 0]" {
>> +	do_load_and_test_core [subst [lindex $item 2]] [lindex $item 3] \
>> +	    [lindex $item 4] [lindex $item 5]
>> +    }
>> +}
>
> Thanks,
> Pedro Alves


I will not send the full patch again because I intend to split it into
minor, more logical patches.  I should be able to send it later
today/tomorrow.

Thanks,

-- 
Sergio
GPG key ID: 0x65FC5E36
Please send encrypted e-mail if possible
http://sergiodj.net/