[2/2] Documentation and testcase

Message ID 1426807358-18295-3-git-send-email-sergiodj@redhat.com
State New, archived
Headers

Commit Message

Sergio Durigan Junior March 19, 2015, 11:22 p.m. UTC
  This patch extends the documentation to include the new feature, and
implements a new testcase for it.

Changes from v3:

  - Included 'bit 5' when mentioning the default value of
    /proc/PID/coredump_filter.

gdb/doc/ChangeLog:
2015-03-19  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-19  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
  

Comments

Pedro Alves March 20, 2015, 7:46 p.m. UTC | #1
On 03/19/2015 11:22 PM, Sergio Durigan Junior wrote:

> ---
>  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 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)}.

Might be good to mention that the settings are inherited by child
processes.  Reading this, I thought "wow, do I really need to
set every time I'm debugging a new pid/process?"

> +    # 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)"

This comment still gave me pause.  The variables are
'char *' not 'char':

  char *private_anon, *shared_anon;
  char *dont_dump;

so I guess you're referring to the issue that plain "print" would
assume they are strings and thus deference the pointer, right?

I honestly think that all that just distracts from what
we're doing.  Why not just:

   # Access the memory the addresses point to.
   gdb_test "print *(char *) $addr($var)" "\(\\\$$decimal = <error: \)?Cannot access memory at address $hex\(>\)?" \

I would never ever think to do:

   gdb_test "print (char *) $addr($var)"

to test the contents of what addr points to.  IOW, reading

   # Access the memory the addresses point to.
   gdb_test "print *(char *) $addr($var)" ...

I'd never really wonder why the leftmost '*' is in there.  It's super
obvious.

Maybe even throw in an /x for super clarity:

   gdb_test "print /x *(char *) $addr($var)" ...


> +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" } }

Does this cover the case of making sure we don't dump file-based
regions?  That's important.

If not (I assume not), we could test that by loading the core
into gdb, but _not_ the program, and then disassembling a function's
address.  It should fail.  Then load the program and disassemble
again.  It should work now.  Or something along those lines.

Thanks,
Pedro Alves
  
Sergio Durigan Junior March 20, 2015, 9:03 p.m. UTC | #2
On Friday, March 20 2015, Pedro Alves wrote:

> On 03/19/2015 11:22 PM, Sergio Durigan Junior wrote:
>
>> ---
>>  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 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)}.
>
> Might be good to mention that the settings are inherited by child
> processes.  Reading this, I thought "wow, do I really need to
> set every time I'm debugging a new pid/process?"

OK, I included a line mentioning this.

>> +    # 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)"
>
> This comment still gave me pause.  The variables are
> 'char *' not 'char':
>
>   char *private_anon, *shared_anon;
>   char *dont_dump;
>
> so I guess you're referring to the issue that plain "print" would
> assume they are strings and thus deference the pointer, right?

Exactly.

> I honestly think that all that just distracts from what
> we're doing.  Why not just:
>
>    # Access the memory the addresses point to.
>    gdb_test "print *(char *) $addr($var)" "\(\\\$$decimal = <error: \)?Cannot access memory at address $hex\(>\)?" \
>
> I would never ever think to do:
>
>    gdb_test "print (char *) $addr($var)"
>
> to test the contents of what addr points to.  IOW, reading
>
>    # Access the memory the addresses point to.
>    gdb_test "print *(char *) $addr($var)" ...
>
> I'd never really wonder why the leftmost '*' is in there.  It's super
> obvious.
>
> Maybe even throw in an /x for super clarity:
>
>    gdb_test "print /x *(char *) $addr($var)" ...

Yeah, maybe you're right, I think we've got too concerned about
something not really important here.

I replaced the comment by the one you proposed, and used the "print/x"
syntax.

>> +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" } }
>
> Does this cover the case of making sure we don't dump file-based
> regions?  That's important.

No, it doesn't cover file-backed mappings.  I didn't want to create
yet-another-file during the test.

> If not (I assume not), we could test that by loading the core
> into gdb, but _not_ the program, and then disassembling a function's
> address.  It should fail.  Then load the program and disassemble
> again.  It should work now.  Or something along those lines.

Hm, OK.  I guess I will try this approach, and if it doesn't happen then
I will see about doing a regular file-backed mapping.

I'll submit another revision of the series when I have something.
  
Pedro Alves March 20, 2015, 10:09 p.m. UTC | #3
On 03/20/2015 09:03 PM, Sergio Durigan Junior wrote:
>> > If not (I assume not), we could test that by loading the core
>> > into gdb, but _not_ the program, and then disassembling a function's
>> > address.  It should fail.  Then load the program and disassemble
>> > again.  It should work now.  Or something along those lines.
> Hm, OK.  I guess I will try this approach, and if it doesn't happen then
> I will see about doing a regular file-backed mapping.

Thanks.  If that approach doesn't work (for some odd reason), let's
try to come up with another approach that still makes sure that the
program's .text is not dumped, instead of mapping some other file.
It's important that we don't regress that specifically.

> I'll submit another revision of the series when I have something.

Thanks.

Thanks,
Pedro Alves
  

Patch

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]
+    }
+}