This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH v2] Improve corefile generation by using /proc/PID/coredump_filter (PR corefile/16902)
- From: Pedro Alves <palves at redhat dot com>
- To: Sergio Durigan Junior <sergiodj at redhat dot com>, GDB Patches <gdb-patches at sourceware dot org>
- Cc: Jan Kratochvil <jan dot kratochvil at redhat dot com>, Oleg Nesterov <oleg at redhat dot com>
- Date: Fri, 13 Mar 2015 19:34:43 +0000
- Subject: Re: [PATCH v2] Improve corefile generation by using /proc/PID/coredump_filter (PR corefile/16902)
- Authentication-results: sourceware.org; auth=none
- References: <878ufc9kau dot fsf at redhat dot com> <87r3stnbdw dot fsf at redhat dot com>
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?
> +
> +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
...
> +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.
> +(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?
> + 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.
> + {
> + /* 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 ?
> +{
> + char *saveptr;
> + char *s;
Likewise.
> +
> + 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?
> +{
> + 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?
> + /* 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! */
}
> +
> + 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?
> + 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/
> + 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.
> + 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. :-)
> + 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.
> 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"
?
> + 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.
> + 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.
> +#
> +# 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
> + 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" {
...
> +
> +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?
> + }
> + }
> + }
> +}
> +
> +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