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

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

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.

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

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