From FEDORA_PATCHES Mon Sep 17 00:00:00 2001

From: Sergio Durigan Junior <sergiodj@redhat.com>

Date: Tue, 23 Apr 2019 18:17:57 -0400

Subject: gdb-rhbz1371380-gcore-elf-headers.patch

;; Implement dump of mappings with ELF headers by gcore

;; RHBZ 1371380, Sergio Durigan Junior.

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.

diff --git a/gdb/linux-tdep.c b/gdb/linux-tdep.c

--- a/gdb/linux-tdep.c

+++ b/gdb/linux-tdep.c

@@ -591,8 +591,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.

@@ -630,12 +630,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

@@ -645,6 +650,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.

@@ -685,13 +691,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

     {

@@ -700,14 +706,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.  */

@@ -1311,7 +1358,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

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

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