[PATCH v4 00/14] Fix BZ 25631 - core file memory access problem

[PATCH v4 00/14] Fix BZ 25631 - core file memory access problem

[Kevin Buettner]

This series fixes several core file related bugs.  The bug which
started this work can be viewed here:

    https://sourceware.org/bugzilla/show_bug.cgi?id=25631

Other problems were found either during review or during development. 
I discuss these in my commit log remarks.

Only some of these patches still need review.  A brief status
of each can be found below:

 1) Remove hack for GDB which sets the section size to 0

    Nick Clifton has approved this patch.

 2) Adjust corefile.exp test to show regression after bfd hack removal
 3) section_table_xfer_memory: Replace section name with callback
    predicate
 4) Provide access to non SEC_HAS_CONTENTS core file sections
 5) Test ability to access unwritten-to mmap data in core file

    I think that Pedro is okay with #2 thru #5.

 6) Update binary_get_section_contents to seek using section's file
    position

    Nick Cliften has approved patch #6.

 7) Add new gdbarch method, read_core_file_mappings

    This is a new patch which needs review.

 8) Use NT_FILE note section for reading core target memory

    This patch is significantly different than the v3 patch.
    It needs review.  

 9) Add test for accessing read-only mmapped data in a core file

    I think that Pedro is oky with this one.

10) gcore command: Place all file-backed mappings in NT_FILE note
11) xfail gdb.base/coredump-filter.exp test which now works without a
    binary
12) Add new command "maint print core-file-backed-mappings"

    #10 thru #12 are new patches which need review.

13) Add documentation for "maint print core-file-backed-mappings"

    This one needs a review from Eli.

14) New core file tests with mappings over existing program memory

    I've made all the changes that Pedro recommended, plus added
    a test for the new maint print command.  It needs review.

 bfd/binary.c                               |  12 +-
 bfd/elf.c                                  |   8 -
 gdb/NEWS                                   |   4 +
 gdb/arch-utils.c                           |  16 ++
 gdb/arch-utils.h                           |  12 +
 gdb/bfd-target.c                           |   3 +-
 gdb/corelow.c                              | 260 ++++++++++++++++++++-
 gdb/doc/gdb.texinfo                        |   8 +
 gdb/exec.c                                 |   8 +-
 gdb/exec.h                                 |  13 +-
 gdb/gdbarch.c                              |  23 ++
 gdb/gdbarch.h                              |   6 +
 gdb/gdbarch.sh                             |   3 +
 gdb/linux-tdep.c                           | 244 +++++++++++++------
 gdb/target.c                               |  18 +-
 gdb/testsuite/gdb.base/coredump-filter.exp |  21 ++
 gdb/testsuite/gdb.base/corefile.exp        |  24 +-
 gdb/testsuite/gdb.base/corefile2.exp       | 179 ++++++++++++++
 gdb/testsuite/gdb.base/coremaker.c         |  30 ++-
 gdb/testsuite/gdb.base/coremaker2.c        | 150 ++++++++++++
 20 files changed, 933 insertions(+), 109 deletions(-)
 create mode 100644 gdb/testsuite/gdb.base/corefile2.exp
 create mode 100644 gdb/testsuite/gdb.base/coremaker2.c

-- 
2.26.2

[PATCH v4 01/14] Remove hack for GDB which sets the section size to 0

[Kevin Buettner]

This commit removes a hack for GDB which was introduced in 2007.
See:

    https://sourceware.org/ml/binutils/2007-08/msg00044.html

That hack mostly allowed GDB's handling of core files to continue to
work without any changes to GDB.

The problem with setting the section size to zero is that GDB won't
know how big that section is/was.  Often, this doesn't matter because
the data in question are found in the exec file.  But it can happen
that the section describes memory that had been allocated, but never
written to.  In this instance, the contents of that memory region are
not written to the core file.  Also, since the region in question was
dynamically allocated, it won't appear in the exec file.  We don't
want these regions to appear as inaccessible to GDB (since they *were*
accessible when the process was live), so it's important that GDB know
the size of the region.

I've made changes to GDB which correctly handles this case.  When
attempting to access memory, GDB will first consider core file data
for which both SEC_ALLOC and SEC_HAS_CONTENTS is set.  Next, if that
fails, GDB will attempt to find the data in the exec file.  Finally,
if that also fails, GDB will attempt to access memory in the sections
which are flagged as SEC_ALLOC, but not SEC_HAS_CONTENTS.

bfd/ChangeLog:

	* elf.c (_bfd_elf_make_section_from_phdr): Remove hack for GDB.
---
 bfd/elf.c | 8 --------
 1 file changed, 8 deletions(-)

diff --git a/bfd/elf.c b/bfd/elf.c
index 9ca42e10d8..991a71ca32 100644
--- a/bfd/elf.c
+++ b/bfd/elf.c
@@ -3026,14 +3026,6 @@ _bfd_elf_make_section_from_phdr (bfd *abfd,
       newsect->alignment_power = bfd_log2 (align);
       if (hdr->p_type == PT_LOAD)
 	{
-	  /* Hack for gdb.  Segments that have not been modified do
-	     not have their contents written to a core file, on the
-	     assumption that a debugger can find the contents in the
-	     executable.  We flag this case by setting the fake
-	     section size to zero.  Note that "real" bss sections will
-	     always have their contents dumped to the core file.  */
-	  if (bfd_get_format (abfd) == bfd_core)
-	    newsect->size = 0;
 	  newsect->flags |= SEC_ALLOC;
 	  if (hdr->p_flags & PF_X)
 	    newsect->flags |= SEC_CODE;
-- 
2.26.2

[PATCH v4 02/14] Adjust corefile.exp test to show regression after bfd hack removal

[Kevin Buettner]

In his review of my BZ 25631 patch series, Pedro was unable to
reproduce the regression which should occur after patch #1, "Remove
hack for GDB which sets the section size to 0", is applied.

Pedro was using an ld version older than 2.30.  Version 2.30
introduced the linker option -z separate-code.  Here's what the man
page has to say about it:

    Create separate code "PT_LOAD" segment header in the object.  This
    specifies a memory segment that should contain only instructions
    and must be in wholly disjoint pages from any other data.

In ld version 2.31, use of separate-code became the default for
Linux/x86.  So, really, 2.31 or later is required in order to see the
regression that occurs in recent Linux distributions when only the
bfd hack removal patch is applied.

For the test case in question, use of the separate-code linker option
means that the global variable "coremaker_ro" ends up in a separate
load segment (though potentially with other read-only data).  The
upshot of this is that when only patch #1 is applied, GDB won't be
able to correctly access coremaker_ro.  The reason for this is due
to the fact that this section will now have a non-zero size, but
will not have contents from the core file to find this data.
So GDB will ask BFD for the contents and BFD will respond with
zeroes for anything from those sections.  GDB should instead be
looking in the executable for this data.  Failing that, it can
then ask BFD for a reasonable value.  This is what a later patch
in this series does.

When using ld versions earlier than 2.31 (or 2.30 w/ the
-z separate-code option explicitly provided to the linker), there is
the possibility that coremaker_ro ends up being placed near other data
which is recorded in the core file.  That means that the correct value
will end up in the core file, simply because it resides on a page that
the kernel chooses to put in the core file.  This is why Pedro wasn't
able to reproduce the regression that should occur after fixing the
BFD hack.

This patch places a big chunk of memory, two pages worth on x86, in
front of "coremaker_ro" to attempt to force it onto another page
without requiring use of that new-fangled linker switch.

Speaking of which, I considered changing the test to use
-z separate-code, but this won't work because it didn't
exist prior to version 2.30.  The linker would probably complain
of an unrecognized switch.  Also, it likely won't be available in
other linkers not based on current binutils.  I.e. it probably won't
work in FreeBSD, NetBSD, etc.

To make this more concrete, this is what *should* happen when
attempting to access coremaker_ro when only patch #1 is applied:

    Core was generated by `/mesquite2/sourceware-git/f28-coresegs/bld/gdb/testsuite/outputs/gdb.base/coref'.
    Program terminated with signal SIGABRT, Aborted.
    #0  0x00007f68205deefb in raise () from /lib64/libc.so.6
    (gdb) p coremaker_ro
    $1 = 0

Note that this result is wrong; 201 should have been printed instead.
But that's the point of the rest of the patch series.

However, without this commit, or when using an old Linux distro with
a pre-2.31 ld, this is what you might see instead:

    Core was generated by `/mesquite2/sourceware-git/f28-coresegs/bld/gdb/testsuite/outputs/gdb.base/coref'.
    Program terminated with signal SIGABRT, Aborted.
    #0  0x00007f63dd658efb in raise () from /lib64/libc.so.6
    (gdb) p coremaker_ro
    $1 = 201

I.e. it prints the right answer, which sort of makes it seem like the
rest of the series isn't required.

Now, back to the patch itself... what should be the size of the memory
chunk placed before coremaker_ro?

It needs to be at least as big as the page size (PAGE_SIZE) from
the kernel.  For x86 and several other architectures this value is
4096.  I used MAPSIZE which is defined to be 8192 in coremaker.c.
So it's twice as big as what's currently needed for most Linux
architectures.  The constant PAGE_SIZE is available from <sys/user.h>,
but this isn't portable either.  In the end, it seemed simpler to
just pick a value and hope that it's big enough.  (Running a separate
program which finds the page size via sysconf(_SC_PAGESIZE) and then
passes it to the compilation via a -D switch seemed like overkill
for a case which is rendered moot by recent linker versions.)

Further information can be found here:

   https://sourceware.org/pipermail/gdb-patches/2020-May/168168.html
   https://sourceware.org/pipermail/gdb-patches/2020-May/168170.html

Thanks to H.J. Lu for telling me about the '-z separate-code' linker
switch.

gdb/testsuite/ChangeLog:

	* gdb.base/coremaker.c (filler_ro): New global constant.
---
 gdb/testsuite/gdb.base/coremaker.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/gdb/testsuite/gdb.base/coremaker.c b/gdb/testsuite/gdb.base/coremaker.c
index 3cc97e1e8e..a39b3ba8a4 100644
--- a/gdb/testsuite/gdb.base/coremaker.c
+++ b/gdb/testsuite/gdb.base/coremaker.c
@@ -42,6 +42,12 @@ char *buf2;
 int coremaker_data = 1;	/* In Data section */
 int coremaker_bss;	/* In BSS section */
 
+/* Place a chunk of memory before coremaker_ro to improve the chances
+   that coremaker_ro will end up on it's own page.  See:
+
+   https://sourceware.org/pipermail/gdb-patches/2020-May/168168.html
+   https://sourceware.org/pipermail/gdb-patches/2020-May/168170.html  */
+const unsigned char filler_ro[MAPSIZE] = {1, 2, 3, 4, 5, 6, 7, 8};
 const int coremaker_ro = 201;	/* In Read-Only Data section */
 
 /* Note that if the mapping fails for any reason, we set buf2
-- 
2.26.2

[PATCH v4 03/14] section_table_xfer_memory: Replace section name with callback predicate

[Kevin Buettner]

This patch is motivated by the need to be able to select sections
that section_table_xfer_memory_partial should consider for memory
transfers.  I'll use this facility in the next patch in this series.

section_table_xfer_memory_partial() can currently be passed a section
name which may be used to make name-based selections.  This is similar
to what I want to do, except that I want to be able to consider
section flags instead of the name.

I'm replacing the section name parameter with a predicate that,
when passed a pointer to a target_section struct, will return
true if that section should be further considered, or false which
indicates that it shouldn't.

I've converted the one existing use where a non-NULL section
name is passed to section_table_xfer_memory_partial().   Instead
of passing the section name, it now looks like this:

	  auto match_cb = [=] (const struct target_section *s)
	    {
	      return (strcmp (section_name, s->the_bfd_section->name) == 0);
	    };

	  return section_table_xfer_memory_partial (readbuf, writebuf,
						    memaddr, len, xfered_len,
						    table->sections,
						    table->sections_end,
						    match_cb);

The other callers all passed NULL; they've been simplified somewhat
in that they no longer need to pass NULL.

gdb/ChangeLog:

	* exec.h (section_table_xfer_memory): Revise declaration,
	replacing section name parameter with an optional callback
	predicate.
	* exec.c (section_table_xfer_memory): Likewise.
	* bfd-target.c, exec.c, target.c, corelow.c: Adjust all callers
	of section_table_xfer_memory.
---
 gdb/bfd-target.c |  3 +--
 gdb/corelow.c    |  3 +--
 gdb/exec.c       |  8 ++++----
 gdb/exec.h       | 13 ++++++++++---
 gdb/target.c     | 11 ++++++++---
 5 files changed, 24 insertions(+), 14 deletions(-)

diff --git a/gdb/bfd-target.c b/gdb/bfd-target.c
index b75abd7fb0..3d266951c5 100644
--- a/gdb/bfd-target.c
+++ b/gdb/bfd-target.c
@@ -77,8 +77,7 @@ target_bfd::xfer_partial (target_object object,
 	return section_table_xfer_memory_partial (readbuf, writebuf,
 						  offset, len, xfered_len,
 						  m_table.sections,
-						  m_table.sections_end,
-						  NULL);
+						  m_table.sections_end);
       }
     default:
       return TARGET_XFER_E_IO;
diff --git a/gdb/corelow.c b/gdb/corelow.c
index b6a12c0818..f4a5fdee12 100644
--- a/gdb/corelow.c
+++ b/gdb/corelow.c
@@ -615,8 +615,7 @@ core_target::xfer_partial (enum target_object object, const char *annex,
 	      (readbuf, writebuf,
 	       offset, len, xfered_len,
 	       m_core_section_table.sections,
-	       m_core_section_table.sections_end,
-	       NULL));
+	       m_core_section_table.sections_end));
 
     case TARGET_OBJECT_AUXV:
       if (readbuf)
diff --git a/gdb/exec.c b/gdb/exec.c
index de473fbcb2..58a058647f 100644
--- a/gdb/exec.c
+++ b/gdb/exec.c
@@ -956,7 +956,8 @@ section_table_xfer_memory_partial (gdb_byte *readbuf, const gdb_byte *writebuf,
 				   ULONGEST *xfered_len,
 				   struct target_section *sections,
 				   struct target_section *sections_end,
-				   const char *section_name)
+				   gdb::function_view<bool
+				     (const struct target_section *)> match_cb)
 {
   int res;
   struct target_section *p;
@@ -970,7 +971,7 @@ section_table_xfer_memory_partial (gdb_byte *readbuf, const gdb_byte *writebuf,
       struct bfd_section *asect = p->the_bfd_section;
       bfd *abfd = asect->owner;
 
-      if (section_name && strcmp (section_name, asect->name) != 0)
+      if (match_cb != nullptr && !match_cb (p))
 	continue;		/* not the section we need.  */
       if (memaddr >= p->addr)
         {
@@ -1043,8 +1044,7 @@ exec_target::xfer_partial (enum target_object object,
     return section_table_xfer_memory_partial (readbuf, writebuf,
 					      offset, len, xfered_len,
 					      table->sections,
-					      table->sections_end,
-					      NULL);
+					      table->sections_end);
   else
     return TARGET_XFER_E_IO;
 }
diff --git a/gdb/exec.h b/gdb/exec.h
index 54e6ff4d9b..82eb39c55d 100644
--- a/gdb/exec.h
+++ b/gdb/exec.h
@@ -65,8 +65,13 @@ extern enum target_xfer_status
    Request to transfer up to LEN 8-bit bytes of the target sections
    defined by SECTIONS and SECTIONS_END.  The OFFSET specifies the
    starting address.
-   If SECTION_NAME is not NULL, only access sections with that same
-   name.
+
+   The MATCH_CB predicate is optional; when provided it will be called
+   for each section under consideration.  When MATCH_CB evaluates as
+   true, the section remains under consideration; a false result
+   removes it from consideration for performing the memory transfers
+   noted above.  See memory_xfer_partial_1() in target.c for an
+   example.
 
    Return the number of bytes actually transfered, or zero when no
    data is available for the requested range.
@@ -83,7 +88,9 @@ extern enum target_xfer_status
 				     ULONGEST, ULONGEST, ULONGEST *,
 				     struct target_section *,
 				     struct target_section *,
-				     const char *);
+				     gdb::function_view<bool
+				       (const struct target_section *)> match_cb
+				         = nullptr);
 
 /* Read from mappable read-only sections of BFD executable files.
    Similar to exec_read_partial_read_only, but return
diff --git a/gdb/target.c b/gdb/target.c
index f4e4f05b5f..001a9f4c80 100644
--- a/gdb/target.c
+++ b/gdb/target.c
@@ -980,11 +980,17 @@ memory_xfer_partial_1 (struct target_ops *ops, enum target_object object,
 	  const char *section_name = section->the_bfd_section->name;
 
 	  memaddr = overlay_mapped_address (memaddr, section);
+
+	  auto match_cb = [=] (const struct target_section *s)
+	    {
+	      return (strcmp (section_name, s->the_bfd_section->name) == 0);
+	    };
+
 	  return section_table_xfer_memory_partial (readbuf, writebuf,
 						    memaddr, len, xfered_len,
 						    table->sections,
 						    table->sections_end,
-						    section_name);
+						    match_cb);
 	}
     }
 
@@ -1002,8 +1008,7 @@ memory_xfer_partial_1 (struct target_ops *ops, enum target_object object,
 	  return section_table_xfer_memory_partial (readbuf, writebuf,
 						    memaddr, len, xfered_len,
 						    table->sections,
-						    table->sections_end,
-						    NULL);
+						    table->sections_end);
 	}
     }
 
-- 
2.26.2

[PATCH v4 04/14] Provide access to non SEC_HAS_CONTENTS core file sections

[Kevin Buettner]

Consider the following program:

- - - mkmmapcore.c - - -

static char *buf;

int
main (int argc, char **argv)
{
  buf = mmap (NULL, 8192, PROT_READ | PROT_WRITE,
              MAP_ANONYMOUS | MAP_PRIVATE, -1, 0);
  abort ();
}
- - - end mkmmapcore.c - - -

Compile it like this:

gcc -g -o mkmmapcore mkmmapcore.c

Now let's run it from GDB.  I've already placed a breakpoint on the
line with the abort() call and have run to that breakpoint.

Breakpoint 1, main (argc=1, argv=0x7fffffffd678) at mkmmapcore.c:11
11	  abort ();
(gdb) x/x buf
0x7ffff7fcb000:	0x00000000

Note that we can examine the memory allocated via the call to mmap().

Now let's try debugging a core file created by running this program.
Depending on your system, in order to make a core file, you may have to
run the following as root (or using sudo):

    echo core > /proc/sys/kernel/core_pattern

It may also be necessary to do:

    ulimit -c unlimited

I'm using Fedora 31. YMMV if you're using one of the BSDs or some other
(non-Linux) system.

This is what things look like when we debug the core file:

    [kev@f31-1 tmp]$ gdb -q ./mkmmapcore core.304767
    Reading symbols from ./mkmmapcore...
    [New LWP 304767]
    Core was generated by `/tmp/mkmmapcore'.
    Program terminated with signal SIGABRT, Aborted.
    #0  __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:50
    50	  return ret;
    (gdb) x/x buf
    0x7ffff7fcb000:	Cannot access memory at address 0x7ffff7fcb000

Note that we can no longer access the memory region allocated by mmap().

Back in 2007, a hack for GDB was added to _bfd_elf_make_section_from_phdr()
in bfd/elf.c:

	  /* Hack for gdb.  Segments that have not been modified do
	     not have their contents written to a core file, on the
	     assumption that a debugger can find the contents in the
	     executable.  We flag this case by setting the fake
	     section size to zero.  Note that "real" bss sections will
	     always have their contents dumped to the core file.  */
	  if (bfd_get_format (abfd) == bfd_core)
	    newsect->size = 0;

You can find the entire patch plus links to other discussion starting
here:

    https://sourceware.org/ml/binutils/2007-08/msg00047.html

This hack sets the size of certain BFD sections to 0, which
effectively causes GDB to ignore them.  I think it's likely that the
bug described above existed even before this hack was added, but I
have no easy way to test this now.

The output from objdump -h shows the result of this hack:

 25 load13        00000000  00007ffff7fcb000  0000000000000000  00013000  2**12
                  ALLOC

(The first field, after load13, shows the size of 0.)

Once the hack is removed, the output from objdump -h shows the correct
size:

 25 load13        00002000  00007ffff7fcb000  0000000000000000  00013000  2**12
                  ALLOC

(This is a digression, but I think it's good that objdump will now show
the correct size.)

If we remove the hack from bfd/elf.c, but do nothing to GDB, we'll
see the following regression:

FAIL: gdb.base/corefile.exp: print coremaker_ro

The reason for this is that all sections which have the BFD flag
SEC_ALLOC set, but for which SEC_HAS_CONTENTS is not set no longer
have zero size.  Some of these sections have data that can (and should)
be read from the executable.  (Sections for which SEC_HAS_CONTENTS
is set should be read from the core file; sections which do not have
this flag set need to either be read from the executable or, failing
that, from the core file using whatever BFD decides is the best value
to present to the user - it uses zeros.)

At present, due to the way that the target strata are traversed when
attempting to access memory, the non-SEC_HAS_CONTENTS sections will be
read as zeroes from the process_stratum (which in this case is the
core file stratum) without first checking the file stratum, which is
where the data might actually be found.

What we should be doing is this:

- Attempt to access core file data for SEC_HAS_CONTENTS sections.
- Attempt to access executable file data if the above fails.
- Attempt to access core file data for non SEC_HAS_CONTENTS sections, if
  both of the above fail.

This corresponds to the analysis of Daniel Jacobowitz back in 2007
when the hack was added to BFD:

    https://sourceware.org/legacy-ml/binutils/2007-08/msg00045.html

The difference, observed by Pedro in his review of my v1 patches, is
that I'm using "the section flags as proxy for the p_filesz/p_memsz
checks."

gdb/ChangeLog:

	PR corefiles/25631
	* corelow.c (core_target:xfer_partial):  Revise
	TARGET_OBJECT_MEMORY case to consider non-SEC_HAS_CONTENTS
	case after first checking the stratum beneath the core
	target.
	(has_all_memory): Return true.
	* target.c (raw_memory_xfer_partial): Revise comment
	regarding use of has_all_memory.
---
 gdb/corelow.c | 47 +++++++++++++++++++++++++++++++++++++++++------
 gdb/target.c  |  7 +++++--
 2 files changed, 46 insertions(+), 8 deletions(-)

diff --git a/gdb/corelow.c b/gdb/corelow.c
index f4a5fdee12..8304d60129 100644
--- a/gdb/corelow.c
+++ b/gdb/corelow.c
@@ -87,7 +87,7 @@ class core_target final : public process_stratum_target
 
   const char *thread_name (struct thread_info *) override;
 
-  bool has_all_memory () override { return false; }
+  bool has_all_memory () override { return true; }
   bool has_memory () override;
   bool has_stack () override;
   bool has_registers () override;
@@ -611,12 +611,47 @@ core_target::xfer_partial (enum target_object object, const char *annex,
   switch (object)
     {
     case TARGET_OBJECT_MEMORY:
-      return (section_table_xfer_memory_partial
-	      (readbuf, writebuf,
-	       offset, len, xfered_len,
-	       m_core_section_table.sections,
-	       m_core_section_table.sections_end));
+      {
+	enum target_xfer_status xfer_status;
+
+	/* Try accessing memory contents from core file data,
+	   restricting consideration to those sections for which
+	   the BFD section flag SEC_HAS_CONTENTS is set.  */
+	auto has_contents_cb = [] (const struct target_section *s)
+	  {
+	    return ((s->the_bfd_section->flags & SEC_HAS_CONTENTS) != 0);
+	  };
+	xfer_status = section_table_xfer_memory_partial
+			(readbuf, writebuf,
+			 offset, len, xfered_len,
+			 m_core_section_table.sections,
+			 m_core_section_table.sections_end,
+			 has_contents_cb);
+	if (xfer_status == TARGET_XFER_OK)
+	  return TARGET_XFER_OK;
+
+	/* Now check the stratum beneath us; this should be file_stratum.  */
+	xfer_status = this->beneath ()->xfer_partial (object, annex, readbuf,
+						      writebuf, offset, len,
+						      xfered_len);
+	if (xfer_status == TARGET_XFER_OK)
+	  return TARGET_XFER_OK;
 
+	/* Finally, attempt to access data in core file sections with
+	   no contents.  These will typically read as all zero.  */
+	auto no_contents_cb = [&] (const struct target_section *s)
+	  {
+	    return !has_contents_cb (s);
+	  };
+	xfer_status = section_table_xfer_memory_partial
+			(readbuf, writebuf,
+			 offset, len, xfered_len,
+			 m_core_section_table.sections,
+			 m_core_section_table.sections_end,
+			 no_contents_cb);
+
+	return xfer_status;
+      }
     case TARGET_OBJECT_AUXV:
       if (readbuf)
 	{
diff --git a/gdb/target.c b/gdb/target.c
index 001a9f4c80..9c56c74d7b 100644
--- a/gdb/target.c
+++ b/gdb/target.c
@@ -925,8 +925,11 @@ raw_memory_xfer_partial (struct target_ops *ops, gdb_byte *readbuf,
       if (res == TARGET_XFER_UNAVAILABLE)
 	break;
 
-      /* We want to continue past core files to executables, but not
-	 past a running target's memory.  */
+      /* Don't continue past targets which have all the memory.
+         At one time, this code was necessary to read data from
+	 executables / shared libraries when data for the requested
+	 addresses weren't available in the core file.  But now the
+	 core target handles this case itself.  */
       if (ops->has_all_memory ())
 	break;
 
-- 
2.26.2

[PATCH v4 05/14] Test ability to access unwritten-to mmap data in core file

[Kevin Buettner]

gdb/testsuite/ChangeLog:

	PR corefiles/25631
	* gdb.base/corefile.exp (accessing anonymous, unwritten-to mmap data):
	New test.
	* gdb.base/coremaker.c (buf3): New global.
	(mmapdata): Add mmap call which uses MAP_ANONYMOUS and MAP_PRIVATE
	flags.
---
 gdb/testsuite/gdb.base/corefile.exp |  6 ++++++
 gdb/testsuite/gdb.base/coremaker.c  | 10 ++++++++++
 2 files changed, 16 insertions(+)

diff --git a/gdb/testsuite/gdb.base/corefile.exp b/gdb/testsuite/gdb.base/corefile.exp
index 34b903b350..d46b38704c 100644
--- a/gdb/testsuite/gdb.base/corefile.exp
+++ b/gdb/testsuite/gdb.base/corefile.exp
@@ -175,6 +175,12 @@ gdb_test_multiple "x/8bd buf2" "$test" {
     }
 }
 
+# Test ability to read anonymous and, more importantly, unwritten-to
+# mmap'd data.
+
+gdb_test "x/wx buf3" "$hex:\[ \t\]+0x00000000" \
+	 "accessing anonymous, unwritten-to mmap data"
+
 # test reinit_frame_cache
 
 gdb_load ${binfile}
diff --git a/gdb/testsuite/gdb.base/coremaker.c b/gdb/testsuite/gdb.base/coremaker.c
index a39b3ba8a4..0981b21738 100644
--- a/gdb/testsuite/gdb.base/coremaker.c
+++ b/gdb/testsuite/gdb.base/coremaker.c
@@ -38,6 +38,7 @@
 
 char *buf1;
 char *buf2;
+char *buf3;
 
 int coremaker_data = 1;	/* In Data section */
 int coremaker_bss;	/* In BSS section */
@@ -104,6 +105,15 @@ mmapdata ()
     }
   /* Touch buf2 so kernel writes it out into 'core'. */
   buf2[0] = buf1[0];
+
+  /* Create yet another region which is allocated, but not written to.  */
+  buf3 = mmap (NULL, MAPSIZE, PROT_READ | PROT_WRITE,
+               MAP_ANONYMOUS | MAP_PRIVATE, -1, 0);
+  if (buf3 == (char *) -1)
+    {
+      perror ("mmap failed");
+      return;
+    }
 }
 
 void
-- 
2.26.2

[PATCH v4 06/14] Update binary_get_section_contents to seek using section's file position

[Kevin Buettner]

I have a patch for GDB which opens and reads from BFDs using the
"binary" target.  However, for it to work, we need to be able to get a
section's contents based from the file position of that section.

At the moment, reading a section's contents will always read from the
start of the file regardless of where that section is located.  While
this was fine for the original use of the "binary" target, it won't
work for my use case.  This change shouldn't impact any existing
callers due to the fact that the single .data section is initialized
with a filepos of 0.

bfd/ChangeLog:

	* binary.c (binary_get_section_contents): Seek using offset
	from section's file position.
---
 bfd/binary.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/bfd/binary.c b/bfd/binary.c
index 999de0d8c4..e872924a2d 100644
--- a/bfd/binary.c
+++ b/bfd/binary.c
@@ -19,10 +19,10 @@
    Foundation, Inc., 51 Franklin Street - Fifth Floor, Boston,
    MA 02110-1301, USA.  */
 
-/* This is a BFD backend which may be used to write binary objects.
-   It may only be used for output, not input.  The intention is that
-   this may be used as an output format for objcopy in order to
-   generate raw binary data.
+/* This is a BFD backend which may be used to read or write binary
+   objects.  Historically, it was used as an output format for objcopy
+   in order to generate raw binary data, but is now used for other
+   purposes as well.
 
    This is very simple.  The only complication is that the real data
    will start at some address X, and in some cases we will not want to
@@ -97,12 +97,12 @@ binary_object_p (bfd *abfd)
 
 static bfd_boolean
 binary_get_section_contents (bfd *abfd,
-			     asection *section ATTRIBUTE_UNUSED,
+			     asection *section,
 			     void * location,
 			     file_ptr offset,
 			     bfd_size_type count)
 {
-  if (bfd_seek (abfd, offset, SEEK_SET) != 0
+  if (bfd_seek (abfd, section->filepos + offset, SEEK_SET) != 0
       || bfd_bread (location, count, abfd) != count)
     return FALSE;
   return TRUE;
-- 
2.26.2

[PATCH v4 07/14] Add new gdbarch method, read_core_file_mappings

[Kevin Buettner]

The new gdbarch method, read_core_file_mappings, will be used for
reading file-backed mappings from a core file.  It'll be used
for two purposes: 1) to construct a table of file-backed mappings
in corelow.c, and 2) for display of core file mappings.

For Linux, I tried a different approach in which knowledge of the note
format was placed directly in corelow.c.  This seemed okay at first;
it was only one note format and the note format was fairly simple.
After looking at FreeBSD's note/mapping reading code, I concluded
that it's best to leave architecture specific details for decoding
the note in (architecture specific) tdep files.

With regard to display of core file mappings, I experimented with
placing the mappings display code in corelow.c.  It has access to the
file-backed mappings which were read in when the core file was loaded.
And, better, still common code could be used for all architectures.
But, again, the FreeBSD mapping code convinced me that this was not
the best approach since it has even more mapping info than Linux.
Display code which would work well for Linux will leave out mappings
as well as protection info for mappings.

So, for these reasons, I'm introducing a new gdbarch method for
reading core file mappings.

gdb/ChangeLog:

	* arch-utils.c (default_read_core_file_mappings): New function.
	* arch-utils.c (default_read_core_file_mappings): Declare.
	* gdbarch.sh (read_core_file_mappings): New gdbarch method.
	* gdbarch.h, gdbarch.c: Regenerate.
---
 gdb/arch-utils.c | 16 ++++++++++++++++
 gdb/arch-utils.h | 12 ++++++++++++
 gdb/gdbarch.c    | 23 +++++++++++++++++++++++
 gdb/gdbarch.h    |  6 ++++++
 gdb/gdbarch.sh   |  3 +++
 5 files changed, 60 insertions(+)

diff --git a/gdb/arch-utils.c b/gdb/arch-utils.c
index 04955ea847..18418773de 100644
--- a/gdb/arch-utils.c
+++ b/gdb/arch-utils.c
@@ -1036,6 +1036,22 @@ default_get_pc_address_flags (frame_info *frame, CORE_ADDR pc)
   return "";
 }
 
+/* See arch-utils.h.  */
+void
+default_read_core_file_mappings (struct gdbarch *gdbarch,
+                                 struct bfd *cbfd,
+				 gdb::function_view<void (ULONGEST count)>
+				   pre_loop_cb,
+				 gdb::function_view<void (int num,
+				                          ULONGEST start,
+							  ULONGEST end,
+							  ULONGEST file_ofs,
+							  const char *filename,
+							  const void *other)>
+				   loop_cb)
+{
+}
+
 void _initialize_gdbarch_utils ();
 void
 _initialize_gdbarch_utils ()
diff --git a/gdb/arch-utils.h b/gdb/arch-utils.h
index 43d64b1f4f..8cb0db04c8 100644
--- a/gdb/arch-utils.h
+++ b/gdb/arch-utils.h
@@ -280,4 +280,16 @@ extern ULONGEST default_type_align (struct gdbarch *gdbarch,
 extern std::string default_get_pc_address_flags (frame_info *frame,
 						 CORE_ADDR pc);
 
+/* Default implementation of gdbarch read_core_file_mappings method.  */
+extern void default_read_core_file_mappings (struct gdbarch *gdbarch,
+					     struct bfd *cbfd,
+					     gdb::function_view<void (ULONGEST count)>
+					       pre_loop_cb,
+					     gdb::function_view<void (int num,
+								      ULONGEST start,
+								      ULONGEST end,
+								      ULONGEST file_ofs,
+								      const char *filename,
+								      const void *other)>
+					       loop_cb);
 #endif /* ARCH_UTILS_H */
diff --git a/gdb/gdbarch.c b/gdb/gdbarch.c
index 6d1bb0d280..dac4e44922 100644
--- a/gdb/gdbarch.c
+++ b/gdb/gdbarch.c
@@ -348,6 +348,7 @@ struct gdbarch
   const disasm_options_and_args_t * valid_disassembler_options;
   gdbarch_type_align_ftype *type_align;
   gdbarch_get_pc_address_flags_ftype *get_pc_address_flags;
+  gdbarch_read_core_file_mappings_ftype *read_core_file_mappings;
 };
 
 /* Create a new ``struct gdbarch'' based on information provided by
@@ -464,6 +465,7 @@ gdbarch_alloc (const struct gdbarch_info *info,
   gdbarch->addressable_memory_unit_size = default_addressable_memory_unit_size;
   gdbarch->type_align = default_type_align;
   gdbarch->get_pc_address_flags = default_get_pc_address_flags;
+  gdbarch->read_core_file_mappings = default_read_core_file_mappings;
   /* gdbarch_alloc() */
 
   return gdbarch;
@@ -712,6 +714,7 @@ verify_gdbarch (struct gdbarch *gdbarch)
   /* Skip verify of valid_disassembler_options, invalid_p == 0 */
   /* Skip verify of type_align, invalid_p == 0 */
   /* Skip verify of get_pc_address_flags, invalid_p == 0 */
+  /* Skip verify of read_core_file_mappings, invalid_p == 0 */
   if (!log.empty ())
     internal_error (__FILE__, __LINE__,
                     _("verify_gdbarch: the following are invalid ...%s"),
@@ -1281,6 +1284,9 @@ gdbarch_dump (struct gdbarch *gdbarch, struct ui_file *file)
   fprintf_unfiltered (file,
                       "gdbarch_dump: ravenscar_ops = %s\n",
                       host_address_to_string (gdbarch->ravenscar_ops));
+  fprintf_unfiltered (file,
+                      "gdbarch_dump: read_core_file_mappings = <%s>\n",
+                      host_address_to_string (gdbarch->read_core_file_mappings));
   fprintf_unfiltered (file,
                       "gdbarch_dump: gdbarch_read_pc_p() = %d\n",
                       gdbarch_read_pc_p (gdbarch));
@@ -5137,6 +5143,23 @@ set_gdbarch_get_pc_address_flags (struct gdbarch *gdbarch,
   gdbarch->get_pc_address_flags = get_pc_address_flags;
 }
 
+void
+gdbarch_read_core_file_mappings (struct gdbarch *gdbarch, struct bfd *cbfd,gdb::function_view<void (ULONGEST count)> pre_loop_cb,gdb::function_view<void (int num, ULONGEST start, ULONGEST end, ULONGEST file_ofs, const char *filename, const void *other)> loop_cb)
+{
+  gdb_assert (gdbarch != NULL);
+  gdb_assert (gdbarch->read_core_file_mappings != NULL);
+  if (gdbarch_debug >= 2)
+    fprintf_unfiltered (gdb_stdlog, "gdbarch_read_core_file_mappings called\n");
+  gdbarch->read_core_file_mappings (gdbarch, cbfd, pre_loop_cb, loop_cb);
+}
+
+void
+set_gdbarch_read_core_file_mappings (struct gdbarch *gdbarch,
+                                     gdbarch_read_core_file_mappings_ftype read_core_file_mappings)
+{
+  gdbarch->read_core_file_mappings = read_core_file_mappings;
+}
+
 
 /* Keep a registry of per-architecture data-pointers required by GDB
    modules.  */
diff --git a/gdb/gdbarch.h b/gdb/gdbarch.h
index 4e51c295b3..f2dd17b757 100644
--- a/gdb/gdbarch.h
+++ b/gdb/gdbarch.h
@@ -1628,6 +1628,12 @@ typedef std::string (gdbarch_get_pc_address_flags_ftype) (frame_info *frame, COR
 extern std::string gdbarch_get_pc_address_flags (struct gdbarch *gdbarch, frame_info *frame, CORE_ADDR pc);
 extern void set_gdbarch_get_pc_address_flags (struct gdbarch *gdbarch, gdbarch_get_pc_address_flags_ftype *get_pc_address_flags);
 
+/* Read core file mappings */
+
+typedef void (gdbarch_read_core_file_mappings_ftype) (struct gdbarch *gdbarch, struct bfd *cbfd,gdb::function_view<void (ULONGEST count)> pre_loop_cb,gdb::function_view<void (int num, ULONGEST start, ULONGEST end, ULONGEST file_ofs, const char *filename, const void *other)> loop_cb);
+extern void gdbarch_read_core_file_mappings (struct gdbarch *gdbarch, struct bfd *cbfd,gdb::function_view<void (ULONGEST count)> pre_loop_cb,gdb::function_view<void (int num, ULONGEST start, ULONGEST end, ULONGEST file_ofs, const char *filename, const void *other)> loop_cb);
+extern void set_gdbarch_read_core_file_mappings (struct gdbarch *gdbarch, gdbarch_read_core_file_mappings_ftype *read_core_file_mappings);
+
 extern struct gdbarch_tdep *gdbarch_tdep (struct gdbarch *gdbarch);
 
 
diff --git a/gdb/gdbarch.sh b/gdb/gdbarch.sh
index ad27a4eca0..e9ec91a321 100755
--- a/gdb/gdbarch.sh
+++ b/gdb/gdbarch.sh
@@ -1177,6 +1177,9 @@ m;ULONGEST;type_align;struct type *type;type;;default_type_align;;0
 # Return a string containing any flags for the given PC in the given FRAME.
 f;std::string;get_pc_address_flags;frame_info *frame, CORE_ADDR pc;frame, pc;;default_get_pc_address_flags;;0
 
+# Read core file mappings
+m;void;read_core_file_mappings;struct bfd *cbfd,gdb::function_view<void (ULONGEST count)> pre_loop_cb,gdb::function_view<void (int num, ULONGEST start, ULONGEST end, ULONGEST file_ofs, const char *filename, const void *other)> loop_cb;cbfd, pre_loop_cb, loop_cb;;default_read_core_file_mappings;;0
+
 EOF
 }
 
-- 
2.26.2

[PATCH v4 08/14] Use NT_FILE note section for reading core target memory

[Kevin Buettner]

In his reviews of my v1 and v2 corefile related patches, Pedro
identified two cases which weren't handled by those patches.

In https://sourceware.org/pipermail/gdb-patches/2020-May/168826.html,
Pedro showed that debugging a core file in which mmap() is used to
create a new mapping over an existing file-backed mapping will
produce incorrect results.  I.e, for his example, GDB would
show:

(gdb) disassemble main
Dump of assembler code for function main:
   0x00000000004004e6 <+0>:	push   %rbp
   0x00000000004004e7 <+1>:	mov    %rsp,%rbp
=> 0x00000000004004ea <+4>:	callq  0x4003f0 <abort@plt>
End of assembler dump.

This sort of looks like it might be correct, but is not due to the
fact that mmap(...MAP_FIXED...) was used to create a mapping (of all
zeros) on top of the .text section.  So, the correct result should be:

(gdb) disassemble main
Dump of assembler code for function main:
   0x00000000004004e6 <+0>:	add    %al,(%rax)
   0x00000000004004e8 <+2>:	add    %al,(%rax)
=> 0x00000000004004ea <+4>:	add    %al,(%rax)
   0x00000000004004ec <+6>:	add    %al,(%rax)
   0x00000000004004ee <+8>:	add    %al,(%rax)
End of assembler dump.

The other case that Pedro found involved an attempted examination of a
particular section in the test case from gdb.base/corefile.exp.  On
Fedora 27 or 28, the following behavior may be observed:

(gdb) info proc mappings
Mapped address spaces:

          Start Addr           End Addr       Size     Offset objfile
...
      0x7ffff7839000     0x7ffff7a38000   0x1ff000   0x1b5000 /usr/lib64/libc-2.27.so
...
(gdb) x/4x 0x7ffff7839000
0x7ffff7839000:	Cannot access memory at address 0x7ffff7839000

FYI, this section appears to be unrelocated vtable data.  See
https://sourceware.org/pipermail/gdb-patches/2020-May/168331.html for
a detailed analysis.

The important thing here is that GDB should be able to access this
address since it should be backed by the shared library.  I.e. it
should do this:

(gdb) x/4x 0x7ffff7839000
0x7ffff7839000:	0x0007ddf0	0x00000000	0x0007dba0	0x00000000

Both of these cases are fixed with this commit.

In a nutshell, this commit opens a "binary" target BFD for each of the
files that are mentioned in an NT_FILE / .note.linuxcore.file note
section.  It then uses these mappings instead of the file stratum
mappings that GDB has used in the past.

If this note section doesn't exist or is mangled for some reason, then
GDB will use the file stratum as before.  Should this happen, then
we can expect both of the above problems to again be present.

See the code comments in the commit for other details.

gdb/ChangeLog:

	* corelow.c (solist.h, unordered_map): Include.
	(class core_target): Add field m_core_file_mappings and
	method build_file_mappings.
	(core_target::core_target): Call build_file_mappings.
	(core_target::~core_target): Free memory associated with
	m_core_file_mappings.
	(core_target::build_file_mappings): New method.
	(core_target::xfer_partial): Use m_core_file_mappings
	for memory transfers.
	* linux-tdep.c (linux_read_core_file_mappings): New
	function.
	(linux_core_info_proc_mappings): Rewrite to use
	linux_read_core_file_mappings.
	(linux_init_abi): Register linux_read_core_file_mappings.
---
 gdb/corelow.c    | 138 +++++++++++++++++++++++++++++++-
 gdb/linux-tdep.c | 203 +++++++++++++++++++++++++++++++----------------
 2 files changed, 270 insertions(+), 71 deletions(-)

diff --git a/gdb/corelow.c b/gdb/corelow.c
index 8304d60129..ac52a1019e 100644
--- a/gdb/corelow.c
+++ b/gdb/corelow.c
@@ -37,6 +37,7 @@
 #include "exec.h"
 #include "readline/tilde.h"
 #include "solib.h"
+#include "solist.h"
 #include "filenames.h"
 #include "progspace.h"
 #include "objfiles.h"
@@ -45,6 +46,7 @@
 #include "gdbsupport/filestuff.h"
 #include "build-id.h"
 #include "gdbsupport/pathstuff.h"
+#include <unordered_map>
 
 #ifndef O_LARGEFILE
 #define O_LARGEFILE 0
@@ -121,6 +123,13 @@ class core_target final : public process_stratum_target
      targets.  */
   target_section_table m_core_section_table {};
 
+  /* File-backed address space mappings: some core files include
+     information about memory mapped files.  */
+  target_section_table m_core_file_mappings {};
+
+  /* Build m_core_file_mappings.  Called from the constructor.  */
+  void build_file_mappings ();
+
   /* FIXME: kettenis/20031023: Eventually this field should
      disappear.  */
   struct gdbarch *m_core_gdbarch = NULL;
@@ -141,11 +150,121 @@ core_target::core_target ()
 			   &m_core_section_table.sections_end))
     error (_("\"%s\": Can't find sections: %s"),
 	   bfd_get_filename (core_bfd), bfd_errmsg (bfd_get_error ()));
+
+  build_file_mappings ();
 }
 
 core_target::~core_target ()
 {
   xfree (m_core_section_table.sections);
+  xfree (m_core_file_mappings.sections);
+}
+
+/* Construct the target_section_table for file-backed mappings if
+   they exist.
+
+   For each unique path in the note, we'll open a BFD with a bfd
+   target of "binary".  This is an unstructured bfd target upon which
+   we'll impose a structure from the mappings in the architecture-specific
+   mappings note.  A BFD section is allocated and initialized for each
+   file-backed mapping.
+
+   We take care to not share already open bfds with other parts of
+   GDB; in particular, we don't want to add new sections to existing
+   BFDs.  We do, however, ensure that the BFDs that we allocate here
+   will go away (be deallocated) when the core target is detached.  */
+
+void
+core_target::build_file_mappings ()
+{
+  std::unordered_map<std::string, struct bfd *> bfd_map;
+
+  /* See linux_read_core_file_mappings() in linux-tdep.c for an example
+     read_core_file_mappings method.  */
+  gdbarch_read_core_file_mappings (m_core_gdbarch, core_bfd,
+
+    /* After determining the number of mappings, read_core_file_mappings
+       will invoke this lambda which allocates target_section storage for
+       the mappings.  */
+    [&] (ULONGEST count)
+      {
+	m_core_file_mappings.sections = XNEWVEC (struct target_section, count);
+	m_core_file_mappings.sections_end = m_core_file_mappings.sections;
+      },
+
+    /* read_core_file_mappings will invoke this lambda for each mapping
+       that it finds.  */
+    [&] (int num, ULONGEST start, ULONGEST end, ULONGEST file_ofs,
+         const char *filename, const void *other)
+      {
+	/* Architecture-specific read_core_mapping methods are expected to
+	   weed out non-file-backed mappings.  */
+	gdb_assert (filename != nullptr);
+
+	struct bfd *bfd = bfd_map[filename];
+	if (bfd == nullptr) {
+
+	  /* Use exec_file_find() to do sysroot expansion.  It'll also strip
+	     the potential sysroot target: prefix.  If there is no sysroot, an
+	     equivalent (possibly more canonical) pathname will be provided.  */
+	  gdb::unique_xmalloc_ptr<char> expanded_fname
+	    = exec_file_find (filename, NULL);
+	  if (expanded_fname == nullptr)
+	    {
+	      warning (_("Can't open file %s during file-backed mapping "
+	                 "note processing"),
+		       expanded_fname.get ());
+	      return;
+	    }
+
+	  bfd = bfd_map[filename] = bfd_openr (expanded_fname.get (), "binary");
+
+	  if (bfd == nullptr || !bfd_check_format (bfd, bfd_object))
+	    {
+	      /* If we get here, there's a good chance that it's due to
+		 an internal error.  We issue a warning instead of an
+		 internal error because of the possibility that the
+		 file was removed in between checking for its
+		 existence during the expansion in exec_file_find()
+		 and the calls to bfd_openr() / bfd_check_format(). 
+		 Output both the path from the core file note along
+		 with its expansion to make debugging this problem
+		 easier.  */
+	      warning (_("Can't open file %s which was expanded to %s "
+			 "during file-backed mapping note processing"),
+		       filename, expanded_fname.get ());
+	      return;
+	    }
+	    /* Ensure that the bfd will be closed when core_bfd is closed. 
+	       This can be checked before/after a core file detach via
+	       "maint info bfds".  */
+	    gdb_bfd_record_inclusion (core_bfd, bfd);
+	}
+
+	/* Make new BFD section.  */
+	char secnamebuf[64];
+	sprintf (secnamebuf, "S%04d", num);
+	char *secname = (char *) bfd_alloc (bfd, strlen (secnamebuf) + 1);
+	if (secname == nullptr)
+	  error (_("Out of memory"));
+	strcpy (secname, secnamebuf);
+	asection *sec = bfd_make_section_anyway (bfd, secname);
+	if (sec == nullptr)
+	  error (_("Can't make section"));
+	sec->filepos = file_ofs;
+	bfd_set_section_flags (sec, SEC_READONLY | SEC_HAS_CONTENTS);
+	bfd_set_section_size (sec, end - start);
+	bfd_set_section_vma (sec, start);
+	bfd_set_section_lma (sec, start);
+	bfd_set_section_alignment (sec, 2);
+
+	/* Set target_section fields.  */
+	struct target_section *ts = m_core_file_mappings.sections_end++;
+	ts->addr = start;
+	ts->endaddr = end;
+	ts->owner = nullptr;
+	ts->the_bfd_section = sec;
+      });
 }
 
 static void add_to_thread_list (bfd *, asection *, void *);
@@ -630,10 +749,21 @@ core_target::xfer_partial (enum target_object object, const char *annex,
 	if (xfer_status == TARGET_XFER_OK)
 	  return TARGET_XFER_OK;
 
-	/* Now check the stratum beneath us; this should be file_stratum.  */
-	xfer_status = this->beneath ()->xfer_partial (object, annex, readbuf,
-						      writebuf, offset, len,
-						      xfered_len);
+	/* Check file backed mappings.  If they're available, use
+	   core file provided mappings (e.g. from .note.linuxcore.file
+	   or the like) as this should provide a more accurate
+	   result.  If not, check the stratum beneath us, which should
+	   be the file stratum.  */
+	if (m_core_file_mappings.sections != nullptr)
+	  xfer_status = section_table_xfer_memory_partial
+			  (readbuf, writebuf,
+			   offset, len, xfered_len,
+			   m_core_file_mappings.sections,
+			   m_core_file_mappings.sections_end);
+	else
+	  xfer_status = this->beneath ()->xfer_partial (object, annex, readbuf,
+							writebuf, offset, len,
+							xfered_len);
 	if (xfer_status == TARGET_XFER_OK)
 	  return TARGET_XFER_OK;
 
diff --git a/gdb/linux-tdep.c b/gdb/linux-tdep.c
index fd4337f100..fa59b09eec 100644
--- a/gdb/linux-tdep.c
+++ b/gdb/linux-tdep.c
@@ -1018,106 +1018,174 @@ linux_info_proc (struct gdbarch *gdbarch, const char *args,
     }
 }
 
-/* Implement "info proc mappings" for a corefile.  */
+/* Implementation of `gdbarch_read_core_file_mappings', as defined in
+   gdbarch.h.
+   
+   This function reads the NT_FILE note (which BFD turns into the
+   section ".note.linuxcore.file").  The format of this note / section
+   is described as follows in the Linux kernel sources in
+   fs/binfmt_elf.c:
+   
+      long count     -- how many files are mapped
+      long page_size -- units for file_ofs
+      array of [COUNT] elements of
+	long start
+	long end
+	long file_ofs
+      followed by COUNT filenames in ASCII: "FILE1" NUL "FILE2" NUL...
+      
+   CBFD is the BFD of the core file.
+
+   PRE_LOOP_CB is the callback function to invoke prior to starting
+   the loop which process individual entries.  This callback will
+   only be executed after the note has been examined in enough
+   detail to verify that it's not malformed in some way.
+   
+   LOOP_CB is the callback function that will be executed once
+   for each mapping.  */
 
 static void
-linux_core_info_proc_mappings (struct gdbarch *gdbarch, const char *args)
+linux_read_core_file_mappings (struct gdbarch *gdbarch,
+			       struct bfd *cbfd,
+			       gdb::function_view<void (ULONGEST count)>
+			         pre_loop_cb,
+			       gdb::function_view<void (int num,
+			                                ULONGEST start,
+							ULONGEST end,
+							ULONGEST file_ofs,
+							const char *filename,
+							const void *other)>
+				 loop_cb)
 {
-  asection *section;
-  ULONGEST count, page_size;
-  unsigned char *descdata, *filenames, *descend;
-  size_t note_size;
-  unsigned int addr_size_bits, addr_size;
-  struct gdbarch *core_gdbarch = gdbarch_from_bfd (core_bfd);
-  /* We assume this for reading 64-bit core files.  */
+  /* Ensure that ULONGEST is big enough for reading 64-bit core files.  */
   gdb_static_assert (sizeof (ULONGEST) >= 8);
 
-  section = bfd_get_section_by_name (core_bfd, ".note.linuxcore.file");
-  if (section == NULL)
-    {
-      warning (_("unable to find mappings in core file"));
-      return;
-    }
+  /* It's not required that the NT_FILE note exists, so return silently
+     if it's not found.  Beyond this point though, we'll complain
+     if problems are found.  */
+  asection *section = bfd_get_section_by_name (cbfd, ".note.linuxcore.file");
+  if (section == nullptr)
+    return;
 
-  addr_size_bits = gdbarch_addr_bit (core_gdbarch);
-  addr_size = addr_size_bits / 8;
-  note_size = bfd_section_size (section);
+  unsigned int addr_size_bits = gdbarch_addr_bit (gdbarch);
+  unsigned int addr_size = addr_size_bits / 8;
+  size_t note_size = bfd_section_size (section);
 
   if (note_size < 2 * addr_size)
-    error (_("malformed core note - too short for header"));
+    {
+      warning (_("malformed core note - too short for header"));
+      return;
+    }
 
-  gdb::def_vector<unsigned char> contents (note_size);
+  gdb::def_vector<gdb_byte> contents (note_size);
   if (!bfd_get_section_contents (core_bfd, section, contents.data (),
 				 0, note_size))
-    error (_("could not get core note contents"));
+    {
+      warning (_("could not get core note contents"));
+      return;
+    }
 
-  descdata = contents.data ();
-  descend = descdata + note_size;
+  gdb_byte *descdata = contents.data ();
+  char *descend = (char *) descdata + note_size;
 
   if (descdata[note_size - 1] != '\0')
-    error (_("malformed note - does not end with \\0"));
+    {
+      warning (_("malformed note - does not end with \\0"));
+      return;
+    }
 
-  count = bfd_get (addr_size_bits, core_bfd, descdata);
+  ULONGEST count = bfd_get (addr_size_bits, core_bfd, descdata);
   descdata += addr_size;
 
-  page_size = bfd_get (addr_size_bits, core_bfd, descdata);
+  ULONGEST page_size = bfd_get (addr_size_bits, core_bfd, descdata);
   descdata += addr_size;
 
   if (note_size < 2 * addr_size + count * 3 * addr_size)
-    error (_("malformed note - too short for supplied file count"));
-
-  printf_filtered (_("Mapped address spaces:\n\n"));
-  if (gdbarch_addr_bit (gdbarch) == 32)
-    {
-      printf_filtered ("\t%10s %10s %10s %10s %s\n",
-		       "Start Addr",
-		       "  End Addr",
-		       "      Size", "    Offset", "objfile");
-    }
-  else
     {
-      printf_filtered ("  %18s %18s %10s %10s %s\n",
-		       "Start Addr",
-		       "  End Addr",
-		       "      Size", "    Offset", "objfile");
+      warning (_("malformed note - too short for supplied file count"));
+      return;
     }
 
-  filenames = descdata + count * 3 * addr_size;
-  while (--count > 0)
+  char *filenames = (char *) descdata + count * 3 * addr_size;
+
+  /* Make sure that the correct number of filenames exist.  Complain
+     if there aren't enough or are too many.  */
+  char *f = filenames;
+  for (int i = 0; i < count; i++)
     {
-      ULONGEST start, end, file_ofs;
+      if (f >= descend)
+        {
+	  warning (_("malformed note - filename area is too small"));
+	  return;
+	}
+      f += strnlen (f, descend - f) + 1;
+    }
+  /* Complain, but don't return early if the filename area is too big.  */
+  if (f != descend)
+    warning (_("malformed note - filename area is too big"));
 
-      if (filenames == descend)
-	error (_("malformed note - filenames end too early"));
+  pre_loop_cb (count);
 
-      start = bfd_get (addr_size_bits, core_bfd, descdata);
+  for (int i = 0; i < count; i++)
+    {
+      ULONGEST start = bfd_get (addr_size_bits, core_bfd, descdata);
       descdata += addr_size;
-      end = bfd_get (addr_size_bits, core_bfd, descdata);
+      ULONGEST end = bfd_get (addr_size_bits, core_bfd, descdata);
       descdata += addr_size;
-      file_ofs = bfd_get (addr_size_bits, core_bfd, descdata);
+      ULONGEST file_ofs
+        = bfd_get (addr_size_bits, core_bfd, descdata) * page_size;
       descdata += addr_size;
+      char * filename = filenames;
+      filenames += strlen ((char *) filenames) + 1;
 
-      file_ofs *= page_size;
-
-      if (gdbarch_addr_bit (gdbarch) == 32)
-	printf_filtered ("\t%10s %10s %10s %10s %s\n",
-			 paddress (gdbarch, start),
-			 paddress (gdbarch, end),
-			 hex_string (end - start),
-			 hex_string (file_ofs),
-			 filenames);
-      else
-	printf_filtered ("  %18s %18s %10s %10s %s\n",
-			 paddress (gdbarch, start),
-			 paddress (gdbarch, end),
-			 hex_string (end - start),
-			 hex_string (file_ofs),
-			 filenames);
-
-      filenames += 1 + strlen ((char *) filenames);
+      loop_cb (i, start, end, file_ofs, filename, nullptr);
     }
 }
 
+/* Implement "info proc mappings" for a corefile.  */
+
+static void
+linux_core_info_proc_mappings (struct gdbarch *gdbarch, const char *args)
+{
+  linux_read_core_file_mappings (gdbarch, core_bfd,
+    [=] (ULONGEST count)
+      {
+	printf_filtered (_("Mapped address spaces:\n\n"));
+	if (gdbarch_addr_bit (gdbarch) == 32)
+	  {
+	    printf_filtered ("\t%10s %10s %10s %10s %s\n",
+			     "Start Addr",
+			     "  End Addr",
+			     "      Size", "    Offset", "objfile");
+	  }
+	else
+	  {
+	    printf_filtered ("  %18s %18s %10s %10s %s\n",
+			     "Start Addr",
+			     "  End Addr",
+			     "      Size", "    Offset", "objfile");
+	  }
+      },
+    [=] (int num, ULONGEST start, ULONGEST end, ULONGEST file_ofs,
+         const char *filename, const void *other)
+      {
+	if (gdbarch_addr_bit (gdbarch) == 32)
+	  printf_filtered ("\t%10s %10s %10s %10s %s\n",
+			   paddress (gdbarch, start),
+			   paddress (gdbarch, end),
+			   hex_string (end - start),
+			   hex_string (file_ofs),
+			   filename);
+	else
+	  printf_filtered ("  %18s %18s %10s %10s %s\n",
+			   paddress (gdbarch, start),
+			   paddress (gdbarch, end),
+			   hex_string (end - start),
+			   hex_string (file_ofs),
+			   filename);
+      });
+}
+
 /* Implement "info proc" for a corefile.  */
 
 static void
@@ -2472,6 +2540,7 @@ linux_init_abi (struct gdbarch_info info, struct gdbarch *gdbarch)
   set_gdbarch_info_proc (gdbarch, linux_info_proc);
   set_gdbarch_core_info_proc (gdbarch, linux_core_info_proc);
   set_gdbarch_core_xfer_siginfo (gdbarch, linux_core_xfer_siginfo);
+  set_gdbarch_read_core_file_mappings (gdbarch, linux_read_core_file_mappings);
   set_gdbarch_find_memory_regions (gdbarch, linux_find_memory_regions);
   set_gdbarch_make_corefile_notes (gdbarch, linux_make_corefile_notes);
   set_gdbarch_has_shared_address_space (gdbarch,
-- 
2.26.2

[PATCH v4 09/14] Add test for accessing read-only mmapped data in a core file

[Kevin Buettner]

This test passes when run using a GDB with my corefile patches.  When
run against a GDB without my patches, I see the following failures,
the first of which is due to the test added by this commit:

FAIL: gdb.base/corefile.exp: accessing read-only mmapped data in core file (mapping address not found in core file)
FAIL: gdb.base/corefile.exp: accessing anonymous, unwritten-to mmap data

gdb/testsuite/ChangeLog:

	* gdb.base/corefile.exp: Add test "accessing read-only mmapped
	data in core file".
	* gdb.base/coremaker.c (buf2ro): New global.
	(mmapdata): Add a read-only mmap mapping.
---
 gdb/testsuite/gdb.base/corefile.exp | 18 +++++++++++++++++-
 gdb/testsuite/gdb.base/coremaker.c  | 14 ++++++++++++--
 2 files changed, 29 insertions(+), 3 deletions(-)

diff --git a/gdb/testsuite/gdb.base/corefile.exp b/gdb/testsuite/gdb.base/corefile.exp
index d46b38704c..b0020a5fa9 100644
--- a/gdb/testsuite/gdb.base/corefile.exp
+++ b/gdb/testsuite/gdb.base/corefile.exp
@@ -34,7 +34,10 @@ if {[build_executable $testfile.exp $testfile $srcfile debug] == -1} {
     return -1
 }
 
-set corefile [core_find $binfile {coremmap.data}]
+# Do not delete coremap.data when calling core_find.  This file is
+# required for GDB to find mmap'd data in the "accessing read-only
+# mmapped data in core file" test.
+set corefile [core_find $binfile {}]
 if {$corefile == ""} {
     return 0
 }
@@ -175,6 +178,19 @@ gdb_test_multiple "x/8bd buf2" "$test" {
     }
 }
 
+set test "accessing read-only mmapped data in core file"
+gdb_test_multiple "x/8bd buf2ro" "$test" {
+    -re ".*:.*0.*1.*2.*3.*4.*5.*6.*7.*$gdb_prompt $" {
+	pass "$test"
+    }
+    -re "0x\[f\]*:.*Cannot access memory at address 0x\[f\]*.*$gdb_prompt $" {
+	fail "$test (mapping failed at runtime)"
+    }
+    -re "0x.*:.*Cannot access memory at address 0x.*$gdb_prompt $" {
+	fail "$test (mapping address not found in core file)"
+    }
+}
+
 # Test ability to read anonymous and, more importantly, unwritten-to
 # mmap'd data.
 
diff --git a/gdb/testsuite/gdb.base/coremaker.c b/gdb/testsuite/gdb.base/coremaker.c
index 0981b21738..3fc13e9287 100644
--- a/gdb/testsuite/gdb.base/coremaker.c
+++ b/gdb/testsuite/gdb.base/coremaker.c
@@ -38,6 +38,7 @@
 
 char *buf1;
 char *buf2;
+char *buf2ro;
 char *buf3;
 
 int coremaker_data = 1;	/* In Data section */
@@ -90,16 +91,25 @@ mmapdata ()
       return;
     }
 
+  /* Map in another copy, read-only.  We won't write to this copy so it
+     will likely not end up in the core file.  */
+  buf2ro = (char *) mmap (0, MAPSIZE, PROT_READ, MAP_PRIVATE, fd, 0);
+  if (buf2ro == (char *) -1)
+    {
+      perror ("mmap failed");
+      return;
+    }
+
   /* Verify that the original data and the mapped data are identical.
      If not, we'd rather fail now than when trying to access the mapped
      data from the core file. */
 
   for (j = 0; j < MAPSIZE; ++j)
     {
-      if (buf1[j] != buf2[j])
+      if (buf1[j] != buf2[j] || buf1[j] != buf2ro[j])
 	{
 	  fprintf (stderr, "mapped data is incorrect");
-	  buf2 = (char *) -1;
+	  buf2 = buf2ro = (char *) -1;
 	  return;
 	}
     }
-- 
2.26.2

[PATCH v4 10/14] gcore command: Place all file-backed mappings in NT_FILE note

[Kevin Buettner]

When making a core file with the GDB's gcore command on Linux,
the same criteria used for determining which mappings should be
dumped were also being used for determining which entries should
be placed in the NT_FILE note.  This is wrong; we want to place
all file-backed mappings in this note.

The predicate function, dump_mapping_p, was used to determine whether
or not to dump a mapping from within linux_find_memory_regions_full.
This commit leaves this predicate in place, but adds a new parameter,
should_dump_mapping_p, to linux_find_memory_regions_full.  It then
calls should_dump_mapping_p instead of dump_mapping_p.  dump_mapping_p
is passed to linux_find_memory_regions_full at one call site; at the
other call site, dump_note_entry_p is passed instead.

gdb/ChangeLog:

	* linux-tdep.c (dump_note_entry_p): New function.
	(linux_dump_mapping_p_ftype): New typedef.
	(linux_find_memory_regions_full): Add new parameter,
	should_dump_mapping_p.
	(linux_find_memory_regions): Adjust call to
	linux_find_memory_regions_full.
	(linux_make_mappings_core_file_notes): Use dump_note_entry_p in
	call to linux_find_memory_regions_full.
---
 gdb/linux-tdep.c | 41 +++++++++++++++++++++++++++++++++++++----
 1 file changed, 37 insertions(+), 4 deletions(-)

diff --git a/gdb/linux-tdep.c b/gdb/linux-tdep.c
index fa59b09eec..302e432968 100644
--- a/gdb/linux-tdep.c
+++ b/gdb/linux-tdep.c
@@ -726,6 +726,25 @@ dump_mapping_p (filter_flags filterflags, const struct smaps_vmflags *v,
   return dump_p;
 }
 
+/* As above, but return true only when we should dump the NT_FILE
+   entry.  */
+
+static int
+dump_note_entry_p (filter_flags filterflags, const struct smaps_vmflags *v,
+		int maybe_private_p, int mapping_anon_p, int mapping_file_p,
+		const char *filename, ULONGEST addr, ULONGEST offset)
+{
+  /* vDSO and vsyscall mappings will end up in the core file.  Don't
+     put them in the NT_FILE note.  */
+  if (strcmp ("[vdso]", filename) == 0
+      || strcmp ("[vsyscall]", filename) == 0)
+    return 0;
+
+  /* Otherwise, any other file-based mapping should be placed in the
+     note.  */
+  return filename != nullptr;
+}
+
 /* Implement the "info proc" command.  */
 
 static void
@@ -1240,10 +1259,20 @@ typedef int linux_find_memory_region_ftype (ULONGEST vaddr, ULONGEST size,
 					    const char *filename,
 					    void *data);
 
+typedef int linux_dump_mapping_p_ftype (filter_flags filterflags,
+					const struct smaps_vmflags *v,
+					int maybe_private_p,
+					int mapping_anon_p,
+					int mapping_file_p,
+					const char *filename,
+					ULONGEST addr,
+					ULONGEST offset);
+
 /* List memory regions in the inferior for a corefile.  */
 
 static int
 linux_find_memory_regions_full (struct gdbarch *gdbarch,
+				linux_dump_mapping_p_ftype *should_dump_mapping_p,
 				linux_find_memory_region_ftype *func,
 				void *obfd)
 {
@@ -1394,9 +1423,10 @@ 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, addr, offset);
+	    should_dump_p = should_dump_mapping_p (filterflags, &v, priv,
+					           mapping_anon_p,
+						   mapping_file_p,
+					           filename, addr, offset);
 	  else
 	    {
 	      /* Older Linux kernels did not support the "Anonymous:" counter.
@@ -1460,6 +1490,7 @@ linux_find_memory_regions (struct gdbarch *gdbarch,
   data.obfd = obfd;
 
   return linux_find_memory_regions_full (gdbarch,
+					 dump_mapping_p,
 					 linux_find_memory_regions_thunk,
 					 &data);
 }
@@ -1543,7 +1574,9 @@ linux_make_mappings_corefile_notes (struct gdbarch *gdbarch, bfd *obfd,
   pack_long (buf, long_type, 1);
   obstack_grow (&data_obstack, buf, TYPE_LENGTH (long_type));
 
-  linux_find_memory_regions_full (gdbarch, linux_make_mappings_callback,
+  linux_find_memory_regions_full (gdbarch, 
+				  dump_note_entry_p,
+				  linux_make_mappings_callback,
 				  &mapping_data);
 
   if (mapping_data.file_count != 0)
-- 
2.26.2

[PATCH v4 11/14] xfail gdb.base/coredump-filter.exp test which now works without a binary

[Kevin Buettner]

See comment in patch for a description of what this is about.  In
a nutshell, a certain memory access was expected to not work in
order to PASS, but due to recent changes, the memory access now
works causing the test to FAIL.

gdb/testsuite/ChangeLog:

	* gdb.base/coredump-filter.exp (test_disasm):  Call
	setup_xfail for test "disassemble function with corefile and
	without a binary".
---
 gdb/testsuite/gdb.base/coredump-filter.exp | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/gdb/testsuite/gdb.base/coredump-filter.exp b/gdb/testsuite/gdb.base/coredump-filter.exp
index ff398f2b85..fd4e0352d8 100644
--- a/gdb/testsuite/gdb.base/coredump-filter.exp
+++ b/gdb/testsuite/gdb.base/coredump-filter.exp
@@ -96,6 +96,27 @@ proc test_disasm { core address should_fail } {
 	}
 
 	if { $should_fail == 1 } {
+	    # As originally conceived, an attempt here to disassemble
+	    # addresses in main() with the core file loaded, but with
+	    # the executable not loaded shouldn't work .  This was a
+	    # good idea because it demonstrates that executable-backed
+	    # memory was not dumped to the core file.  However, this
+	    # test now fails (i.e. now successfully disassembles
+	    # addresses in main()) due to the fact that GDB loads the
+	    # file-based mappings in Linux's NT_FILE note.  So, even
+	    # though the executable wasn't explicitly loaded, GDB will
+	    # still be able to find the file-backed memory via the
+	    # note (referencing the file) within the core file.  The
+	    # data in question is not actually stored in the core
+	    # file, but is instead found in one of the files mentioned
+	    # in the core file note.
+	    #
+	    # It's tempting to simply remove this test because it's
+	    # apparently no longer useful.  However, the idea behind
+	    # the test is useful, so it's marked as xfail, along with
+	    # this comment, as a reminder for someone to come up with
+	    # a new approach.
+	    setup_xfail "*-*-*"
 	    gdb_test "x/i \$pc" "=> $hex:\tCannot access memory at address $hex" \
 		"disassemble function with corefile and without a binary"
 	} else {
-- 
2.26.2

[PATCH v4 12/14] Add new command "maint print core-file-backed-mappings"

[Kevin Buettner]

I wrote a read_core_file_mappings method for FreeBSD and then registered
this gdbarch method.  I saw some strange behavior while testing it and
wanted a way to make sure that mappings were being correctly loaded
into corelow.c, so I wrote the new command which is the topic of this
commit.  I think it might be occasionally useful for debugging strange
corefile behavior.

With regard to FreeBSD, my work isn't ready yet.  Unlike Linux,
FreeBSD puts all mappings into its core file note.  And, unlike Linux,
it doesn't dump load segments which occupy no space in the file.  So
my (perhaps naive) implementation of a FreeBSD read_core_file_mappings
didn't work all that well:  I saw more failures in the corefile2.exp
tests than without it.  I think it should be possible to make FreeBSD
work as well as Linux, but it will require doing something with all of
the mappings, not just the file based mappings that I was considering.

gdb/ChangeLog:

	* corelow.c (gdbcmd.h): Include.
	(core_target::info_proc_mappings): New method.
	(get_current_core_target): New function.
	(maintenance_print_core_file_backed_mappings): New function.
	(_initialize_corelow): Add core-file-backed-mappings to
	"maint print" commands.
---
 gdb/corelow.c | 82 +++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 82 insertions(+)

diff --git a/gdb/corelow.c b/gdb/corelow.c
index ac52a1019e..173418745b 100644
--- a/gdb/corelow.c
+++ b/gdb/corelow.c
@@ -47,6 +47,7 @@
 #include "build-id.h"
 #include "gdbsupport/pathstuff.h"
 #include <unordered_map>
+#include "gdbcmd.h"
 
 #ifndef O_LARGEFILE
 #define O_LARGEFILE 0
@@ -113,6 +114,9 @@ class core_target final : public process_stratum_target
 				  const char *human_name,
 				  bool required);
 
+  /* See definition.  */
+  void info_proc_mappings (struct gdbarch *gdbarch);
+
 private: /* per-core data */
 
   /* The core's section table.  Note that these target sections are
@@ -1027,9 +1031,87 @@ core_target::info_proc (const char *args, enum info_proc_what request)
   return true;
 }
 
+/* Get a pointer to the current core target.  If not connected to a
+   core target, return NULL.  */
+
+static core_target *
+get_current_core_target ()
+{
+  target_ops *proc_target = current_inferior ()->process_target ();
+  return dynamic_cast<core_target *> (proc_target);
+}
+
+/* Display file backed mappings from core file.  */
+
+void
+core_target::info_proc_mappings (struct gdbarch *gdbarch)
+{
+  if (m_core_file_mappings.sections != m_core_file_mappings.sections_end)
+    {
+      printf_filtered (_("Mapped address spaces:\n\n"));
+      if (gdbarch_addr_bit (gdbarch) == 32)
+	{
+	  printf_filtered ("\t%10s %10s %10s %10s %s\n",
+			   "Start Addr",
+			   "  End Addr",
+			   "      Size", "    Offset", "objfile");
+	}
+      else
+	{
+	  printf_filtered ("  %18s %18s %10s %10s %s\n",
+			   "Start Addr",
+			   "  End Addr",
+			   "      Size", "    Offset", "objfile");
+	}
+    }
+
+  for (const struct target_section *tsp = m_core_file_mappings.sections;
+       tsp < m_core_file_mappings.sections_end; tsp++ )
+    {
+      ULONGEST start = tsp->addr;
+      ULONGEST end = tsp->endaddr;
+      ULONGEST file_ofs = tsp->the_bfd_section->filepos;
+      const char *filename = bfd_get_filename (tsp->the_bfd_section->owner);
+
+      if (gdbarch_addr_bit (gdbarch) == 32)
+	printf_filtered ("\t%10s %10s %10s %10s %s\n",
+			 paddress (gdbarch, start),
+			 paddress (gdbarch, end),
+			 hex_string (end - start),
+			 hex_string (file_ofs),
+			 filename);
+      else
+	printf_filtered ("  %18s %18s %10s %10s %s\n",
+			 paddress (gdbarch, start),
+			 paddress (gdbarch, end),
+			 hex_string (end - start),
+			 hex_string (file_ofs),
+			 filename);
+    }
+}
+
+/* Implement "maintenance print core-file-backed-mappings" command.  
+
+   If mappings are loaded, the results should be similar to the
+   mappings shown by "info proc mappings".  This command is mainly
+   a debugging tool to make sure that the expected mappings are
+   present after loading a core file.  */
+
+static void
+maintenance_print_core_file_backed_mappings (const char *args, int from_tty)
+{
+  core_target *targ = get_current_core_target ();
+  gdb_assert (targ != nullptr);
+  targ->info_proc_mappings (targ->core_gdbarch ());
+}
+
 void _initialize_corelow ();
 void
 _initialize_corelow ()
 {
   add_target (core_target_info, core_target_open, filename_completer);
+  add_cmd ("core-file-backed-mappings", class_maintenance,
+           maintenance_print_core_file_backed_mappings,
+	   _("Print core file's file-backed mappings"),
+	   &maintenanceprintlist);
 }
-- 
2.26.2

[PATCH v4 13/14] Add documentation for "maint print core-file-backed-mappings"

[Kevin Buettner]

gdb/ChangeLog:

	* NEWS (New commands): Mention new command
	"maintenance print core-file-backed-mappings".

gdb/doc/ChangeLog:

	* gdb.texinfo (Maintenance Commands): Add documentation for
	new command "maintenance print core-file-backed-mappings".
---
 gdb/NEWS            | 4 ++++
 gdb/doc/gdb.texinfo | 8 ++++++++
 2 files changed, 12 insertions(+)

diff --git a/gdb/NEWS b/gdb/NEWS
index a116d62bca..fe5396b3a0 100644
--- a/gdb/NEWS
+++ b/gdb/NEWS
@@ -85,6 +85,10 @@ maintenance print xml-tdesc [FILE]
   the target description is read from FILE into GDB, and then
   reprinted.
 
+maintenance print core-file-backed-mappings
+  Prints file-backed mappings loaded from a core file's note section.
+  Output is expected to be similar to that of "info proc mappings".
+
 * Changed commands
 
 alias [-a] [--] ALIAS = COMMAND [DEFAULT-ARGS...]
diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index fbe9f850af..6e54115730 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -38526,6 +38526,14 @@ library.  This exercises all @code{libthread_db} functionality used by
 @code{libthread_db} uses.  Note that parts of the test may be skipped
 on some platforms when debugging core files.
 
+@kindex maint print core-file-backed-mappings
+@cindex memory address space mappings
+@item maint print core-file-backed-mappings
+Print the file-backed mappings which were loaded from a core file note.
+This output represents state internal to @value{GDBN} and should be
+similar to the mappings displayed by the @code{info proc mappings}
+command.
+
 @kindex maint print dummy-frames
 @item maint print dummy-frames
 Prints the contents of @value{GDBN}'s internal dummy-frame stack.
-- 
2.26.2

[PATCH v4 14/14] New core file tests with mappings over existing program memory

[Kevin Buettner]

This test case was inspired by Pedro's demonstration of a problem
with my v2 patches.  It can be found here:

    https://sourceware.org/pipermail/gdb-patches/2020-May/168826.html

In a nutshell, my earlier patches could not handle the case in
which a read-only mapping created with mmap() was created at
an address used by other file-backed read-only memory in use by
the process.

This problem has been fixed (for Linux, anyway) by the commit "Use
NT_FILE note section for reading core target memory".

When I run this test without any of my recent corefile patches,
I see these failures:

FAIL: gdb.base/corefile2.exp: kernel core: print/x mbuf_ro[0]@4
FAIL: gdb.base/corefile2.exp: kernel core: print/x mbuf_ro[pagesize-4]@4
FAIL: gdb.base/corefile2.exp: kernel core: print/x mbuf_ro[-3]@6
FAIL: gdb.base/corefile2.exp: kernel core: print/x mbuf_rw[pagesize-3]@6
FAIL: gdb.base/corefile2.exp: kernel core: print/x mbuf_ro[pagesize-3]@6
FAIL: gdb.base/corefile2.exp: maint print core-file-backed-mappings
FAIL: gdb.base/corefile2.exp: gcore core: print/x mbuf_ro[-3]@6

The ones involving mbuf_ro will almost certainly fail when run on
non-Linux systems; I've used setup_xfail on those tests to prevent
them from outright FAILing when not run on Linux.  For a time, I
had considered skipping these tests altogether when not run on
Linux, but I changed my mind due to this failure...

FAIL: gdb.base/corefile2.exp: print/x mbuf_rw[pagesize-3]@6

I think it *should* pass without my recent corefile patches.  The fact
that it doesn't is likely due to a bug in GDB.  The following
interaction with GDB demonstrates the problem:

(gdb) print/x mbuf_rw[pagesize-3]@6
$1 = {0x0, 0x0, 0x0, 0x0, 0x0, 0x0}
(gdb) print/x mbuf_rw[pagesize]@3
$2 = {0x6b, 0x6b, 0x6b}

The last three values in display of $1 should be the same as those
shown by $2.  Like this...

(gdb) print/x mbuf_rw[pagesize-3]@6
$1 = {0x0, 0x0, 0x0, 0x6b, 0x6b, 0x6b}
(gdb) print/x mbuf_rw[pagesize]@3
$2 = {0x6b, 0x6b, 0x6b}

That latter output was obtained with the use of all of my current
corefile patches.  I see no failures on Linux when running this test
with my current set of corefile patches.  I tested 3 architectures:
x86_64, s390x, and aarch64.

I also tested on FreeBSD 12.1-RELEASE.  I see the following results
both with and without the current set of core file patches:

    # of expected passes		25
    # of expected failures		8

Of particular interest is that I did *not* see the problematic mbuf_rw
failure noted earlier (both with and without the core file patches).
I still don't have an explanation for why this failure occurred on
Linux.  Prior to running the tests, I had hypothesized that I'd see
this failure on FreeBSD too, but testing shows that this is not the
case.

Also of importance is that we see no FAILs with this test on FreeBSD
which indicates that I XFAILed the correct tests.

This version runs the interesting tests twice, once with a kernel
created core file and another time with a gcore created core file.

It also does a very minimal test of the new command "maint print
core-file-backed-mappings".

gdb/testsuite/ChangeLog:

	* gdb.base/corefile2.exp: New file.
	* gdb.base/coremaker2.exp: New file.
---
 gdb/testsuite/gdb.base/corefile2.exp | 179 +++++++++++++++++++++++++++
 gdb/testsuite/gdb.base/coremaker2.c  | 150 ++++++++++++++++++++++
 2 files changed, 329 insertions(+)
 create mode 100644 gdb/testsuite/gdb.base/corefile2.exp
 create mode 100644 gdb/testsuite/gdb.base/coremaker2.c

diff --git a/gdb/testsuite/gdb.base/corefile2.exp b/gdb/testsuite/gdb.base/corefile2.exp
new file mode 100644
index 0000000000..fde6f07a68
--- /dev/null
+++ b/gdb/testsuite/gdb.base/corefile2.exp
@@ -0,0 +1,179 @@
+# Copyright 2020 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/>.
+
+# Tests of core file memory accesses when mmap() has been used to
+# create a "hole" of zeroes over pre-existing memory regions.  See
+# coremaker2.c for details.
+
+# are we on a target board
+if ![isnative] then {
+    return
+}
+
+# Some of these tests will only work on GNU/Linux due to the
+# fact that Linux core files includes a section describing
+# memory address to file mappings.  We'll use set_up_xfail for the
+# affected tests.  As other targets become supported, the condition
+# can be changed accordingly.
+
+set xfail 0
+if { ![istarget *-linux*] } {
+    set xfail 1
+}
+
+standard_testfile coremaker2.c
+
+if {[build_executable $testfile.exp $testfile $srcfile debug] == -1} {
+    untested "failed to compile"
+    return -1
+}
+
+set corefile [core_find $binfile {}]
+if {$corefile == ""} {
+    return 0
+}
+
+gdb_start
+gdb_reinitialize_dir $srcdir/$subdir
+gdb_load ${binfile}
+
+# Attempt to load the core file.
+
+gdb_test_multiple "core-file $corefile" "core-file command" {
+    -re ".* program is being debugged already.*y or n. $" {
+	# gdb_load may connect us to a gdbserver.
+	send_gdb "y\n"
+	exp_continue
+    }
+    -re "Core was generated by .*corefile.*\r\n\#0  .*\(\).*\r\n$gdb_prompt $" {
+	pass "core-file command"
+    }
+    -re "Core was generated by .*\r\n\#0  .*\(\).*\r\n$gdb_prompt $" {
+	pass "core-file command (with bad program name)"
+    }
+    -re ".*registers from core file: File in wrong format.* $" {
+	fail "core-file command (could not read registers from core file)"
+    }
+}
+
+# Perform the "interesting" tests which check the contents of certain
+# memory regions.
+
+proc do_tests { } {
+    global xfail
+
+    # Check contents of beginning of buf_rw and buf_ro.
+
+    gdb_test {print/x buf_rw[0]@4} {\{0x6b, 0x6b, 0x6b, 0x6b\}}
+    gdb_test {print/x buf_ro[0]@4} {\{0xc5, 0xc5, 0xc5, 0xc5\}}
+
+    # Check for correct contents at beginning of mbuf_rw and mbuf_ro.
+
+    gdb_test {print/x mbuf_rw[0]@4} {\{0x0, 0x0, 0x0, 0x0\}}
+
+    if { $xfail } { setup_xfail "*-*-*" }
+    gdb_test {print/x mbuf_ro[0]@4} {\{0x0, 0x0, 0x0, 0x0\}}
+
+    # Check contents of mbuf_rw and mbuf_ro at the end of these regions.
+
+    gdb_test {print/x mbuf_rw[pagesize-4]@4} {\{0x0, 0x0, 0x0, 0x0\}}
+
+    if { $xfail } { setup_xfail "*-*-*" }
+    gdb_test {print/x mbuf_ro[pagesize-4]@4} {\{0x0, 0x0, 0x0, 0x0\}}
+
+    # Check contents of mbuf_rw and mbuf_ro, right before the hole,
+    # overlapping into the beginning of these mmap'd regions.
+
+    gdb_test {print/x mbuf_rw[-3]@6} {\{0x6b, 0x6b, 0x6b, 0x0, 0x0, 0x0\}}
+
+    if { $xfail } { setup_xfail "*-*-*" }
+    gdb_test {print/x mbuf_ro[-3]@6} {\{0xc5, 0xc5, 0xc5, 0x0, 0x0, 0x0\}}
+
+    # Likewise, at the end of the mbuf_rw and mbuf_ro, with overlap.
+
+    # If this test FAILs, it's probably a genuine bug unrelated to whether
+    # the core file includes a section describing memory address to file
+    # mappings or not.  (So don't xfail it!)
+    gdb_test {print/x mbuf_rw[pagesize-3]@6} {\{0x0, 0x0, 0x0, 0x6b, 0x6b, 0x6b\}}
+
+    if { $xfail } { setup_xfail "*-*-*" }
+    gdb_test {print/x mbuf_ro[pagesize-3]@6} {\{0x0, 0x0, 0x0, 0xc5, 0xc5, 0xc5\}}
+
+    # Check contents of (what should be) buf_rw and buf_ro immediately after
+    # mbuf_rw and mbuf_ro holes.
+
+    gdb_test {print/x mbuf_rw[pagesize]@4} {\{0x6b, 0x6b, 0x6b, 0x6b\}}
+    gdb_test {print/x mbuf_ro[pagesize]@4} {\{0xc5, 0xc5, 0xc5, 0xc5\}}
+
+    # Check contents at ends of buf_rw and buf_rw.
+
+    gdb_test {print/x buf_rw[sizeof(buf_rw)-4]@4} {\{0x6b, 0x6b, 0x6b, 0x6b\}}
+    gdb_test {print/x buf_ro[sizeof(buf_ro)-4]@4} {\{0xc5, 0xc5, 0xc5, 0xc5\}}
+}
+
+# Run tests with kernel-produced core file.
+
+with_test_prefix "kernel core" {
+    do_tests
+}
+
+# Verify that "maint print core-file-backed-mappings" exists and does not
+# crash GDB. If it produces any output all, make sure that that output
+# at least mentions binfile.
+
+set test "maint print core-file-backed-mappings"
+gdb_test_multiple $test "" {
+    -re ".*$binfile.*$gdb_prompt $" {
+	pass $test
+    }
+    -re "^$test\[\r\n\]*$gdb_prompt $" {
+	pass "$test (no output)"
+    }
+}
+
+# Restart and run to the abort call.
+
+clean_restart $binfile
+
+if ![runto_main] then {
+    fail "can't run to main"
+    return
+}
+
+gdb_breakpoint [gdb_get_line_number "abort"]
+gdb_continue_to_breakpoint "at abort"
+
+# Do not execute abort call; instead, invoke gcore command to make a
+# gdb-produced core file.
+
+set corefile [standard_output_file gcore.test]
+set core_supported [gdb_gcore_cmd "$corefile" "save a corefile"]
+if {!$core_supported} {
+  return
+}
+
+clean_restart $binfile
+
+set core_loaded [gdb_core_cmd "$corefile" "re-load generated corefile"]
+if { $core_loaded == -1 } {
+    # No use proceeding from here.
+    return
+}
+
+# Run tests using gcore-produced core file.
+
+with_test_prefix "gcore core" {
+    do_tests
+}
diff --git a/gdb/testsuite/gdb.base/coremaker2.c b/gdb/testsuite/gdb.base/coremaker2.c
new file mode 100644
index 0000000000..ecba247f50
--- /dev/null
+++ b/gdb/testsuite/gdb.base/coremaker2.c
@@ -0,0 +1,150 @@
+/* Copyright 1992-2020 Free Software Foundation, Inc.
+
+   This file is part of GDB.
+
+   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/>.  */
+
+/*  This test has two large memory areas buf_rw and buf_ro. 
+
+    buf_rw is written to by the program while buf_ro is initialized at
+    compile / load time.  Thus, when a core file is created, buf_rw's
+    memory should reside in the core file, but buf_ro probably won't be.
+    Instead, the contents of buf_ro are available from the executable.
+
+    Now, for the wrinkle:  We create a one page read-only mapping over
+    both of these areas.  This will create a one page "hole" of all
+    zeros in each area.
+
+    Will GDB be able to correctly read memory from each of the four
+    (or six, if you count the regions on the other side of each hole)
+    memory regions?  */
+
+#include <stdio.h>
+#include <sys/types.h>
+#include <fcntl.h>
+#include <sys/mman.h>
+#include <signal.h>
+#include <stdlib.h>
+#include <unistd.h>
+#include <string.h>
+#include <errno.h>
+
+/* These are globals so that we can find them easily when debugging
+   the core file.  */
+long pagesize;
+unsigned long long addr;
+char *mbuf_ro;
+char *mbuf_rw;
+
+/* 24 KiB buffer.  */
+char buf_rw[24 * 1024];
+
+/* 24 KiB worth of data.  For this test case, we can't allocate a
+   buffer and then fill it; we want GDB to have to read this data
+   from the executable; it should NOT find it in the core file.  */
+
+#define C5_16 \
+  0xc5, 0xc5, 0xc5, 0xc5, \
+  0xc5, 0xc5, 0xc5, 0xc5, \
+  0xc5, 0xc5, 0xc5, 0xc5, \
+  0xc5, 0xc5, 0xc5, 0xc5
+
+#define C5_256 \
+  C5_16, C5_16, C5_16, C5_16, \
+  C5_16, C5_16, C5_16, C5_16, \
+  C5_16, C5_16, C5_16, C5_16, \
+  C5_16, C5_16, C5_16, C5_16
+
+#define C5_1k \
+  C5_256, C5_256, C5_256, C5_256
+
+#define C5_24k \
+  C5_1k, C5_1k, C5_1k, C5_1k, \
+  C5_1k, C5_1k, C5_1k, C5_1k, \
+  C5_1k, C5_1k, C5_1k, C5_1k, \
+  C5_1k, C5_1k, C5_1k, C5_1k, \
+  C5_1k, C5_1k, C5_1k, C5_1k, \
+  C5_1k, C5_1k, C5_1k, C5_1k
+
+const char buf_ro[] = { C5_24k };
+
+int
+main (int argc, char **argv)
+{
+  int i, bitcount;
+
+#ifdef _SC_PAGESIZE
+  pagesize = sysconf (_SC_PAGESIZE);
+#else
+  pagesize = 8192;
+#endif
+
+  /* Verify that pagesize is a power of 2.  */
+  bitcount = 0;
+  for (i = 0; i < 4 * sizeof (pagesize); i++)
+    if (pagesize & (1 << i))
+      bitcount++;
+
+  if (bitcount != 1)
+    {
+      fprintf (stderr, "pagesize is not a power of 2.\n");
+      exit (1);
+    }
+
+  /* Compute an address that should be within buf_ro.  Complain if not.  */
+  addr = ((unsigned long long) buf_ro + pagesize) & ~(pagesize - 1);
+
+  if (addr <= (unsigned long long) buf_ro
+      || addr >= (unsigned long long) buf_ro + sizeof (buf_ro))
+    {
+      fprintf (stderr, "Unable to compute a suitable address within buf_ro.\n");
+      exit (1);
+    }
+
+  mbuf_ro = mmap ((void *) addr, pagesize, PROT_READ,
+               MAP_ANONYMOUS | MAP_PRIVATE | MAP_FIXED, -1, 0);
+
+  if (mbuf_ro == MAP_FAILED)
+    {
+      fprintf (stderr, "mmap #1 failed: %s.\n", strerror (errno));
+      exit (1);
+    }
+
+  /* Write (and fill) the R/W region.  */
+  for (i = 0; i < sizeof (buf_rw); i++)
+    buf_rw[i] = 0x6b;
+
+  /* Compute an mmap address within buf_rw.  Complain if it's somewhere
+     else.  */
+  addr = ((unsigned long long) buf_rw + pagesize) & ~(pagesize - 1);
+
+  if (addr <= (unsigned long long) buf_rw
+      || addr >= (unsigned long long) buf_rw + sizeof (buf_rw))
+    {
+      fprintf (stderr, "Unable to compute a suitable address within buf_rw.\n");
+      exit (1);
+    }
+
+  mbuf_rw = mmap ((void *) addr, pagesize, PROT_READ,
+               MAP_ANONYMOUS | MAP_PRIVATE | MAP_FIXED, -1, 0);
+
+  if (mbuf_rw == MAP_FAILED)
+    {
+      fprintf (stderr, "mmap #2 failed: %s.\n", strerror (errno));
+      exit (1);
+    }
+
+  /* With correct ulimit, etc. this should cause a core dump.  */
+  abort ();
+}
-- 
2.26.2

Re: [PATCH v4 13/14] Add documentation for "maint print core-file-backed-mappings"

[Eli Zaretskii]

> From: Kevin Buettner <kevinb@redhat.com>
> Cc: Kevin Buettner <kevinb@redhat.com>,
> 	Eli Zaretskii <eliz@gnu.org>
> Date: Sun,  5 Jul 2020 15:58:07 -0700
> 
> gdb/ChangeLog:
> 
> 	* NEWS (New commands): Mention new command
> 	"maintenance print core-file-backed-mappings".
> 
> gdb/doc/ChangeLog:
> 
> 	* gdb.texinfo (Maintenance Commands): Add documentation for
> 	new command "maintenance print core-file-backed-mappings".

Thanks, this is OK.

Re: [PATCH v4 11/14] xfail gdb.base/coredump-filter.exp test which now works without a binary

[Pedro Alves]

On 7/5/20 11:58 PM, Kevin Buettner via Gdb-patches wrote:
> See comment in patch for a description of what this is about.  In
> a nutshell, a certain memory access was expected to not work in
> order to PASS, but due to recent changes, the memory access now
> works causing the test to FAIL.
> 
> gdb/testsuite/ChangeLog:
> 
> 	* gdb.base/coredump-filter.exp (test_disasm):  Call
> 	setup_xfail for test "disassemble function with corefile and
> 	without a binary".
> ---
>  gdb/testsuite/gdb.base/coredump-filter.exp | 21 +++++++++++++++++++++
>  1 file changed, 21 insertions(+)
> 
> diff --git a/gdb/testsuite/gdb.base/coredump-filter.exp b/gdb/testsuite/gdb.base/coredump-filter.exp
> index ff398f2b85..fd4e0352d8 100644
> --- a/gdb/testsuite/gdb.base/coredump-filter.exp
> +++ b/gdb/testsuite/gdb.base/coredump-filter.exp
> @@ -96,6 +96,27 @@ proc test_disasm { core address should_fail } {
>  	}
>  
>  	if { $should_fail == 1 } {
> +	    # As originally conceived, an attempt here to disassemble
> +	    # addresses in main() with the core file loaded, but with
> +	    # the executable not loaded shouldn't work .  This was a
> +	    # good idea because it demonstrates that executable-backed
> +	    # memory was not dumped to the core file.  However, this
> +	    # test now fails (i.e. now successfully disassembles
> +	    # addresses in main()) due to the fact that GDB loads the
> +	    # file-based mappings in Linux's NT_FILE note.  So, even
> +	    # though the executable wasn't explicitly loaded, GDB will
> +	    # still be able to find the file-backed memory via the
> +	    # note (referencing the file) within the core file.  The
> +	    # data in question is not actually stored in the core
> +	    # file, but is instead found in one of the files mentioned
> +	    # in the core file note.
> +	    #
> +	    # It's tempting to simply remove this test because it's
> +	    # apparently no longer useful.  However, the idea behind
> +	    # the test is useful, so it's marked as xfail, along with
> +	    # this comment, as a reminder for someone to come up with
> +	    # a new approach.
> +	    setup_xfail "*-*-*"
>  	    gdb_test "x/i \$pc" "=> $hex:\tCannot access memory at address $hex" \
>  		"disassemble function with corefile and without a binary"
>  	} else {
> 

Could we instead make the test dependent on whether we have
the file mappings?  Like e.g., try if "info proc mappings" works
and store the result in a $have_mappings variable.  And then:

if !$have_mappings
  expect error
else
  expect some reasonable output and no error

Maybe the "should_fail" variable would be better renamed to
"have_binary" or something like that?

Also, not sure whether the test_disasm intro comment needs updating.

Thanks,
Pedro Alves

Re: [PATCH v4 12/14] Add new command "maint print core-file-backed-mappings"

[Pedro Alves]

On 7/5/20 11:58 PM, Kevin Buettner via Gdb-patches wrote:
> +  for (const struct target_section *tsp = m_core_file_mappings.sections;
> +       tsp < m_core_file_mappings.sections_end; tsp++ )
> +    {

Spurious space after tsp++.

If you need to break the for line, I personally prefer to then
put each statement in its own line, like:

 for (const struct target_section *tsp = m_core_file_mappings.sections;
      tsp < m_core_file_mappings.sections_end; 
      ++tsp)

> +static void
> +maintenance_print_core_file_backed_mappings (const char *args, int from_tty)
> +{
> +  core_target *targ = get_current_core_target ();
> +  gdb_assert (targ != nullptr);

This assert will crash GDB if you're debugging a core dump, right?
That doesn't look right.  (Please add a test for that.)

> +  targ->info_proc_mappings (targ->core_gdbarch ());
> +}


I don't understand what this command provides that "info proc mappings"
doesn't?  Can you give an example of when you'd use this command
over "info proc mappings" ?

Thanks,
Pedro Alves

Re: [PATCH v4 08/14] Use NT_FILE note section for reading core target memory

[Pedro Alves]

On 7/5/20 11:58 PM, Kevin Buettner via Gdb-patches wrote:

> +void
> +core_target::build_file_mappings ()
> +{
> +  std::unordered_map<std::string, struct bfd *> bfd_map;
> +
> +  /* See linux_read_core_file_mappings() in linux-tdep.c for an example
> +     read_core_file_mappings method.  */
> +  gdbarch_read_core_file_mappings (m_core_gdbarch, core_bfd,
> +
> +    /* After determining the number of mappings, read_core_file_mappings
> +       will invoke this lambda which allocates target_section storage for
> +       the mappings.  */
> +    [&] (ULONGEST count)
> +      {
> +	m_core_file_mappings.sections = XNEWVEC (struct target_section, count);
> +	m_core_file_mappings.sections_end = m_core_file_mappings.sections;
> +      },
> +
> +    /* read_core_file_mappings will invoke this lambda for each mapping
> +       that it finds.  */
> +    [&] (int num, ULONGEST start, ULONGEST end, ULONGEST file_ofs,
> +         const char *filename, const void *other)
> +      {
> +	/* Architecture-specific read_core_mapping methods are expected to
> +	   weed out non-file-backed mappings.  */
> +	gdb_assert (filename != nullptr);
> +
> +	struct bfd *bfd = bfd_map[filename];
> +	if (bfd == nullptr) {

Brace on next line.

> +
> +	  /* Use exec_file_find() to do sysroot expansion.  It'll also strip
> +	     the potential sysroot target: prefix.  If there is no sysroot, an
> +	     equivalent (possibly more canonical) pathname will be provided.  */
> +	  gdb::unique_xmalloc_ptr<char> expanded_fname
> +	    = exec_file_find (filename, NULL);
> +	  if (expanded_fname == nullptr)
> +	    {
> +	      warning (_("Can't open file %s during file-backed mapping "
> +	                 "note processing"),
> +		       expanded_fname.get ());
> +	      return;
> +	    }
> +
> +	  bfd = bfd_map[filename] = bfd_openr (expanded_fname.get (), "binary");
> +
> +	  if (bfd == nullptr || !bfd_check_format (bfd, bfd_object))
> +	    {
> +	      /* If we get here, there's a good chance that it's due to
> +		 an internal error.  We issue a warning instead of an
> +		 internal error because of the possibility that the
> +		 file was removed in between checking for its
> +		 existence during the expansion in exec_file_find()
> +		 and the calls to bfd_openr() / bfd_check_format(). 
> +		 Output both the path from the core file note along
> +		 with its expansion to make debugging this problem
> +		 easier.  */
> +	      warning (_("Can't open file %s which was expanded to %s "
> +			 "during file-backed mapping note processing"),
> +		       filename, expanded_fname.get ());
> +	      return;

Don't we have to close the bfd if it was opened, but failed the
bfd_check_format check?

> +	    }
> +	    /* Ensure that the bfd will be closed when core_bfd is closed. 
> +	       This can be checked before/after a core file detach via
> +	       "maint info bfds".  */
> +	    gdb_bfd_record_inclusion (core_bfd, bfd);
> +	}
> +
> +	/* Make new BFD section.  */
> +	char secnamebuf[64];
> +	sprintf (secnamebuf, "S%04d", num);
> +	char *secname = (char *) bfd_alloc (bfd, strlen (secnamebuf) + 1);
> +	if (secname == nullptr)
> +	  error (_("Out of memory"));
> +	strcpy (secname, secnamebuf);
> +	asection *sec = bfd_make_section_anyway (bfd, secname);
> +	if (sec == nullptr)
> +	  error (_("Can't make section"));

SECNAME leaks if you throw here.  Use a unique pointer?
Also, should these be malloc_failure instead?

> +	sec->filepos = file_ofs;
> +	bfd_set_section_flags (sec, SEC_READONLY | SEC_HAS_CONTENTS);
> +	bfd_set_section_size (sec, end - start);
> +	bfd_set_section_vma (sec, start);
> +	bfd_set_section_lma (sec, start);
> +	bfd_set_section_alignment (sec, 2);
> +
> +	/* Set target_section fields.  */
> +	struct target_section *ts = m_core_file_mappings.sections_end++;
> +	ts->addr = start;
> +	ts->endaddr = end;
> +	ts->owner = nullptr;
> +	ts->the_bfd_section = sec;
> +      });
>  }
>  

Otherwise LGTM.

Re: [PATCH v4 14/14] New core file tests with mappings over existing program memory

[Pedro Alves]

On 7/5/20 11:58 PM, Kevin Buettner via Gdb-patches wrote:
> +# Verify that "maint print core-file-backed-mappings" exists and does not
> +# crash GDB. If it produces any output all, make sure that that output
> +# at least mentions binfile.

Did you mean "any output AT all"?  Also, double space after period.

> +
> +set test "maint print core-file-backed-mappings"
> +gdb_test_multiple $test "" {
> +    -re ".*$binfile.*$gdb_prompt $" {
> +	pass $test
> +    }
> +    -re "^$test\[\r\n\]*$gdb_prompt $" {
> +	pass "$test (no output)"
> +    }
> +}

Re: [PATCH v4 12/14] Add new command "maint print core-file-backed-mappings"

[Pedro Alves]

On 7/10/20 9:08 PM, Pedro Alves wrote:> 
>> +static void
>> +maintenance_print_core_file_backed_mappings (const char *args, int from_tty)
>> +{
>> +  core_target *targ = get_current_core_target ();
>> +  gdb_assert (targ != nullptr);

> This assert will crash GDB if you're debugging a core dump, right?
> That doesn't look right.  (Please add a test for that.)

This assert will crash GDB if you're NOT debugging a core dump,
of course.

Re: [PATCH v4 00/14] Fix BZ 25631 - core file memory access problem

[Pedro Alves]

On 7/5/20 11:57 PM, Kevin Buettner via Gdb-patches wrote:
> This series fixes several core file related bugs.  The bug which
> started this work can be viewed here:
> 
>     https://sourceware.org/bugzilla/show_bug.cgi?id=25631
> 
> Other problems were found either during review or during development. 
> I discuss these in my commit log remarks.
> 
> Only some of these patches still need review.  A brief status
> of each can be found below:
> 
>  1) Remove hack for GDB which sets the section size to 0
> 
>     Nick Clifton has approved this patch.
> 
>  2) Adjust corefile.exp test to show regression after bfd hack removal
>  3) section_table_xfer_memory: Replace section name with callback
>     predicate
>  4) Provide access to non SEC_HAS_CONTENTS core file sections
>  5) Test ability to access unwritten-to mmap data in core file
> 
>     I think that Pedro is okay with #2 thru #5.
> 
>  6) Update binary_get_section_contents to seek using section's file
>     position
> 
>     Nick Cliften has approved patch #6.
> 
>  7) Add new gdbarch method, read_core_file_mappings
> 
>     This is a new patch which needs review.
> 
>  8) Use NT_FILE note section for reading core target memory
> 
>     This patch is significantly different than the v3 patch.
>     It needs review.  
> 
>  9) Add test for accessing read-only mmapped data in a core file
> 
>     I think that Pedro is oky with this one.
> 
> 10) gcore command: Place all file-backed mappings in NT_FILE note
> 11) xfail gdb.base/coredump-filter.exp test which now works without a
>     binary
> 12) Add new command "maint print core-file-backed-mappings"
> 
>     #10 thru #12 are new patches which need review.
> 
> 13) Add documentation for "maint print core-file-backed-mappings"
> 
>     This one needs a review from Eli.
> 
> 14) New core file tests with mappings over existing program memory
> 
>     I've made all the changes that Pedro recommended, plus added
>     a test for the new maint print command.  It needs review.


I think this looks overall very good.

I've sent some comments to patches #8, #11, #12, and #14.

Once those are revolved, I'll be happy to see this go in.

Pedro Alves

RE: [PATCH v4 11/14] xfail gdb.base/coredump-filter.exp test which now works without a binary

[Strasuns, Mihails]

Idea: now that NT_FILE works, it probably makes sense to replace this test with two sequential ones:

1) First test that it still works if file is accessible in the filesystem
2) Temporarily move / rename the file and test that disassemble doesn't work anymore

Best regards,
Mihails

> -----Original Message-----
> From: Gdb-patches <gdb-patches-bounces@sourceware.org> On Behalf Of
> Pedro Alves
> Sent: Friday, July 10, 2020 10:08 PM
> To: Kevin Buettner <kevinb@redhat.com>; gdb-patches@sourceware.org
> Subject: Re: [PATCH v4 11/14] xfail gdb.base/coredump-filter.exp test which
> now works without a binary
> 
> On 7/5/20 11:58 PM, Kevin Buettner via Gdb-patches wrote:
> > See comment in patch for a description of what this is about.  In a
> > nutshell, a certain memory access was expected to not work in order to
> > PASS, but due to recent changes, the memory access now works causing
> > the test to FAIL.
> >
> > gdb/testsuite/ChangeLog:
> >
> > 	* gdb.base/coredump-filter.exp (test_disasm):  Call
> > 	setup_xfail for test "disassemble function with corefile and
> > 	without a binary".
> > ---
> >  gdb/testsuite/gdb.base/coredump-filter.exp | 21
> +++++++++++++++++++++
> >  1 file changed, 21 insertions(+)
> >
> > diff --git a/gdb/testsuite/gdb.base/coredump-filter.exp
> > b/gdb/testsuite/gdb.base/coredump-filter.exp
> > index ff398f2b85..fd4e0352d8 100644
> > --- a/gdb/testsuite/gdb.base/coredump-filter.exp
> > +++ b/gdb/testsuite/gdb.base/coredump-filter.exp
> > @@ -96,6 +96,27 @@ proc test_disasm { core address should_fail } {
> >  	}
> >
> >  	if { $should_fail == 1 } {
> > +	    # As originally conceived, an attempt here to disassemble
> > +	    # addresses in main() with the core file loaded, but with
> > +	    # the executable not loaded shouldn't work .  This was a
> > +	    # good idea because it demonstrates that executable-backed
> > +	    # memory was not dumped to the core file.  However, this
> > +	    # test now fails (i.e. now successfully disassembles
> > +	    # addresses in main()) due to the fact that GDB loads the
> > +	    # file-based mappings in Linux's NT_FILE note.  So, even
> > +	    # though the executable wasn't explicitly loaded, GDB will
> > +	    # still be able to find the file-backed memory via the
> > +	    # note (referencing the file) within the core file.  The
> > +	    # data in question is not actually stored in the core
> > +	    # file, but is instead found in one of the files mentioned
> > +	    # in the core file note.
> > +	    #
> > +	    # It's tempting to simply remove this test because it's
> > +	    # apparently no longer useful.  However, the idea behind
> > +	    # the test is useful, so it's marked as xfail, along with
> > +	    # this comment, as a reminder for someone to come up with
> > +	    # a new approach.
> > +	    setup_xfail "*-*-*"
> >  	    gdb_test "x/i \$pc" "=> $hex:\tCannot access memory at address
> $hex" \
> >  		"disassemble function with corefile and without a binary"
> >  	} else {
> >
> 
> Could we instead make the test dependent on whether we have the file
> mappings?  Like e.g., try if "info proc mappings" works and store the result in
> a $have_mappings variable.  And then:
> 
> if !$have_mappings
>   expect error
> else
>   expect some reasonable output and no error
> 
> Maybe the "should_fail" variable would be better renamed to "have_binary"
> or something like that?
> 
> Also, not sure whether the test_disasm intro comment needs updating.
> 
> Thanks,
> Pedro Alves
Intel Deutschland GmbH
Registered Address: Am Campeon 10-12, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de
Managing Directors: Christin Eisenschmid, Gary Kershaw
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928

Re: [PATCH v4 11/14] xfail gdb.base/coredump-filter.exp test which now works without a binary

[Kevin Buettner]

On Mon, 13 Jul 2020 11:38:49 +0000
"Strasuns, Mihails via Gdb-patches" <gdb-patches@sourceware.org> wrote:

> Idea: now that NT_FILE works, it probably makes sense to replace this test with two sequential ones:
> 
> 1) First test that it still works if file is accessible in the filesystem
> 2) Temporarily move / rename the file and test that disassemble doesn't work anymore

That's a good idea.  I'll give it a try...

Thanks,

Kevin

Re: [PATCH v4 12/14] Add new command "maint print core-file-backed-mappings"

[Kevin Buettner]

On Fri, 10 Jul 2020 21:08:15 +0100
Pedro Alves <pedro@palves.net> wrote:

> I don't understand what this command provides that "info proc mappings"
> doesn't?  Can you give an example of when you'd use this command
> over "info proc mappings" ?

I wanted it while adding core file note support for FreeBSD.  I wanted
to check that 1) the mappings were actually be loaded and 2) the
mappings loaded by corelow.c were correct.  I think it might be occasionally
useful by other GDB developers for debugging core file related problems.

A normal GDB user won't use this command; that's why it's a maintenance
command.  Depending on the OS there may be differences between
"info proc mappings" and this new maintenance command.  At the
moment, there aren't any differences on Linux, but there will be for
FreeBSD.  FreeBSD provides more mappings and more fields for each
mapping.  I would expect that "info proc mappings" on FreeBSD will
show all mappings along with the additional fields.  The maintenance
command will show a subset of that information; as such it might not
seem terribly useful, but the point of it is to output information from
the corelow.c created data structures so that a developer can check
that they exist and are correct.

I'll add a few more words to the commit log.

Kevin

Re: [PATCH v4 08/14] Use NT_FILE note section for reading core target memory

[Kevin Buettner]

On Fri, 10 Jul 2020 21:08:27 +0100
Pedro Alves <pedro@palves.net> wrote:

> > +	/* Make new BFD section.  */
> > +	char secnamebuf[64];
> > +	sprintf (secnamebuf, "S%04d", num);
> > +	char *secname = (char *) bfd_alloc (bfd, strlen (secnamebuf) + 1);
> > +	if (secname == nullptr)
> > +	  error (_("Out of memory"));
> > +	strcpy (secname, secnamebuf);
> > +	asection *sec = bfd_make_section_anyway (bfd, secname);
> > +	if (sec == nullptr)
> > +	  error (_("Can't make section"));  
> 
> SECNAME leaks if you throw here.  Use a unique pointer?

It turns out that this code will leak even if the error pathway is not
taken.  After studying bfd/section.c, it appears that nothing ever
frees the section name.  bfd_make_section_anyway{,with_flags} is
usually (though not always) called with a constant string like
".dynstr", ".plt", ".buildid", etc.  In those cases where it is passed
a dynamically allocated string, I see no provision for freeing the
section name.  I strongly suspect that there are leaks where this
occurs.

It turns out that bfd_make_section_anyway() will create a new
section even if passed a name of an already existing section.  I'm
experimenting with just passing in "load" as the name for each
needed section.  So far, it seems to work.

Kevin

Re: [PATCH v4 08/14] Use NT_FILE note section for reading core target memory

[Kevin Buettner]

On Fri, 10 Jul 2020 21:08:27 +0100
Pedro Alves <pedro@palves.net> wrote:

> > +	/* Make new BFD section.  */
> > +	char secnamebuf[64];
> > +	sprintf (secnamebuf, "S%04d", num);
> > +	char *secname = (char *) bfd_alloc (bfd, strlen (secnamebuf) + 1);
> > +	if (secname == nullptr)
> > +	  error (_("Out of memory"));
> > +	strcpy (secname, secnamebuf);
> > +	asection *sec = bfd_make_section_anyway (bfd, secname);
> > +	if (sec == nullptr)
> > +	  error (_("Can't make section"));  
> 
[...]
> Also, should these be malloc_failure instead?

I think that malloc_failure would have been appropriate for a failed
bfd_alloc() call.  I've removed this call now, so it's no longer
relevant.

I think that calling error() is still appropriate for a failed call to
bfd_make_section_anyway().  I did take a look to see how other code
within GDB handled a failed call to bfd_make_section_anyway.* :

- record_full_base_target::save_record in record.c calls error().
- write_gcore_file_1 and gcore_create_callback in gcore.c both call
  error().
- dump_bfd_file in cli/cli-dump.c does NOT handle failure from
  bfd_make_section_anyway.  I.e. it'll segfault from a NULL pointer
  access!

Kevin

Re: [PATCH v4 08/14] Use NT_FILE note section for reading core target memory

[Tom Tromey]

>>>>> "Kevin" == Kevin Buettner via Gdb-patches <gdb-patches@sourceware.org> writes:

>> > +	char *secname = (char *) bfd_alloc (bfd, strlen (secnamebuf) + 1);

>> SECNAME leaks if you throw here.  Use a unique pointer?

Kevin> It turns out that this code will leak even if the error pathway is not
Kevin> taken.  After studying bfd/section.c, it appears that nothing ever
Kevin> frees the section name.  bfd_make_section_anyway{,with_flags} is
Kevin> usually (though not always) called with a constant string like
Kevin> ".dynstr", ".plt", ".buildid", etc.  In those cases where it is passed
Kevin> a dynamically allocated string, I see no provision for freeing the
Kevin> section name.  I strongly suspect that there are leaks where this
Kevin> occurs.

bfd_alloc allocates on the BFD's objalloc (which is like an obstack, but
"optimized").  So, it's a leak in the "lingerer" sense (memory allocated
that isn't useful), but not in the ordinary sense, because it will be
freed when the BFD is closed.

Presumably if we had ASAN and/or valgrind annotations for obstack and
objalloc, this would show up as a real leak.  But we don't, so it won't.

I didn't look through BFD to see how section names might be allocated,
but if bfd_alloc is used, then it's fine.

Tom