This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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


On Thursday, April 25 2019, Kevin Buettner wrote:

> 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!

Thanks for the review, Kevin.

> [...]
>> 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.

Ah, OK.  I tend to use them when I'm writing ChangeLog entries, but
that's a matter of style.  I'll remove them.

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

Yeah.  I had the same thought, and reached the same conclusion :-).

>>  {
>>    /* 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.

Alright.

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

I have a patch here to add SELFMAG and ELFMAG to elf/common.h.  I'm not
entirely sure where I should send it (binutils?  gcc?), but I plan to
propose it once I'm finished with this build-id patch.

>> +	    {
>> +	      /* 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.

Thanks, I pushed the patch below to master:

57e5e645010430b3d73f8c6a757d09f48dc8f8d5

I'll wait for another GM to give an opinion about including this into
8.3

Thanks,

-- 
Sergio
GPG key ID: 237A 54B1 0287 28BF 00EF  31F4 D0EB 7628 65FC 5E36
Please send encrypted e-mail if possible
http://sergiodj.net/

>From 57e5e645010430b3d73f8c6a757d09f48dc8f8d5 Mon Sep 17 00:00:00 2001
From: Sergio Durigan Junior <sergiodj@redhat.com>
Date: Tue, 23 Apr 2019 18:17:57 -0400
Subject: [PATCH] Implement dump of mappings with ELF headers by gcore

This patch has a long story, but it all started back in 2015, with
commit df8411da087dc05481926f4c4a82deabc5bc3859 ("Implement support
for checking /proc/PID/coredump_filter").  The purpose of that commit
was to bring GDB's corefile generation closer to what the Linux kernel
does.  However, back then, I did not implement the full support for
the dumping of memory mappings containing ELF headers (like mappings
of DSOs or executables).  These mappings were being dumped most of
time, though, because the default value of /proc/PID/coredump_filter
is 0x33, which would cause anonymous private mappings (DSOs/executable
code mappings have this type) to be dumped.  Well, until something
happened on binutils...

A while ago, I noticed something strange was happening with one of our
local testcases on Fedora GDB: it was failing due to some strange
build-id problem.  On Fedora GDB, we (unfortunately) carry a bunch of
"local" patches, and some of these patches actually extend upstream's
build-id support in order to generate more useful information for the
user of a Fedora system (for example, when the user loads a corefile
into GDB, we detect whether the executable that generated that
corefile is present, and if it's not we issue a warning suggesting
that it should be installed, while also providing the build-id of the
executable).  A while ago, Fedora GDB stopped printing those warnings.

I wanted to investigate this right away, and spent some time trying to
determine what was going on, but other things happened and I got
sidetracked.  Meanwhile, the bug started to be noticed by some of our
users, and its priority started changing.  Then, someone on IRC also
mentioned the problem, and when I tried helping him, I noticed he
wasn't running Fedora.  Hm...  So maybe the bug was *also* present
upstream.

After "some" time investigating, and with a lot of help from Keith and
others, I was finally able to determine that yes, the bug is also
present upstream, and that even though it started with a change in ld,
it is indeed a GDB issue.

So, as I said, the problem started with binutils, more specifically
after the following commit was pushed:

  commit f6aec96dce1ddbd8961a3aa8a2925db2021719bb
  Author: H.J. Lu <hjl.tools@gmail.com>
  Date:   Tue Feb 27 11:34:20 2018 -0800

      ld: Add --enable-separate-code

This commit makes ld use "-z separate-code" by default on x86-64
machines.  What this means is that code pages and data pages are now
separated in the binary, which is confusing GDB when it tries to decide
what to dump.

BTW, Fedora 28 binutils doesn't have this code, which means that
Fedora 28 GDB doesn't have the problem.  From Fedora 29 on, binutils
was rebased and incorporated the commit above, which started causing
Fedora GDB to fail.

Anyway, the first thing I tried was to pass "-z max-page-size" and
specify a bigger page size (I saw a patch that did this and was
proposed to Linux, so I thought it might help).  Obviously, this
didn't work, because the real "problem" is that ld will always use
separate pages for code and data.  So I decided to look into how GDB
dumped the pages, and that's where I found the real issue.

What happens is that, because of "-z separate-code", the first two pages
of the ELF binary are (from /proc/PID/smaps):

  00400000-00401000 r--p 00000000 fc:01 799548                             /file
  Size:                  4 kB
  KernelPageSize:        4 kB
  MMUPageSize:           4 kB
  Rss:                   4 kB
  Pss:                   4 kB
  Shared_Clean:          0 kB
  Shared_Dirty:          0 kB
  Private_Clean:         4 kB
  Private_Dirty:         0 kB
  Referenced:            4 kB
  Anonymous:             0 kB
  LazyFree:              0 kB
  AnonHugePages:         0 kB
  ShmemPmdMapped:        0 kB
  Shared_Hugetlb:        0 kB
  Private_Hugetlb:       0 kB
  Swap:                  0 kB
  SwapPss:               0 kB
  Locked:                0 kB
  THPeligible:    0
  VmFlags: rd mr mw me dw sd
  00401000-00402000 r-xp 00001000 fc:01 799548                             /file
  Size:                  4 kB
  KernelPageSize:        4 kB
  MMUPageSize:           4 kB
  Rss:                   4 kB
  Pss:                   4 kB
  Shared_Clean:          0 kB
  Shared_Dirty:          0 kB
  Private_Clean:         0 kB
  Private_Dirty:         4 kB
  Referenced:            4 kB
  Anonymous:             4 kB
  LazyFree:              0 kB
  AnonHugePages:         0 kB
  ShmemPmdMapped:        0 kB
  Shared_Hugetlb:        0 kB
  Private_Hugetlb:       0 kB
  Swap:                  0 kB
  SwapPss:               0 kB
  Locked:                0 kB
  THPeligible:    0
  VmFlags: rd ex mr mw me dw sd

Whereas before, we had only one:

  00400000-00401000 r-xp 00000000 fc:01 798593                             /file
  Size:                  4 kB
  KernelPageSize:        4 kB
  MMUPageSize:           4 kB
  Rss:                   4 kB
  Pss:                   4 kB
  Shared_Clean:          0 kB
  Shared_Dirty:          0 kB
  Private_Clean:         0 kB
  Private_Dirty:         4 kB
  Referenced:            4 kB
  Anonymous:             4 kB
  LazyFree:              0 kB
  AnonHugePages:         0 kB
  ShmemPmdMapped:        0 kB
  Shared_Hugetlb:        0 kB
  Private_Hugetlb:       0 kB
  Swap:                  0 kB
  SwapPss:               0 kB
  Locked:                0 kB
  THPeligible:    0
  VmFlags: rd ex mr mw me dw sd

Notice how we have "Anonymous" data mapped into the page.  This will be
important.

So, the way GDB decides which pages it should dump has been revamped
by my patch in 2015, and now it takes the contents of
/proc/PID/coredump_filter into account.  The default value for Linux
is 0x33, which means:

  Dump anonymous private, anonymous shared, ELF headers and HugeTLB
  private pages.

Or:

  filter_flags filterflags = (COREFILTER_ANON_PRIVATE
			      | COREFILTER_ANON_SHARED
			      | COREFILTER_ELF_HEADERS
			      | COREFILTER_HUGETLB_PRIVATE);

Now, it is important to keep in mind that GDB doesn't always have *all*
of the necessary information to exactly determine the type of a page, so
the whole algorithm is based on heuristics (you can take a look at
linux-tdep.c:dump_mapping_p and
linux-tdep.c:linux_find_memory_regions_full for more info).

Before the patch to make ld use "-z separate-code", the (single) page
containing data and code was being flagged as an anonymous (due to the
non-zero "Anonymous:" field) private (due to the "r-xp" permission),
which means that it was being dumped into the corefile.  That's why it
was working fine.

Now, as you can imagine, when "-z separate-code" is used, the *data*
page (which is where the ELF notes are, including the build-id one) now
doesn't have any "Anonymous:" mapping, so the heuristic is flagging it
as file-backed private, which is *not* dumped by default.

The next question I had to answer was: how come a corefile generated by
the Linux kernel was correct?  Well, the answer is that GDB, unlike
Linux, doesn't actually implement the COREFILTER_ELF_HEADERS support.
On Linux, even though the data page is also treated as a file-backed
private mapping, it is also checked to see if there are any ELF headers
in the page, and then, because we *do* have ELF headers there, it is
dumped.

So, after more time trying to think of ways to fix this, I was able to
implement an algorithm that reads the first few bytes of the memory
mapping being processed, and checks to see if the ELF magic code is
present.  This is basically what Linux does as well, except that, if
it finds the ELF magic code, it just dumps one page to the corefile,
whereas GDB will dump the whole mapping.  But I don't think that's a
big issue, to be honest.

It's also important to explain that we *only* perform the ELF magic
code check if:

  - The algorithm has decided *not* to dump the mapping so far, and;
  - The mapping is private, and;
  - The mapping's offset is zero, and;
  - The user has requested us to dump mappings with ELF headers.

IOW, we're not going to blindly check every mapping.

As for the testcase, I struggled even more trying to write it.  Since
our build-id support on upstream GDB is not very extensive, it's not
really possible to determine whether a corefile contains build-id
information or not just by using GDB.  So, after thinking a lot about
the problem, I decided to rely on an external tool, eu-unstrip, in
order to verify whether the dump was successful.  I verified the test
here on my machine, and everything seems to work as expected (i.e., it
fails without the patch, and works with the patch applied).  We are
working hard to upstream our "local" Fedora GDB patches, and we intend
to submit our build-id extension patches "soon", so hopefully we'll be
able to use GDB itself to perform this verification.

I built and regtested this on the BuildBot, and no problems were
found.

gdb/ChangeLog:
2019-04-25  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.

gdb/testsuite/ChangeLog:
2019-04-25  Sergio Durigan Junior  <sergiodj@redhat.com>

	PR corefiles/11608
	PR corefiles/18187
	* gdb.base/coredump-filter-build-id.exp: New file.
---
 gdb/linux-tdep.c                              | 73 +++++++++++++++----
 .../gdb.base/coredump-filter-build-id.exp     | 69 ++++++++++++++++++
 gdb/testsuite/lib/future.exp                  | 10 +++
 3 files changed, 139 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..c1666d189a 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)
 {
   /* 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,55 @@ 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 ();
+
+	  /* The EI_MAG* and ELFMAG* constants come from
+	     <elf/common.h>.  */
+	  if (h[EI_MAG0] == ELFMAG0 && h[EI_MAG1] == ELFMAG1
+	      && h[EI_MAG2] == ELFMAG2 && h[EI_MAG3] == ELFMAG3)
+	    {
+	      /* This mapping contains an ELF header, so we
+		 should dump it.  */
+	      dump_p = 1;
+	    }
+	}
+    }
+
+  return dump_p;
 }
 
 /* Implement the "info proc" command.  */
@@ -1306,7 +1353,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
-- 
2.17.2


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]