Re: [PATCH 2/2] Documentation and testcase

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

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.

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