Re: [PATCH+8.3?] Implement dump of mappings with ELF headers by gcore

[Kevin Buettner <kevinb at redhat dot com>]

On Tue, 23 Apr 2019 19:46:35 -0400
Sergio Durigan Junior <sergiodj@redhat.com> wrote:

> This patch has a long story, but it all started back in 2015, with
[...]

Thanks for the detailed commit comment!

[...]
> I think it makes sense to include this patch into 8.3, since it's
> pretty "self-contained", but I will leave that decision to the GMs.
> 
> gdb/ChangeLog:
> 2019-04-24  Sergio Durigan Junior  <sergiodj@redhat.com>
> 
> 	PR corefiles/11608
> 	PR corefiles/18187
> 	* linux-tdep.c (dump_mapping_p): Add new parameters "ADDR" and
> 	"OFFSET".  Verify if current mapping contains an ELF header.
> 	(linux_find_memory_regions_full): Adjust call to
> 	"dump_mapping_p".

I don't think that the double quotes enclosing ADDR and OFFSET are
needed here.

> gdb/testsuite/ChangeLog:
> 2019-04-24  Sergio Durigan Junior  <sergiodj@redhat.com>
> 
> 	PR corefiles/11608
> 	PR corefiles/18187
> 	* gdb.base/coredump-filter-build-id.exp: New file.
> 	* lib/future.exp (gdb_find_eu-unstrip): New procedure.
> ---
>  gdb/linux-tdep.c                              | 71 +++++++++++++++----
>  .../gdb.base/coredump-filter-build-id.exp     | 69 ++++++++++++++++++
>  gdb/testsuite/lib/future.exp                  | 10 +++
>  3 files changed, 137 insertions(+), 13 deletions(-)
>  create mode 100644 gdb/testsuite/gdb.base/coredump-filter-build-id.exp
> 
> diff --git a/gdb/linux-tdep.c b/gdb/linux-tdep.c
> index 5de985def3..d71a00666f 100644
> --- a/gdb/linux-tdep.c
> +++ b/gdb/linux-tdep.c
> @@ -586,8 +586,8 @@ mapping_is_anonymous_p (const char *filename)
>  }
>  
>  /* 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.
> +   MAYBE_PRIVATE_P, MAPPING_ANONYMOUS_P, ADDR and OFFSET) 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.
> @@ -625,12 +625,17 @@ mapping_is_anonymous_p (const char *filename)
>       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.  */
> +     This should work OK enough, however.
> +
> +   - Even if, at the end, we decided that we should not dump the
> +     mapping, we still have to check if it is something like an ELF
> +     header (of a DSO or an executable, for example).  If it is, and
> +     if the user is interested in dump it, then we should dump it.  */
>  
>  static int
>  dump_mapping_p (filter_flags filterflags, const struct smaps_vmflags *v,
>  		int maybe_private_p, int mapping_anon_p, int mapping_file_p,
> -		const char *filename)
> +		const char *filename, ULONGEST addr, ULONGEST offset)

I was surprised to see that addr is ULONGEST instead of CORE_ADDR, but
I see that this matches the type of of the variables passed by the caller.
Both are typedef'd to unsigned long long, so it probably doesn't matter.

>  {
>    /* Initially, we trust in what we received from our caller.  This
>       value may not be very precise (i.e., it was probably gathered
> @@ -640,6 +645,7 @@ dump_mapping_p (filter_flags filterflags, const struct smaps_vmflags *v,
>       (assuming that the version of the Linux kernel being used
>       supports it, of course).  */
>    int private_p = maybe_private_p;
> +  int dump_p;
>  
>    /* We always dump vDSO and vsyscall mappings, because it's likely that
>       there'll be no file to read the contents from at core load time.
> @@ -680,13 +686,13 @@ dump_mapping_p (filter_flags filterflags, const struct smaps_vmflags *v,
>  	  /* This is a special situation.  It can happen when we see a
>  	     mapping that is file-backed, but that contains anonymous
>  	     pages.  */
> -	  return ((filterflags & COREFILTER_ANON_PRIVATE) != 0
> -		  || (filterflags & COREFILTER_MAPPED_PRIVATE) != 0);
> +	  dump_p = ((filterflags & COREFILTER_ANON_PRIVATE) != 0
> +		    || (filterflags & COREFILTER_MAPPED_PRIVATE) != 0);
>  	}
>        else if (mapping_anon_p)
> -	return (filterflags & COREFILTER_ANON_PRIVATE) != 0;
> +	dump_p = (filterflags & COREFILTER_ANON_PRIVATE) != 0;
>        else
> -	return (filterflags & COREFILTER_MAPPED_PRIVATE) != 0;
> +	dump_p = (filterflags & COREFILTER_MAPPED_PRIVATE) != 0;
>      }
>    else
>      {
> @@ -695,14 +701,53 @@ dump_mapping_p (filter_flags filterflags, const struct smaps_vmflags *v,
>  	  /* This is a special situation.  It can happen when we see a
>  	     mapping that is file-backed, but that contains anonymous
>  	     pages.  */
> -	  return ((filterflags & COREFILTER_ANON_SHARED) != 0
> -		  || (filterflags & COREFILTER_MAPPED_SHARED) != 0);
> +	  dump_p = ((filterflags & COREFILTER_ANON_SHARED) != 0
> +		    || (filterflags & COREFILTER_MAPPED_SHARED) != 0);
>  	}
>        else if (mapping_anon_p)
> -	return (filterflags & COREFILTER_ANON_SHARED) != 0;
> +	dump_p = (filterflags & COREFILTER_ANON_SHARED) != 0;
>        else
> -	return (filterflags & COREFILTER_MAPPED_SHARED) != 0;
> +	dump_p = (filterflags & COREFILTER_MAPPED_SHARED) != 0;
>      }
> +
> +  /* Even if we decided that we shouldn't dump this mapping, we still
> +     have to check whether (a) the user wants us to dump mappings
> +     containing an ELF header, and (b) the mapping in question
> +     contains an ELF header.  If (a) and (b) are true, then we should
> +     dump this mapping.
> +
> +     A mapping contains an ELF header if it is a private mapping, its
> +     offset is zero, and its first word is ELFMAG.  */
> +  if (!dump_p && private_p && offset == 0
> +      && (filterflags & COREFILTER_ELF_HEADERS) != 0)
> +    {
> +      /* Let's check if we have an ELF header.  */
> +      gdb::unique_xmalloc_ptr<char> header;
> +      int errcode;
> +
> +      /* Useful define specifying the size of the ELF magical
> +	 header.  */
> +#ifndef SELFMAG
> +#define SELFMAG 4
> +#endif
> +
> +      /* Read the first SELFMAG bytes and check if it is ELFMAG.  */
> +      if (target_read_string (addr, &header, SELFMAG, &errcode) == SELFMAG
> +	  && errcode == 0)
> +	{
> +	  const char *h = header.get ();
> +
> +	  if (h[EI_MAG0] == ELFMAG0 && h[EI_MAG1] == ELFMAG1
> +	      && h[EI_MAG2] == ELFMAG2 && h[EI_MAG3] == ELFMAG3)

Somewhere in here, either above the "if" or in the comment for SELFMAG,
I think it'd be useful to note that the ELFMAG0..ELFMAG3 and 
EI_MAG0..EI_MAG3 come from elf/common.h.

(My concern here was whether ELFMAG0, etc. were defined by a
system header.  If so, we can't use those defines here.  But since
they're defined in elf/common.h, they're perfectly fine.  This is
why I think it'd be useful to note where we expect them to come
from.  Otherwise, such a comment would just be clutter.)

Regarding SELFMAG, it's a shame that it's not it's not also defined in
elf/common.h.  I see that SELFMAG is also used in
gdbserver/linux-ppc-low.c.  I'm guessing that it obtains this value
from either /usr/include/elf.h or /usr/include/linux/elf.h.  (I don't
want to hold up your patch to make that header file change though; there
might be a good reason to not touch elf/common.h.)

> +	    {
> +	      /* This mapping contains an ELF header, so we
> +		 should dump it.  */
> +	      dump_p = 1;
> +	    }
> +	}
> +    }
> +
> +  return dump_p;
>  }
>  
>  /* Implement the "info proc" command.  */
> @@ -1306,7 +1351,7 @@ linux_find_memory_regions_full (struct gdbarch *gdbarch,
>  	  if (has_anonymous)
>  	    should_dump_p = dump_mapping_p (filterflags, &v, priv,
>  					    mapping_anon_p, mapping_file_p,
> -					    filename);
> +					    filename, addr, offset);
>  	  else
>  	    {
>  	      /* Older Linux kernels did not support the "Anonymous:" counter.
> diff --git a/gdb/testsuite/gdb.base/coredump-filter-build-id.exp b/gdb/testsuite/gdb.base/coredump-filter-build-id.exp
> new file mode 100644
> index 0000000000..fc2b039406
> --- /dev/null
> +++ b/gdb/testsuite/gdb.base/coredump-filter-build-id.exp
> @@ -0,0 +1,69 @@
> +# Copyright 2019 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/>.
> +
> +# Test whether GDB's gcore/generate-core-file command can dump memory
> +# mappings with ELF headers, containing a build-id note.
> +#
> +# Due to the fact that we don't have an easy way to process a corefile
> +# and look for specific notes using GDB/dejagnu, we rely on an
> +# external tool, eu-unstrip, to verify if the corefile contains
> +# build-ids.
> +
> +standard_testfile "normal.c"
> +
> +# This test is Linux x86_64 only.
> +if { ![istarget *-*-linux*] } {
> +    untested "$testfile.exp"
> +    return -1
> +}
> +if { ![istarget "x86_64-*-*"] || ![is_lp64_target] } {
> +    untested "$testfile.exp"
> +    return -1
> +}
> +
> +if { [prepare_for_testing "failed to prepare" $testfile $srcfile debug] } {
> +    return -1
> +}
> +
> +if { ![runto_main] } {
> +    untested "could not run to main"
> +    return -1
> +}
> +
> +# First we need to generate a corefile.
> +set corefilename "[standard_output_file gcore.test]"
> +if { ![gdb_gcore_cmd "$corefilename" "save corefile"] } {
> +    verbose -log "Could not save corefile"
> +    untested "$testfile.exp"
> +    return -1
> +}
> +
> +# Determine if GDB dumped the mapping containing the build-id.  This
> +# is done by invoking an external program (eu-unstrip).
> +if { [catch "exec [gdb_find_eu-unstrip] -n --core $corefilename" output] == 0 } {
> +    set line [lindex [split $output "\n"] 0]
> +    set test "gcore dumped mapping with build-id"
> +
> +    verbose -log "First line of eu-unstrip: $line"
> +
> +    if { [regexp "^${hex}\\+${hex} \[a-f0-9\]+@${hex}.*[string_to_regexp $binfile]$" $line] } {
> +	pass "$test"
> +    } else {
> +	fail "$test"
> +    }
> +} else {
> +    verbose -log "Could not execute eu-unstrip program"
> +    untested "$testfile.exp"
> +}
> diff --git a/gdb/testsuite/lib/future.exp b/gdb/testsuite/lib/future.exp
> index a56cd019b4..122e652858 100644
> --- a/gdb/testsuite/lib/future.exp
> +++ b/gdb/testsuite/lib/future.exp
> @@ -162,6 +162,16 @@ proc gdb_find_readelf {} {
>      return $readelf
>  }
>  
> +proc gdb_find_eu-unstrip {} {
> +    global EU_UNSTRIP_FOR_TARGET
> +    if [info exists EU_UNSTRIP_FOR_TARGET] {
> +	set eu_unstrip $EU_UNSTRIP_FOR_TARGET
> +    } else {
> +	set eu_unstrip [transform eu-unstrip]
> +    }
> +    return $eu_unstrip
> +}
> +
>  proc gdb_default_target_compile {source destfile type options} {
>      global target_triplet
>      global tool_root_dir

The test case LGTM though I'm not familiar with eu-unstrip.

I think it's okay for master with the nits that I found fixed.  I don't
have an opinion about including it in 8.3.

Kevin