[PATCH 1/4] Improve identification of memory mappings

[PATCH 1/4] Improve identification of memory mappings

[Sergio Durigan Junior]

This commit implements the new 'enum memory_mapping_state', which can
be used to represent the different states of each memory mapping from
the inferior.  These states are:

  - MODIFIED, which means that the mapping should be dumped in
    corefiles

  - UNMODIFIED, which means that the mapping should not be dumped in
    corefiles (e.g., mappings that have been marked as VM_DONTDUMP), and

  - UNKNOWN, which means that we don't know whether the mapping should
    or should not be dumped.

The last state is not used in this patch, but will be used in the
following patches of this series.

It is also worth mentioning that I do not intend to commit this patch
independently.  IOW, I will merge everything before pushing one single
commit.

Changes from v2:

  - Moved declaration of 'enum memory_mapping_state' from
    gdb/common/common-defs.h to gdb/defs.h.

  - Renamed variable to 'modified_state' (instead of 'state').

gdb/ChangeLog:
2015-03-18  Sergio Durigan Junior  <sergiodj@redhat.com>
	    Jan Kratochvil  <jan.kratochvil@redhat.com>
	    Oleg Nesterov  <oleg@redhat.com>

	PR corefiles/16092
	* defs.h (enum memory_mapping_state): New enum.
	(find_memory_region_ftype): Remove 'int modified' parameter,
	replacing by 'enum memory_mapping_state modified_state'.
	* gcore.c (gcore_create_callback): Likewise.
	(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 (linux_find_memory_region_ftype): Remove 'int
	modified' parameter, replacing by 'enum memory_mapping_state
	modified_state'.
	(linux_find_memory_regions_thunk): Likewise.
	(linux_make_mappings_callback): Likewise.
	(find_mapping_size): Likewise.
	* procfs.c (find_memory_regions_callback): Likewise.
---
 gdb/defs.h       | 13 ++++++++++++-
 gdb/gcore.c      | 16 ++++++++++------
 gdb/gnu-nat.c    |  4 ++--
 gdb/linux-tdep.c | 25 ++++++++++++++++---------
 gdb/procfs.c     |  2 +-
 5 files changed, 41 insertions(+), 19 deletions(-)

diff --git a/gdb/defs.h b/gdb/defs.h
index 72512f6..37b1430 100644
--- a/gdb/defs.h
+++ b/gdb/defs.h
@@ -330,6 +330,16 @@ extern void init_source_path (void);
 
 /* From exec.c */
 
+/* Enum used to inform the state of a memory mapping.  This is used in
+   functions implementing find_memory_region_ftype.  */
+
+enum memory_mapping_state
+  {
+    MEMORY_MAPPING_MODIFIED,
+    MEMORY_MAPPING_UNMODIFIED,
+    MEMORY_MAPPING_UNKNOWN_STATE,
+  };
+
 /* * Process memory area starting at ADDR with length SIZE.  Area is
    readable iff READ is non-zero, writable if WRITE is non-zero,
    executable if EXEC is non-zero.  Area is possibly changed against
@@ -338,7 +348,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/gcore.c b/gdb/gcore.c
index 44b9d0c..751ddac 100644
--- a/gdb/gcore.c
+++ b/gdb/gcore.c
@@ -415,7 +415,8 @@ 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 modified_state, void *data)
 {
   bfd *obfd = data;
   asection *osec;
@@ -424,7 +425,8 @@ gcore_create_callback (CORE_ADDR vaddr, unsigned long size, int read,
   /* 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 (read == 0 && write == 0 && exec == 0
+      && modified_state == MEMORY_MAPPING_UNMODIFIED)
     {
       if (info_verbose)
         {
@@ -435,7 +437,8 @@ gcore_create_callback (CORE_ADDR vaddr, unsigned long size, int read,
       return 0;
     }
 
-  if (write == 0 && modified == 0 && !solib_keep_data_in_core (vaddr, size))
+  if (write == 0 && modified_state == MEMORY_MAPPING_UNMODIFIED
+      && !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 +531,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 +545,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 +554,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..15725c7 100644
--- a/gdb/linux-tdep.c
+++ b/gdb/linux-tdep.c
@@ -807,7 +807,9 @@ 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
+					    modified_state,
 					    const char *filename,
 					    void *data);
 
@@ -847,7 +849,8 @@ linux_find_memory_regions_full (struct gdbarch *gdbarch,
 	  const char *permissions, *device, *filename;
 	  size_t permissions_len, device_len;
 	  int read, write, exec;
-	  int modified = 0, has_anonymous = 0;
+	  enum memory_mapping_state modified_state = MEMORY_MAPPING_UNMODIFIED;
+	  int has_anonymous = 0;
 
 	  read_mapping (line, &addr, &endaddr, &permissions, &permissions_len,
 			&offset, &device, &device_len, &inode, &filename);
@@ -885,18 +888,18 @@ linux_find_memory_regions_full (struct gdbarch *gdbarch,
 		      break;
 		    }
 		  if (number != 0)
-		    modified = 1;
+		    modified_state = MEMORY_MAPPING_MODIFIED;
 		}
 	    }
 
 	  /* 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;
+	    modified_state = MEMORY_MAPPING_MODIFIED;
 
 	  /* Invoke the callback function to create the corefile segment.  */
 	  func (addr, endaddr - addr, offset, inode,
-		read, write, exec, modified, filename, obfd);
+		read, write, exec, modified_state, filename, obfd);
 	}
 
       do_cleanups (cleanup);
@@ -926,12 +929,14 @@ 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 modified_state,
 				 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, modified_state,
+		     data->obfd);
 }
 
 /* A variant of linux_find_memory_regions_full that is suitable as the
@@ -1074,7 +1079,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 modified_state,
 			      const char *filename, void *data)
 {
   struct linux_make_mappings_data *map_data = data;
@@ -1872,7 +1878,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 modified_state,
 		   void *data)
 {
   struct mem_range *range = data;
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);
 }
 
-- 
1.9.3

[PATCH 2/4] Update gcore_create_callback to better handle different states of mappings

[Sergio Durigan Junior]

This patch updates gdb/gcore.c's gcore_create_callback function and
changes its logic to improve the handling of different states of
memory mappings.

One of the major modifications is that mappings marked as UNMODIFIED
are now completely ignored by the function (i.e., not even the segment
headers are created anymore).  This is now possible because of the
introduction of the new UNKNOWN state (see below).  We now know that
when the mapping is UNMODIFIED, it means that the user has either
chose to ignore it via /proc/PID/coredump_filter, or that it is marked
as VM_DONTDUMP (and therefore should not be dumped anyway).  This is
what the Linux kernel does, too.

The new UNKNOWN state is being used to identify situations when we
don't really know whether the mapping should be dumped or not.  Based
on that, we run an existing heuristic responsible for deciding if we
should include the mapping's contents or not.

One last thing worth mentioning is that this check:

  if (read == 0 && write == 0 && exec == 0
      && modified_state == MEMORY_MAPPING_UNMODIFIED)

has been simplified to:

  if (read == 0)

This is because if the mapping has not 'read' permission set, it does
not make sense to include its contents in the corefile (GDB would
actually not be allowed to do that).  Therefore, we just create a
segment header for it.  The Linux kernel differs from GDB here because
it actually include the contents of this mapping in the corefile, but
this is because it can read its contents.

Changes from v2:

  - Return immediately if modified_state == MEMORY_MAPPING_UNMODIFIED

  - Improve comments explaining the case above, and also the case when
    "read == 0".

gdb/ChangeLog:
2015-03-18  Sergio Durigan Junior  <sergiodj@redhat.com>
	    Jan Kratochvil  <jan.kratochvil@redhat.com>
	    Oleg Nesterov  <oleg@redhat.com>

	PR corefiles/16092
	* gcore.c (gcore_create_callback): Update code to handle the case
	when 'modified_state == MEMORY_MAPPING_UNMODIFIED'.  Simplify
	condition used to decide when to create only a segment header for
	the mapping.  Improve check to decide when to run a heuristic to
	decide whether to dump the mapping's contents.
---
 gdb/gcore.c | 39 +++++++++++++++++++++++++--------------
 1 file changed, 25 insertions(+), 14 deletions(-)

diff --git a/gdb/gcore.c b/gdb/gcore.c
index 751ddac..8dfcc02 100644
--- a/gdb/gcore.c
+++ b/gdb/gcore.c
@@ -422,23 +422,34 @@ gcore_create_callback (CORE_ADDR vaddr, unsigned long size, int read,
   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_state == MEMORY_MAPPING_UNMODIFIED)
+  if (modified_state == MEMORY_MAPPING_UNMODIFIED)
     {
-      if (info_verbose)
-        {
-          fprintf_filtered (gdb_stdout, "Ignore segment, %s bytes at %s\n",
-                            plongest (size), paddress (target_gdbarch (), vaddr));
-        }
-
+      /* When the memory mapping is marked as unmodified, this means
+	 that it should not be included in the coredump file (either
+	 because it was marked as VM_DONTDUMP, or because the user
+	 explicitly chose to ignore it using the
+	 /proc/PID/coredump_filter mechanism).
+
+	 We could in theory create a section header for it (i.e., mark
+	 it as '~(SEC_LOAD | SEC_HAS_CONTENTS)', just like we do when
+	 the mapping does not have the 'read' permission set), but the
+	 Linux kernel itself ignores these mappings, and so do we.  */
       return 0;
     }
-
-  if (write == 0 && modified_state == MEMORY_MAPPING_UNMODIFIED
-      && !solib_keep_data_in_core (vaddr, size))
+  else if (read == 0)
+    {
+      /* If the memory segment has no read permission set, 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.
+
+	 The Linux kernel differs from GDB in this point because it
+	 can actually read this mapping, so it dumps this mapping's
+	 contents in the corefile.  */
+      flags &= ~(SEC_LOAD | SEC_HAS_CONTENTS);
+    }
+  else if (write == 0 && modified_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.  */
-- 
1.9.3

[PATCH 3/4] Implement support for checking /proc/PID/coredump_filter

[Sergio Durigan Junior]

This is the core patch.  It implements the support for honoring the
/proc/PID/coredump_filter file.

The code haven't changed much from v2.  I basically included more
comments (or extended the existing ones) explaining things that Pedro
asked.

Changes from v2:

  - Improved error mechanism on mapping_is_anonymous_p.

  - Improved comments on mapping_is_anonymous_p.

gdb/ChangeLog:
2015-03-18  Sergio Durigan Junior  <sergiodj@redhat.com>
	    Jan Kratochvil  <jan.kratochvil@redhat.com>
	    Oleg Nesterov  <oleg@redhat.com>

	PR corefiles/16092
	* 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_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.
	(show_use_coredump_filter): New function.
	(_initialize_linux_tdep): New command 'set use-coredump-filter'.
---
 gdb/linux-tdep.c | 403 +++++++++++++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 379 insertions(+), 24 deletions(-)

diff --git a/gdb/linux-tdep.c b/gdb/linux-tdep.c
index 15725c7..345c178 100644
--- a/gdb/linux-tdep.c
+++ b/gdb/linux-tdep.c
@@ -35,9 +35,61 @@
 #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,
+  };
+
+/* This struct is used to map flags found in the "VmFlags:" field (in
+   the /proc/<PID>/smaps file).  */
+
+struct smaps_vmflags
+  {
+    /* 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 +433,220 @@ 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)
+{
+  char *saveptr;
+  const char *s;
+
+  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)
+{
+  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);
+
+      /* Let's be pessimistic and assume there will be an error while
+	 compiling the regex'es.  */
+      init_regex_p = -1;
+
+      /* DEV_ZERO_REGEX matches "/dev/zero" filenames (with or
+	 without the "(deleted)" string in the end).  We know for
+	 sure, based on the Linux kernel code, that memory mappings
+	 whose associated filename is "/dev/zero" are guaranteed to be
+	 MAP_ANONYMOUS.  */
+      compile_rx_or_error (&dev_zero_regex, "^/dev/zero\\( (deleted)\\)\\?$",
+			   _("Could not compile regex to match /dev/zero "
+			     "filename"));
+      /* SHMEM_FILE_REGEX matches "/SYSV%08x" filenames (with or
+	 without the "(deleted)" string in the end).  These filenames
+	 refer to shared memory (shmem), and memory mappings
+	 associated with them are MAP_ANONYMOUS as well.  */
+      compile_rx_or_error (&shmem_file_regex,
+			   "^/\\?SYSV[0-9a-fA-F]\\{8\\}\\( (deleted)\\)\\?$",
+			   _("Could not compile regex to match shmem "
+			     "filenames"));
+      /* 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);
+
+      /* If we reached this point, then everything succeeded.  */
+      init_regex_p = 1;
+    }
+
+  if (init_regex_p == -1)
+    {
+      /* There was an error while compiling the regex'es above.  In
+	 order to try to give some reliable information to the caller,
+	 we just try to find the string "(deleted)" in the filename.
+	 If we managed to find it, then we assume the mapping is
+	 anonymous.  */
+      return strstr (filename, "(deleted)") != NULL;
+    }
+
+  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;
+
+  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.
+
+   In a nutshell, this is the logic that we follow in order to decide
+   if a mapping should be dumped or not.
+
+   - If the mapping is associated to a file whose name ends with "
+     (deleted)", or if the file is "/dev/zero", or if it is
+     "/SYSV%08x" (shared memory), or if there is no file associated
+     with it, or if the AnonHugePages: or the Anonymous: fields in the
+     /proc/PID/smaps have contents, then GDB considers this mapping to
+     be anonymous.  Otherwise, GDB considers this mapping to be a
+     file-backed mapping (because there will be a file associated with
+     it).
+ 
+     It is worth mentioning that, from all those checks described
+     above, the most fragile is the one to see if the file name ends
+     with " (deleted)".  This does not necessarily mean that the
+     mapping is anonymous, because the deleted file associated with
+     the mapping may have been a hard link to another file, for
+     example.  The Linux kernel checks to see if "i_nlink == 0", but
+     GDB cannot easily do this check.  Therefore, we made a compromise
+     here, and we assume that if the file name ends with " (deleted)",
+     then the mapping is indeed anonymous.  FWIW, this is something
+     the Linux kernel could do better: expose this information in a
+     more direct way.
+ 
+   - If we see the flag "sh" in the "VmFlags:" field (in
+     /proc/PID/smaps), then certainly the memory mapping is shared
+     (VM_SHARED).  If we have access to the VmFlags, and we don't see
+     the "sh" there, then certainly the mapping is private.  However,
+     Linux kernels before commit
+     834f82e2aa9a8ede94b17b656329f850c1471514 do not have the
+     "VmFlags:" field; in that case, we use another heuristic: if we
+     see 'p' in the permission flags, then we assume that the mapping
+     is private, even though the presence of the 's' flag there would
+     mean VM_MAYSHARE, which means the mapping could still be private.
+     This should work OK enough, however.  */
+
+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.  */
+  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;
+
+      /* Update our notion of whether this mapping is shared or
+	 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
@@ -821,49 +1087,85 @@ 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;
-	  enum memory_mapping_state modified_state = MEMORY_MAPPING_UNMODIFIED;
+	  int read, write, exec, private;
+	  enum memory_mapping_state modified_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 a Linux kernel released before the commit
+	     834f82e2aa9a8ede94b17b656329f850c1471514, we will not
+	     have the 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];
 
@@ -872,11 +1174,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;
@@ -887,15 +1195,39 @@ linux_find_memory_regions_full (struct gdbarch *gdbarch,
 			       mapsfilename);
 		      break;
 		    }
-		  if (number != 0)
-		    modified_state = MEMORY_MAPPING_MODIFIED;
+		  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_state = MEMORY_MAPPING_MODIFIED;
+	  /* If a mapping should not be dumped we still should create
+	     a segment for it, just without SEC_LOAD (see
+	     gcore_create_callback).  */
+	  if (has_anonymous)
+	    {
+	      if (dump_mapping_p (filterflags, &v, private, mapping_anon_p,
+				  filename))
+		modified_state = MEMORY_MAPPING_MODIFIED;
+	      else
+		modified_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.  */
+	      modified_state = MEMORY_MAPPING_UNKNOWN_STATE;
+	    }
 
 	  /* Invoke the callback function to create the corefile segment.  */
 	  func (addr, endaddr - addr, offset, inode,
@@ -1979,6 +2311,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.  */
 
@@ -2015,4 +2358,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);
 }
-- 
1.9.3

[PATCH v3 0/4] Improve corefile generation by using /proc/PID/coredump_filter (PR corefile/16902)

[Sergio Durigan Junior]

Hello,

This is the third version of the patch.  It addresses comments made by
Pedro and Eli.  I think each patch contains enough description to be
self-explanatory, so I am not repeating everything here.

Worth mentioning: I intend to merge all patches and push this fix in a
single commit when it gets approved.  I only splited the patches now
because this makes it easier for reviewers.

Thanks,

[PATCH 4/4] Documentation and testcase

[Sergio Durigan Junior]

This patch extends the documentation to include the new feature, and
implements a new testcase for it.

Changes from v2:

  - Fixes on docs proposed by Pedro and Eli

  - Extended comments on testcase as requested by Pedro

gdb/doc/ChangeLog:
2015-03-18  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-18  Sergio Durigan Junior  <sergiodj@redhat.com>

	PR corefiles/16092
	* gdb.base/coredump-filter.c: New file.
	* gdb.base/coredump-filter.exp: Likewise.
---
 gdb/doc/gdb.texinfo                        |  33 ++++++++
 gdb/testsuite/gdb.base/coredump-filter.c   |  61 ++++++++++++++
 gdb/testsuite/gdb.base/coredump-filter.exp | 128 +++++++++++++++++++++++++++++
 3 files changed, 222 insertions(+)
 create mode 100644 gdb/testsuite/gdb.base/coredump-filter.c
 create mode 100644 gdb/testsuite/gdb.base/coredump-filter.exp

diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index 9e71642..bd372ed 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -10952,6 +10952,39 @@ 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.  @var{pid} is the process ID of a currently running process.
+
+
+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.  For more
+information about the bits that can be set in the
+@file{/proc/@var{pid}/coredump_filter} file, please refer to the
+manpage of @code{core(5)}.
+
+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
+to decide which pages will be dumped in the core dump file.  This
+value is currently @code{0x33}, which means that bits @code{0}
+(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/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..b7eb74d
--- /dev/null
+++ b/gdb/testsuite/gdb.base/coredump-filter.exp
@@ -0,0 +1,128 @@
+# 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 "could not compile test program"
+    return -1
+}
+
+if { ![runto_main] } {
+    untested "could not run to main"
+    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"
+
+    remote_exec target "sh -c \"echo $filter_flag > /proc/$ipid/coredump_filter\""
+
+    # Generate a corefile
+    gdb_gcore_cmd "$core" "save corefile"
+}
+
+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
+    }
+
+    # The variables are 'char', and using it here would be OK because
+    # GDB actually reads the contents of the memory (i.e., it
+    # dereferences the pointer).  However, to make it clear that we
+    # are interested not in the pointer itself, but in the memory it
+    # points to, we are using '*(unsigned int *)'.
+    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.
+#
+# 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 "corefile generation is not supported"
+    return -1
+}
+
+# Getting the inferior's PID
+set infpid ""
+gdb_test_multiple "info inferiors" "getting inferior pid" {
+    -re "process \($decimal\).*\r\n$gdb_prompt $" {
+	set infpid $expect_out(1,string)
+    }
+}
+
+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)
+	    }
+	}
+    }
+}
+
+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]
+    }
+}
-- 
1.9.3

Re: [PATCH 4/4] Documentation and testcase

[Eli Zaretskii]

> From: Sergio Durigan Junior <sergiodj@redhat.com>
> Cc: Pedro Alves <palves@redhat.com>,        Sergio Durigan Junior <sergiodj@redhat.com>
> Date: Wed, 18 Mar 2015 15:38:43 -0400
> 
> This patch extends the documentation to include the new feature, and
> implements a new testcase for it.

Thanks, the docs are OK.

> +value is currently @code{0x33}, which means that bits @code{0}
> +(anonymous private mappings), @code{1} (anonymous shared mappings) and
> +@code{4} (ELF headers) are active.

What happened to bit 5?

Re: [PATCH 4/4] Documentation and testcase

[Sergio Durigan Junior]

On Wednesday, March 18 2015, Eli Zaretskii wrote:

>> From: Sergio Durigan Junior <sergiodj@redhat.com>
>> Cc: Pedro Alves <palves@redhat.com>,        Sergio Durigan Junior <sergiodj@redhat.com>
>> Date: Wed, 18 Mar 2015 15:38:43 -0400
>> 
>> This patch extends the documentation to include the new feature, and
>> implements a new testcase for it.
>
> Thanks, the docs are OK.

Thanks.

>> +value is currently @code{0x33}, which means that bits @code{0}
>> +(anonymous private mappings), @code{1} (anonymous shared mappings) and
>> +@code{4} (ELF headers) are active.
>
> What happened to bit 5?

Same mistake that I made on the first e-mail.  Fixed now.

Thanks,

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

gdb/doc/ChangeLog:
2015-03-18  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-18  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/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index 552da31..5382e91 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -10952,6 +10952,39 @@ 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.  @var{pid} is the process ID of a currently running process.
+
+
+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.  For more
+information about the bits that can be set in the
+@file{/proc/@var{pid}/coredump_filter} file, please refer to the
+manpage of @code{core(5)}.
+
+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
+to decide which pages will be dumped in the core dump file.  This
+value is currently @code{0x33}, which means that bits @code{0}
+(anonymous private mappings), @code{1} (anonymous shared mappings),
+@code{4} (ELF headers) and @code{5} (private huge pages) are active.
+This will cause these memory mappings to be dumped automatically.
 @end table
 
 @node Character Sets
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..b7eb74d
--- /dev/null
+++ b/gdb/testsuite/gdb.base/coredump-filter.exp
@@ -0,0 +1,128 @@
+# 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 "could not compile test program"
+    return -1
+}
+
+if { ![runto_main] } {
+    untested "could not run to main"
+    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"
+
+    remote_exec target "sh -c \"echo $filter_flag > /proc/$ipid/coredump_filter\""
+
+    # Generate a corefile
+    gdb_gcore_cmd "$core" "save corefile"
+}
+
+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
+    }
+
+    # The variables are 'char', and using it here would be OK because
+    # GDB actually reads the contents of the memory (i.e., it
+    # dereferences the pointer).  However, to make it clear that we
+    # are interested not in the pointer itself, but in the memory it
+    # points to, we are using '*(unsigned int *)'.
+    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.
+#
+# 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 "corefile generation is not supported"
+    return -1
+}
+
+# Getting the inferior's PID
+set infpid ""
+gdb_test_multiple "info inferiors" "getting inferior pid" {
+    -re "process \($decimal\).*\r\n$gdb_prompt $" {
+	set infpid $expect_out(1,string)
+    }
+}
+
+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)
+	    }
+	}
+    }
+}
+
+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]
+    }
+}

Re: [PATCH 1/4] Improve identification of memory mappings

[Pedro Alves]

Thanks for splitting.  It indeed makes it easier to understand
the changes.

On 03/18/2015 07:38 PM, Sergio Durigan Junior wrote:
> This commit implements the new 'enum memory_mapping_state', which can
> be used to represent the different states of each memory mapping from
> the inferior.  These states are:
> 
>   - MODIFIED, which means that the mapping should be dumped in
>     corefiles
> 
>   - UNMODIFIED, which means that the mapping should not be dumped in
>     corefiles (e.g., mappings that have been marked as VM_DONTDUMP), and
> 
>   - UNKNOWN, which means that we don't know whether the mapping should
>     or should not be dumped.
> 

I'm sorry to push back on this, but it sounds to me that this is mixing
up orthogonal aspects.

For example, what if a mapping is indeed modified, but the tdep code
decides it should not be dumped?  With this interface, you need to
"lie" and pass down UNMODIFIED.

And then, what if a mapping is definitely not modified, but the
tdep code decides it should dumped (e.g., say we could tell that the
vdso mapping really wasn't modified, but we still want to dump
it anyhow because there's no file on the filesystem to read the
vdso contents from later at core load time).  With this interface,
you need to pass down either MODIFIED or UNKNOWN.

So it sounds to me that instead, the arch/target code that is walking
the memory mappings should just not call the "dump this mapping"
callback if it decides the mapping should not be dumped.

And if we do _that_ first, then, what other changes to
gcore_create_callback would be required to make it do what
we need?

This may need further discussion, but in any case, note that the
descriptions above of what each state means ...

> +/* Enum used to inform the state of a memory mapping.  This is used in
> +   functions implementing find_memory_region_ftype.  */
> +
> +enum memory_mapping_state
> +  {
> +    MEMORY_MAPPING_MODIFIED,
> +    MEMORY_MAPPING_UNMODIFIED,
> +    MEMORY_MAPPING_UNKNOWN_STATE,
> +  };

... should be here in the code.

Thanks,
Pedro Alves

Re: [PATCH 2/4] Update gcore_create_callback to better handle different states of mappings

[Pedro Alves]

On 03/18/2015 07:38 PM, Sergio Durigan Junior wrote:
> This patch updates gdb/gcore.c's gcore_create_callback function and
> changes its logic to improve the handling of different states of
> memory mappings.
> 
> One of the major modifications is that mappings marked as UNMODIFIED
> are now completely ignored by the function (i.e., not even the segment
> headers are created anymore).  This is now possible because of the
> introduction of the new UNKNOWN state (see below).  We now know that
> when the mapping is UNMODIFIED, it means that the user has either
> chose to ignore it via /proc/PID/coredump_filter, or that it is marked
> as VM_DONTDUMP (and therefore should not be dumped anyway).  This is
> what the Linux kernel does, too.
> 
> The new UNKNOWN state is being used to identify situations when we
> don't really know whether the mapping should be dumped or not.  Based
> on that, we run an existing heuristic responsible for deciding if we
> should include the mapping's contents or not.
> 
> One last thing worth mentioning is that this check:
> 
>   if (read == 0 && write == 0 && exec == 0
>       && modified_state == MEMORY_MAPPING_UNMODIFIED)
> 
> has been simplified to:
> 
>   if (read == 0)
> 
> This is because if the mapping has not 'read' permission set, it does
> not make sense to include its contents in the corefile (GDB would
> actually not be allowed to do that). 

I don't think this is true.  GDB can certainly read memory of
mappings that don't have the read permission bit set: ptrace
bypasses the memory protections.  Otherwise, how could gdb poke
breakpoints instructions?  AFAICS, it can even read memory mapped
with no permissions at all:

(top-gdb) info inferiors
  Num  Description       Executable
* 1    process 24337     /home/pedro/gdb/mygit/build/gdb/gdb
(top-gdb) shell cat /proc/24337/maps
...
3615fb4000-36161b3000 ---p 001b4000 fd:00 263789                         /usr/lib64/libc-2.18.so
...
(top-gdb) x/4b 0x3615fb4000
0x3615fb4000:   0xf6    0x9a    0xf7    0x15


> Therefore, we just create a
> segment header for it.  The Linux kernel differs from GDB here because
> it actually include the contents of this mapping in the corefile, but
> this is because it can read its contents.
> 
> Changes from v2:
> 
>   - Return immediately if modified_state == MEMORY_MAPPING_UNMODIFIED
> 
>   - Improve comments explaining the case above, and also the case when
>     "read == 0".
> 
> gdb/ChangeLog:
> 2015-03-18  Sergio Durigan Junior  <sergiodj@redhat.com>
> 	    Jan Kratochvil  <jan.kratochvil@redhat.com>
> 	    Oleg Nesterov  <oleg@redhat.com>
> 
> 	PR corefiles/16092
> 	* gcore.c (gcore_create_callback): Update code to handle the case
> 	when 'modified_state == MEMORY_MAPPING_UNMODIFIED'.  Simplify
> 	condition used to decide when to create only a segment header for
> 	the mapping.  Improve check to decide when to run a heuristic to
> 	decide whether to dump the mapping's contents.
> ---
>  gdb/gcore.c | 39 +++++++++++++++++++++++++--------------
>  1 file changed, 25 insertions(+), 14 deletions(-)
> 
> diff --git a/gdb/gcore.c b/gdb/gcore.c
> index 751ddac..8dfcc02 100644
> --- a/gdb/gcore.c
> +++ b/gdb/gcore.c
> @@ -422,23 +422,34 @@ gcore_create_callback (CORE_ADDR vaddr, unsigned long size, int read,
>    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_state == MEMORY_MAPPING_UNMODIFIED)
> +  if (modified_state == MEMORY_MAPPING_UNMODIFIED)
>      {
> -      if (info_verbose)
> -        {
> -          fprintf_filtered (gdb_stdout, "Ignore segment, %s bytes at %s\n",
> -                            plongest (size), paddress (target_gdbarch (), vaddr));
> -        }
> -
> +      /* When the memory mapping is marked as unmodified, this means
> +	 that it should not be included in the coredump file (either
> +	 because it was marked as VM_DONTDUMP, or because the user
> +	 explicitly chose to ignore it using the
> +	 /proc/PID/coredump_filter mechanism).
> +
> +	 We could in theory create a section header for it (i.e., mark
> +	 it as '~(SEC_LOAD | SEC_HAS_CONTENTS)', just like we do when
> +	 the mapping does not have the 'read' permission set), but the
> +	 Linux kernel itself ignores these mappings, and so do we.  */
>        return 0;

It really seems wrong to me to bake in Linuxisms into this generic
function, shared with all supported configurations.  As mentioned
elsewhere, I think we should instead leave it up to the (Linux-specific)
caller to simply not call this function if the mapping shouldn't
be dumped at all.

>      }
> -
> -  if (write == 0 && modified_state == MEMORY_MAPPING_UNMODIFIED
> -      && !solib_keep_data_in_core (vaddr, size))
> +  else if (read == 0)
> +    {
> +      /* If the memory segment has no read permission set, 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.
> +
> +	 The Linux kernel differs from GDB in this point because it
> +	 can actually read this mapping, so it dumps this mapping's
> +	 contents in the corefile.  */
> +      flags &= ~(SEC_LOAD | SEC_HAS_CONTENTS);
> +    }
> +  else if (write == 0 && modified_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.  */
> 

Thanks,
Pedro Alves

Re: [PATCH 3/4] Implement support for checking /proc/PID/coredump_filter

[Pedro Alves]

On 03/18/2015 07:38 PM, Sergio Durigan Junior wrote:
> +	  /* If a mapping should not be dumped we still should create
> +	     a segment for it, just without SEC_LOAD (see
> +	     gcore_create_callback).  */

I think this comment is stale now?

> +	  if (has_anonymous)
> +	    {
> +	      if (dump_mapping_p (filterflags, &v, private, mapping_anon_p,
> +				  filename))
> +		modified_state = MEMORY_MAPPING_MODIFIED;
> +	      else
> +		modified_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.  */
> +	      modified_state = MEMORY_MAPPING_UNKNOWN_STATE;
> +	    }

Thanks,
Pedro Alves

Re: [PATCH 1/4] Improve identification of memory mappings

[Sergio Durigan Junior]

On Thursday, March 19 2015, Pedro Alves wrote:

> On 03/18/2015 07:38 PM, Sergio Durigan Junior wrote:
>> This commit implements the new 'enum memory_mapping_state', which can
>> be used to represent the different states of each memory mapping from
>> the inferior.  These states are:
>> 
>>   - MODIFIED, which means that the mapping should be dumped in
>>     corefiles
>> 
>>   - UNMODIFIED, which means that the mapping should not be dumped in
>>     corefiles (e.g., mappings that have been marked as VM_DONTDUMP), and
>> 
>>   - UNKNOWN, which means that we don't know whether the mapping should
>>     or should not be dumped.
>> 
>
> I'm sorry to push back on this, but it sounds to me that this is mixing
> up orthogonal aspects.
>
> For example, what if a mapping is indeed modified, but the tdep code
> decides it should not be dumped?  With this interface, you need to
> "lie" and pass down UNMODIFIED.
>
> And then, what if a mapping is definitely not modified, but the
> tdep code decides it should dumped (e.g., say we could tell that the
> vdso mapping really wasn't modified, but we still want to dump
> it anyhow because there's no file on the filesystem to read the
> vdso contents from later at core load time).  With this interface,
> you need to pass down either MODIFIED or UNKNOWN.
>
> So it sounds to me that instead, the arch/target code that is walking
> the memory mappings should just not call the "dump this mapping"
> callback if it decides the mapping should not be dumped.

Right, I agree there is some confusion in the terms being used here.
Thanks for giving examples that make this confusion obvious.

While I still think gcore_create_callback should probably receive more
attention, I will withdraw this patch because it doesn't really help to
fix the problem at hand, which is to make GDB obey
/proc/PID/coredump_filter.

> And if we do _that_ first, then, what other changes to
> gcore_create_callback would be required to make it do what
> we need?

If we do what you proposed, we wouldn't need to change
gcore_create_callback at all *to fix the specific problem of making GDB
obey /proc/PID/smaps*.  This is why, as I said, I am withdrawing this
patch.

However, IMHO gcore_create_callback still has some problems.  For
example, this heuristic used to determine whether a mapping should be
dumped or not:

  if (write == 0 && modified == 0 && !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.  */
      struct objfile *objfile;
      struct obj_section *objsec;

      ALL_OBJSECTIONS (objfile, objsec)
	{
	  bfd *abfd = objfile->obfd;
	  asection *asec = objsec->the_bfd_section;
	  bfd_vma align = (bfd_vma) 1 << bfd_get_section_alignment (abfd,
								    asec);
	  bfd_vma start = obj_section_addr (objsec) & -align;
	  bfd_vma end = (obj_section_endaddr (objsec) + align - 1) & -align;

	  /* Match if either the entire memory region lies inside the
	     section (i.e. a mapping covering some pages of a large
	     segment) or the entire section lies inside the memory region
	     (i.e. a mapping covering multiple small sections).

	     This BFD was synthesized from reading target memory,
	     we don't want to omit that.  */
	  if (objfile->separate_debug_objfile_backlink == NULL
	      && ((vaddr >= start && vaddr + size <= end)
	          || (start >= vaddr && end <= vaddr + size))
	      && !(bfd_get_file_flags (abfd) & BFD_IN_MEMORY))
	    {
	      flags &= ~(SEC_LOAD | SEC_HAS_CONTENTS);
	      goto keep;	/* Break out of two nested for loops.  */
	    }
	}

    keep:;
    }

will not be used by any code, because everyone will be passing
'modified' as 1 with my following patch (the only code that could pass
'modified' as zero was linux_find_memory_regions_full, which I will
patch to only pass 1 as well).

> This may need further discussion, but in any case, note that the
> descriptions above of what each state means ...
>
>> +/* Enum used to inform the state of a memory mapping.  This is used in
>> +   functions implementing find_memory_region_ftype.  */
>> +
>> +enum memory_mapping_state
>> +  {
>> +    MEMORY_MAPPING_MODIFIED,
>> +    MEMORY_MAPPING_UNMODIFIED,
>> +    MEMORY_MAPPING_UNKNOWN_STATE,
>> +  };
>
> ... should be here in the code.

This is not needed anymore.

Thanks,

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

Re: [PATCH 2/4] Update gcore_create_callback to better handle different states of mappings

[Sergio Durigan Junior]

On Thursday, March 19 2015, Pedro Alves wrote:

> On 03/18/2015 07:38 PM, Sergio Durigan Junior wrote:
>> This patch updates gdb/gcore.c's gcore_create_callback function and
>> changes its logic to improve the handling of different states of
>> memory mappings.
>> 
>> One of the major modifications is that mappings marked as UNMODIFIED
>> are now completely ignored by the function (i.e., not even the segment
>> headers are created anymore).  This is now possible because of the
>> introduction of the new UNKNOWN state (see below).  We now know that
>> when the mapping is UNMODIFIED, it means that the user has either
>> chose to ignore it via /proc/PID/coredump_filter, or that it is marked
>> as VM_DONTDUMP (and therefore should not be dumped anyway).  This is
>> what the Linux kernel does, too.
>> 
>> The new UNKNOWN state is being used to identify situations when we
>> don't really know whether the mapping should be dumped or not.  Based
>> on that, we run an existing heuristic responsible for deciding if we
>> should include the mapping's contents or not.
>> 
>> One last thing worth mentioning is that this check:
>> 
>>   if (read == 0 && write == 0 && exec == 0
>>       && modified_state == MEMORY_MAPPING_UNMODIFIED)
>> 
>> has been simplified to:
>> 
>>   if (read == 0)
>> 
>> This is because if the mapping has not 'read' permission set, it does
>> not make sense to include its contents in the corefile (GDB would
>> actually not be allowed to do that). 
>
> I don't think this is true.  GDB can certainly read memory of
> mappings that don't have the read permission bit set: ptrace
> bypasses the memory protections.  Otherwise, how could gdb poke
> breakpoints instructions?  AFAICS, it can even read memory mapped
> with no permissions at all:
>
> (top-gdb) info inferiors
>   Num  Description       Executable
> * 1    process 24337     /home/pedro/gdb/mygit/build/gdb/gdb
> (top-gdb) shell cat /proc/24337/maps
> ...
> 3615fb4000-36161b3000 ---p 001b4000 fd:00 263789                         /usr/lib64/libc-2.18.so
> ...
> (top-gdb) x/4b 0x3615fb4000
> 0x3615fb4000:   0xf6    0x9a    0xf7    0x15

Indeed, this is not true.  I apologize for making this confusion.

>> Therefore, we just create a
>> segment header for it.  The Linux kernel differs from GDB here because
>> it actually include the contents of this mapping in the corefile, but
>> this is because it can read its contents.
>> 
>> Changes from v2:
>> 
>>   - Return immediately if modified_state == MEMORY_MAPPING_UNMODIFIED
>> 
>>   - Improve comments explaining the case above, and also the case when
>>     "read == 0".
>> 
>> gdb/ChangeLog:
>> 2015-03-18  Sergio Durigan Junior  <sergiodj@redhat.com>
>> 	    Jan Kratochvil  <jan.kratochvil@redhat.com>
>> 	    Oleg Nesterov  <oleg@redhat.com>
>> 
>> 	PR corefiles/16092
>> 	* gcore.c (gcore_create_callback): Update code to handle the case
>> 	when 'modified_state == MEMORY_MAPPING_UNMODIFIED'.  Simplify
>> 	condition used to decide when to create only a segment header for
>> 	the mapping.  Improve check to decide when to run a heuristic to
>> 	decide whether to dump the mapping's contents.
>> ---
>>  gdb/gcore.c | 39 +++++++++++++++++++++++++--------------
>>  1 file changed, 25 insertions(+), 14 deletions(-)
>> 
>> diff --git a/gdb/gcore.c b/gdb/gcore.c
>> index 751ddac..8dfcc02 100644
>> --- a/gdb/gcore.c
>> +++ b/gdb/gcore.c
>> @@ -422,23 +422,34 @@ gcore_create_callback (CORE_ADDR vaddr, unsigned long size, int read,
>>    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_state == MEMORY_MAPPING_UNMODIFIED)
>> +  if (modified_state == MEMORY_MAPPING_UNMODIFIED)
>>      {
>> -      if (info_verbose)
>> -        {
>> -          fprintf_filtered (gdb_stdout, "Ignore segment, %s bytes at %s\n",
>> -                            plongest (size), paddress (target_gdbarch (), vaddr));
>> -        }
>> -
>> +      /* When the memory mapping is marked as unmodified, this means
>> +	 that it should not be included in the coredump file (either
>> +	 because it was marked as VM_DONTDUMP, or because the user
>> +	 explicitly chose to ignore it using the
>> +	 /proc/PID/coredump_filter mechanism).
>> +
>> +	 We could in theory create a section header for it (i.e., mark
>> +	 it as '~(SEC_LOAD | SEC_HAS_CONTENTS)', just like we do when
>> +	 the mapping does not have the 'read' permission set), but the
>> +	 Linux kernel itself ignores these mappings, and so do we.  */
>>        return 0;
>
> It really seems wrong to me to bake in Linuxisms into this generic
> function, shared with all supported configurations.  As mentioned
> elsewhere, I think we should instead leave it up to the (Linux-specific)
> caller to simply not call this function if the mapping shouldn't
> be dumped at all.

I totally agree.  As I did with the patch to add the new enum
memory_mapping_state, I am also withdrawing this patch.  I will send a
new series soon.

Thanks,

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

Re: [PATCH 3/4] Implement support for checking /proc/PID/coredump_filter

[Sergio Durigan Junior]

On Thursday, March 19 2015, Pedro Alves wrote:

> On 03/18/2015 07:38 PM, Sergio Durigan Junior wrote:
>> +	  /* If a mapping should not be dumped we still should create
>> +	     a segment for it, just without SEC_LOAD (see
>> +	     gcore_create_callback).  */
>
> I think this comment is stale now?

Yes.  Fixed.

Thanks,

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

Re: [PATCH 1/4] Improve identification of memory mappings

[Pedro Alves]

On 03/19/2015 11:06 PM, Sergio Durigan Junior wrote:

> However, IMHO gcore_create_callback still has some problems.  For
> example, this heuristic used to determine whether a mapping should be
> dumped or not:
> 
>   if (write == 0 && modified == 0 && !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.  */
>       struct objfile *objfile;
>       struct obj_section *objsec;
> 
>       ALL_OBJSECTIONS (objfile, objsec)
> 	{
> 	  bfd *abfd = objfile->obfd;
> 	  asection *asec = objsec->the_bfd_section;
> 	  bfd_vma align = (bfd_vma) 1 << bfd_get_section_alignment (abfd,
> 								    asec);
> 	  bfd_vma start = obj_section_addr (objsec) & -align;
> 	  bfd_vma end = (obj_section_endaddr (objsec) + align - 1) & -align;
> 
> 	  /* Match if either the entire memory region lies inside the
> 	     section (i.e. a mapping covering some pages of a large
> 	     segment) or the entire section lies inside the memory region
> 	     (i.e. a mapping covering multiple small sections).
> 
> 	     This BFD was synthesized from reading target memory,
> 	     we don't want to omit that.  */
> 	  if (objfile->separate_debug_objfile_backlink == NULL
> 	      && ((vaddr >= start && vaddr + size <= end)
> 	          || (start >= vaddr && end <= vaddr + size))
> 	      && !(bfd_get_file_flags (abfd) & BFD_IN_MEMORY))
> 	    {
> 	      flags &= ~(SEC_LOAD | SEC_HAS_CONTENTS);
> 	      goto keep;	/* Break out of two nested for loops.  */
> 	    }
> 	}
> 
>     keep:;
>     }
> 
> will not be used by any code, because everyone will be passing
> 'modified' as 1 with my following patch (the only code that could pass
> 'modified' as zero was linux_find_memory_regions_full, which I will
> patch to only pass 1 as well).

Alright.  Good that that now became clear.

I found the initial submission for that, btw:

  https://sourceware.org/ml/gdb-patches/2003-10/msg00164.html

I wonder whether it'd be worth to keep that somehow, for the fallback
cases when /proc//smaps or some other /proc file you're relying
on for file-backed read-only region identification is missing
(because old kernel, or even /proc not mounted).  Maybe not.

Thanks,
Pedro Alves

Re: [PATCH 1/4] Improve identification of memory mappings

[Sergio Durigan Junior]

On Friday, March 20 2015, Pedro Alves wrote:

> On 03/19/2015 11:06 PM, Sergio Durigan Junior wrote:
>
>> However, IMHO gcore_create_callback still has some problems.  For
>> example, this heuristic used to determine whether a mapping should be
>> dumped or not:
>> 
>>   if (write == 0 && modified == 0 && !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.  */
>>       struct objfile *objfile;
>>       struct obj_section *objsec;
>> 
>>       ALL_OBJSECTIONS (objfile, objsec)
>> 	{
>> 	  bfd *abfd = objfile->obfd;
>> 	  asection *asec = objsec->the_bfd_section;
>> 	  bfd_vma align = (bfd_vma) 1 << bfd_get_section_alignment (abfd,
>> 								    asec);
>> 	  bfd_vma start = obj_section_addr (objsec) & -align;
>> 	  bfd_vma end = (obj_section_endaddr (objsec) + align - 1) & -align;
>> 
>> 	  /* Match if either the entire memory region lies inside the
>> 	     section (i.e. a mapping covering some pages of a large
>> 	     segment) or the entire section lies inside the memory region
>> 	     (i.e. a mapping covering multiple small sections).
>> 
>> 	     This BFD was synthesized from reading target memory,
>> 	     we don't want to omit that.  */
>> 	  if (objfile->separate_debug_objfile_backlink == NULL
>> 	      && ((vaddr >= start && vaddr + size <= end)
>> 	          || (start >= vaddr && end <= vaddr + size))
>> 	      && !(bfd_get_file_flags (abfd) & BFD_IN_MEMORY))
>> 	    {
>> 	      flags &= ~(SEC_LOAD | SEC_HAS_CONTENTS);
>> 	      goto keep;	/* Break out of two nested for loops.  */
>> 	    }
>> 	}
>> 
>>     keep:;
>>     }
>> 
>> will not be used by any code, because everyone will be passing
>> 'modified' as 1 with my following patch (the only code that could pass
>> 'modified' as zero was linux_find_memory_regions_full, which I will
>> patch to only pass 1 as well).
>
> Alright.  Good that that now became clear.
>
> I found the initial submission for that, btw:
>
>   https://sourceware.org/ml/gdb-patches/2003-10/msg00164.html

Thanks for doing that.

> I wonder whether it'd be worth to keep that somehow, for the fallback
> cases when /proc//smaps or some other /proc file you're relying
> on for file-backed read-only region identification is missing
> (because old kernel, or even /proc not mounted).  Maybe not.

I will not touch this code in this series, of course, but I also think
that this part should be used more often.  For example, there are cases
when we are not sure whether the mapping needs to be dumped or not
(e.g., see gdb/gnu-nat.c), and currently we are just passing 'modified =
1', which makes GDB dump those pages.  I guess this is a situation where
this code could help.

Anyway, something that I can look into later.

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