[PATCH v2 0/5] Fix BZ 25631 - core file memory access problem

[PATCH v2 0/5] Fix BZ 25631 - core file memory access problem

[Kevin Buettner]

This series fixes a bug with accessing memory from core files.

The bug can be viewed here...

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

...though I also provide similar details in part 4 of this series.

It incorporates suggestions made by Keith and Pedro in their reviews.
The biggest change from v1 is that the section splitting code that
I introduced in v1 has been entirely removed.  Instead, I now pass
a predicate to section_table_xfer_memory as suggested by Pedro.

Pedro has asked me to write another test case for a problem
that he found while reviewing v1.  I will attempt to do that, but I
wanted to post this patch series so that review can start sooner.
If I manage to create a good test, I'll post that separately.

Kevin Buettner (5):
  Remove hack for GDB which sets the section size to 0
  Adjust corefile.exp test to show regression after bfd hack removal
  section_table_xfer_memory: Replace section name with callback
    predicate
  Provide access to non SEC_HAS_CONTENTS core file sections
  Test ability to access unwritten-to mmap data in core file

 bfd/elf.c                           |  8 -----
 gdb/bfd-target.c                    |  3 +-
 gdb/corelow.c                       | 48 ++++++++++++++++++++++++-----
 gdb/exec.c                          |  8 ++---
 gdb/exec.h                          | 13 ++++++--
 gdb/target.c                        | 18 ++++++++---
 gdb/testsuite/gdb.base/corefile.exp |  6 ++++
 gdb/testsuite/gdb.base/coremaker.c  | 16 ++++++++++
 8 files changed, 91 insertions(+), 29 deletions(-)

-- 
2.25.4

[PATCH v2 1/5] Remove hack for GDB which sets the section size to 0

[Kevin Buettner]

[Note: This patch has been approved by Nick Clifton.]

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 e9c525974b..f54d693b09 100644
--- a/bfd/elf.c
+++ b/bfd/elf.c
@@ -3025,14 +3025,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.25.4

[PATCH v2 2/5] 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 patch

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 an 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 55330fd3e8..4c41b9c926 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.25.4

[PATCH v2 3/5] 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 b60010453d..e3bd0bc452 100644
--- a/gdb/corelow.c
+++ b/gdb/corelow.c
@@ -617,8 +617,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 a2added5e2..02dd37ed10 100644
--- a/gdb/exec.c
+++ b/gdb/exec.c
@@ -908,7 +908,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;
@@ -922,7 +923,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 (p))
 	continue;		/* not the section we need.  */
       if (memaddr >= p->addr)
         {
@@ -995,8 +996,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..7057b1be5a 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
+				         = [] (auto *) { return true; } );
 
 /* 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 6982a806e3..b83da21710 100644
--- a/gdb/target.c
+++ b/gdb/target.c
@@ -1036,11 +1036,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);
 	}
     }
 
@@ -1058,8 +1064,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.25.4

[PATCH v2 4/5] 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 e3bd0bc452..17e862aee9 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;
@@ -613,12 +613,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 b83da21710..e74871e262 100644
--- a/gdb/target.c
+++ b/gdb/target.c
@@ -981,8 +981,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.25.4

[PATCH v2 5/5] 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_ANONYMOUSE 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 4c41b9c926..657764ca6b 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.25.4

Re: [PATCH v2 2/5] Adjust corefile.exp test to show regression after bfd hack removal

[Pedro Alves]

On 5/13/20 6:11 PM, Kevin Buettner via Gdb-patches wrote:
> 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.

Thanks, I confirm that I can see the regression with this.

Pedro Alves

Re: [PATCH v2 3/5] section_table_xfer_memory: Replace section name with callback predicate

[Pedro Alves]

On 5/13/20 6:11 PM, Kevin Buettner via Gdb-patches wrote:
> 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 b60010453d..e3bd0bc452 100644
> --- a/gdb/corelow.c
> +++ b/gdb/corelow.c
> @@ -617,8 +617,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 a2added5e2..02dd37ed10 100644
> --- a/gdb/exec.c
> +++ b/gdb/exec.c
> @@ -908,7 +908,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;
> @@ -922,7 +923,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 (p))
>  	continue;		/* not the section we need.  */
>        if (memaddr >= p->addr)
>          {
> @@ -995,8 +996,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..7057b1be5a 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
> +				         = [] (auto *) { return true; } );

The '(' after bool should go on the next line.  I think a default of nullptr,
along with the obvious change in section_table_xfer_memory_partial
is better than this default lambda.  Like:

				     gdb::function_view<bool 
                                       (const struct target_section *)> match_cb
                                          = nullptr);

This allows writing code where you can explicitly write "no filter"
just using the convenient nullptr, like:

gdb::function_view<bool (const struct target_section *)> 
  match_cb = nullptr;

if (whatever)
  match_cb = [] (....) { .......; };

section_table_xfer_memory_partial (...., match_cb);

Plus, I suspect is generates less code, not sure the linker
merges all the instances of the lambda generated at each
call site that uses the default.

>  
>  /* 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 6982a806e3..b83da21710 100644
> --- a/gdb/target.c
> +++ b/gdb/target.c
> @@ -1036,11 +1036,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)

[=] is pedantically cheaper, since you're just copying a pointer,
compared to [&] resulting in a pointer (reference) to pointer.

> +	    {
> +	      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);
>  	}
>      }
>  
OK otherwise.

Thanks,
Pedro Alves

Re: [PATCH v2 4/5] Provide access to non SEC_HAS_CONTENTS core file sections

[Pedro Alves]

On 5/13/20 6:11 PM, Kevin Buettner via Gdb-patches wrote:
> 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."

So, as discussed at <https://sourceware.org/pipermail/gdb-patches/2020-May/168826.html>,
I still think that this is an heuristic, and that we're trading one
bug for another.  It does seem like the bug that we're fixing
is more important than the one we're introducing, so if there's really
nothing else in the core file that would distinguish the coremaker_ro
case from an anonymous mapping that should be read from the core (i.e.,
read as zeroes), then I suppose this is the best we can do, and the
patch looks good to me.  I would like to see the commit log and the
comments slightly adjusted to mention this, though.

Could you remind me again why is it that the core dump includes a
load segment for coremaker_ro in the first place?  If this is
read-only data, why did the kernel decide to output a bss-like
load segment for it?

Thanks,
Pedro Alves

Re: [PATCH v2 5/5] Test ability to access unwritten-to mmap data in core file

[Pedro Alves]

On 5/13/20 6:11 PM, Kevin Buettner via Gdb-patches wrote:
> 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_ANONYMOUSE and MAP_PRIVATE
> 	flags.

OK.

Thanks,
Pedro Alves

Re: [PATCH v2 4/5] Provide access to non SEC_HAS_CONTENTS core file sections

[Kevin Buettner]

On Wed, 20 May 2020 17:45:28 +0100
Pedro Alves <palves@redhat.com> wrote:

> So, as discussed at <https://sourceware.org/pipermail/gdb-patches/2020-May/168826.html>,
> I still think that this is an heuristic, and that we're trading one
> bug for another.  It does seem like the bug that we're fixing
> is more important than the one we're introducing, so if there's really
> nothing else in the core file that would distinguish the coremaker_ro
> case from an anonymous mapping that should be read from the core (i.e.,
> read as zeroes), then I suppose this is the best we can do, and the
> patch looks good to me.  I would like to see the commit log and the
> comments slightly adjusted to mention this, though.

On Linux, we're able to do "info proc mappings" when looking at a
corefile.  Linux core files include a .note.linuxcore.file/pid section
which contain the map data that you see when running "info proc
mappings".

It may be possible to use these mappings to provide a more accurate
view of memory contents when working with a core file.  In addition
to file backed data that we already know about from the executable and
shared libraries, there may be additional file backed data that can
be found via files listed in the .note section mentioned earlier.  Or,
as I found while investigating that one of those weird F27 mappings,
there may be data from a shared library which can't be found in
what GDB knows about when loading the shared library.  (In that
particular case, the memory region in question contained some
vtable data prior to being remapped - though I'm guessing about
the remapping bit.)

If you think it worth pursuing, I can look at using the core-provided
proc mapping information to provide a more accurate view of memory
when looking at a core dump.

If I do this, is it still worth pushing this v2 series first?  Or should
it wait until I attempt to incorporate info from the .note section?

> Could you remind me again why is it that the core dump includes a
> load segment for coremaker_ro in the first place?  If this is
> read-only data, why did the kernel decide to output a bss-like
> load segment for it?

On F27 and F28, coremaker_ro was placed on a page that also had
read/write data; it got included in with that data when the core file
was written.

On F29 and beyond (due to use of particular bintutils verions/options
which participated in making the executable), coremaker_ro was placed
on a page with only read-only data.  In this case, the core file
didn't include data for that page; it lets the debugger find that data
from the executable.

Kevin

Re: [PATCH v2 4/5] Provide access to non SEC_HAS_CONTENTS core file sections

[Pedro Alves]

On 5/21/20 8:50 AM, Kevin Buettner wrote:
> On Wed, 20 May 2020 17:45:28 +0100
> Pedro Alves <palves@redhat.com> wrote:
> 
>> So, as discussed at <https://sourceware.org/pipermail/gdb-patches/2020-May/168826.html>,
>> I still think that this is an heuristic, and that we're trading one
>> bug for another.  It does seem like the bug that we're fixing
>> is more important than the one we're introducing, so if there's really
>> nothing else in the core file that would distinguish the coremaker_ro
>> case from an anonymous mapping that should be read from the core (i.e.,
>> read as zeroes), then I suppose this is the best we can do, and the
>> patch looks good to me.  I would like to see the commit log and the
>> comments slightly adjusted to mention this, though.
> 
> On Linux, we're able to do "info proc mappings" when looking at a
> corefile.  Linux core files include a .note.linuxcore.file/pid section
> which contain the map data that you see when running "info proc
> mappings".
> 
> It may be possible to use these mappings to provide a more accurate
> view of memory contents when working with a core file.  In addition
> to file backed data that we already know about from the executable and
> shared libraries, there may be additional file backed data that can
> be found via files listed in the .note section mentioned earlier.  Or,
> as I found while investigating that one of those weird F27 mappings,
> there may be data from a shared library which can't be found in
> what GDB knows about when loading the shared library.  (In that
> particular case, the memory region in question contained some
> vtable data prior to being remapped - though I'm guessing about
> the remapping bit.)
> 
> If you think it worth pursuing, I can look at using the core-provided
> proc mapping information to provide a more accurate view of memory
> when looking at a core dump.
> 
> If I do this, is it still worth pushing this v2 series first?  Or should
> it wait until I attempt to incorporate info from the .note section?
> 
>> Could you remind me again why is it that the core dump includes a
>> load segment for coremaker_ro in the first place?  If this is
>> read-only data, why did the kernel decide to output a bss-like
>> load segment for it?
> 
> On F27 and F28, coremaker_ro was placed on a page that also had
> read/write data; it got included in with that data when the core file
> was written.
> 

Ah, right.  In this case, reading from the core should always work.

> On F29 and beyond (due to use of particular bintutils verions/options
> which participated in making the executable), coremaker_ro was placed
> on a page with only read-only data.  In this case, the core file
> didn't include data for that page; it lets the debugger find that data
> from the executable.

OK.  

I think I asked the wrong question.  Let's go back to what you described
in the commit log:

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

The question should have been -- why is there a !SEC_HAS_CONTENTS load segment
for the page that contains coremaker_ro in the core file, at all?  
Or rather, why did the kernel decide to output a .bss load segment with
no contents for that page?  If there wasn't one, then we wouldn't need this
heuristic.  This is looking like a kernel bug to me.  Like, if the mapping
was file backed and wasn't touched, then it should have been skipped.
If it was touched (or for some other reason that could justify dumping the
mapping), then the kernel should have included its current contents
in the dump, instead of indicating "no contents".  No?

Thanks,
Pedro Alves

Re: [PATCH v2 4/5] Provide access to non SEC_HAS_CONTENTS core file sections

[Pedro Alves]

On 5/21/20 1:40 PM, Pedro Alves via Gdb-patches wrote:
> On 5/21/20 8:50 AM, Kevin Buettner wrote:
>> On Wed, 20 May 2020 17:45:28 +0100
>> Pedro Alves <palves@redhat.com> wrote:
>>
>>> So, as discussed at <https://sourceware.org/pipermail/gdb-patches/2020-May/168826.html>,
>>> I still think that this is an heuristic, and that we're trading one
>>> bug for another.  It does seem like the bug that we're fixing
>>> is more important than the one we're introducing, so if there's really
>>> nothing else in the core file that would distinguish the coremaker_ro
>>> case from an anonymous mapping that should be read from the core (i.e.,
>>> read as zeroes), then I suppose this is the best we can do, and the
>>> patch looks good to me.  I would like to see the commit log and the
>>> comments slightly adjusted to mention this, though.
>>
>> On Linux, we're able to do "info proc mappings" when looking at a
>> corefile.  Linux core files include a .note.linuxcore.file/pid section
>> which contain the map data that you see when running "info proc
>> mappings".
>>
>> It may be possible to use these mappings to provide a more accurate
>> view of memory contents when working with a core file.  In addition
>> to file backed data that we already know about from the executable and
>> shared libraries, there may be additional file backed data that can
>> be found via files listed in the .note section mentioned earlier.  Or,
>> as I found while investigating that one of those weird F27 mappings,
>> there may be data from a shared library which can't be found in
>> what GDB knows about when loading the shared library.  (In that
>> particular case, the memory region in question contained some
>> vtable data prior to being remapped - though I'm guessing about
>> the remapping bit.)
>>
>> If you think it worth pursuing, I can look at using the core-provided
>> proc mapping information to provide a more accurate view of memory
>> when looking at a core dump.
>>
>> If I do this, is it still worth pushing this v2 series first?  Or should
>> it wait until I attempt to incorporate info from the .note section?
>>
>>> Could you remind me again why is it that the core dump includes a
>>> load segment for coremaker_ro in the first place?  If this is
>>> read-only data, why did the kernel decide to output a bss-like
>>> load segment for it?
>>
>> On F27 and F28, coremaker_ro was placed on a page that also had
>> read/write data; it got included in with that data when the core file
>> was written.
>>
> 
> Ah, right.  In this case, reading from the core should always work.
> 
>> On F29 and beyond (due to use of particular bintutils verions/options
>> which participated in making the executable), coremaker_ro was placed
>> on a page with only read-only data.  In this case, the core file
>> didn't include data for that page; it lets the debugger find that data
>> from the executable.
> 
> OK.  
> 
> I think I asked the wrong question.  Let's go back to what you described
> in the commit log:
> 
>>    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.
> 
> The question should have been -- why is there a !SEC_HAS_CONTENTS load segment
> for the page that contains coremaker_ro in the core file, at all?  
> Or rather, why did the kernel decide to output a .bss load segment with
> no contents for that page?  If there wasn't one, then we wouldn't need this
> heuristic.  This is looking like a kernel bug to me.  Like, if the mapping
> was file backed and wasn't touched, then it should have been skipped.
> If it was touched (or for some other reason that could justify dumping the
> mapping), then the kernel should have included its current contents
> in the dump, instead of indicating "no contents".  No?

Blah, sorry, got distracted here while I write the above, and I didn't realize
I still asked the wrong question.  I don't know why I talked
about coremaker_ro above.  I meant to talk about those "Some of these sections
have data that can (and should) be read from the executable.".  Why do we have
segments for those in the core file?  Hopefully the rest of the
comments make more sense now.

Thus, take 3:

The question should have been -- why are there SEC_ALLOC && !SEC_HAS_CONTENTS
load segments for that data that should be read from the executable?
Or rather, why did the kernel decide to output a .bss-like load segment with
no contents for that mapping?  If there wasn't such a load segment
in the core, then we wouldn't need this heuristic.  This is looking like a kernel
bug to me.  Like, if the mapping was file backed and wasn't touched, then it
should have been skipped.  If it was touched (or for some other reason
that could justify dumping the mapping), then the kernel should have
included its current contents in the dump, instead of
indicating "no contents".  No?

Thanks,
Pedro Alves

Re: [PATCH v2 4/5] Provide access to non SEC_HAS_CONTENTS core file sections

[Kevin Buettner]

On Thu, 21 May 2020 15:23:00 +0100
Pedro Alves <palves@redhat.com> wrote:

> On 5/21/20 1:40 PM, Pedro Alves via Gdb-patches wrote:
> > On 5/21/20 8:50 AM, Kevin Buettner wrote:  
> >> On Wed, 20 May 2020 17:45:28 +0100
> >> Pedro Alves <palves@redhat.com> wrote:
> >>  
> >>> So, as discussed at <https://sourceware.org/pipermail/gdb-patches/2020-May/168826.html>,
> >>> I still think that this is an heuristic, and that we're trading one
> >>> bug for another.  It does seem like the bug that we're fixing
> >>> is more important than the one we're introducing, so if there's really
> >>> nothing else in the core file that would distinguish the coremaker_ro
> >>> case from an anonymous mapping that should be read from the core (i.e.,
> >>> read as zeroes), then I suppose this is the best we can do, and the
> >>> patch looks good to me.  I would like to see the commit log and the
> >>> comments slightly adjusted to mention this, though.  
> >>
> >> On Linux, we're able to do "info proc mappings" when looking at a
> >> corefile.  Linux core files include a .note.linuxcore.file/pid section
> >> which contain the map data that you see when running "info proc
> >> mappings".
> >>
> >> It may be possible to use these mappings to provide a more accurate
> >> view of memory contents when working with a core file.  In addition
> >> to file backed data that we already know about from the executable and
> >> shared libraries, there may be additional file backed data that can
> >> be found via files listed in the .note section mentioned earlier.  Or,
> >> as I found while investigating that one of those weird F27 mappings,
> >> there may be data from a shared library which can't be found in
> >> what GDB knows about when loading the shared library.  (In that
> >> particular case, the memory region in question contained some
> >> vtable data prior to being remapped - though I'm guessing about
> >> the remapping bit.)
> >>
> >> If you think it worth pursuing, I can look at using the core-provided
> >> proc mapping information to provide a more accurate view of memory
> >> when looking at a core dump.
> >>
> >> If I do this, is it still worth pushing this v2 series first?  Or should
> >> it wait until I attempt to incorporate info from the .note section?
> >>  
> >>> Could you remind me again why is it that the core dump includes a
> >>> load segment for coremaker_ro in the first place?  If this is
> >>> read-only data, why did the kernel decide to output a bss-like
> >>> load segment for it?  
> >>
> >> On F27 and F28, coremaker_ro was placed on a page that also had
> >> read/write data; it got included in with that data when the core file
> >> was written.
> >>  
> > 
> > Ah, right.  In this case, reading from the core should always work.
> >   
> >> On F29 and beyond (due to use of particular bintutils verions/options
> >> which participated in making the executable), coremaker_ro was placed
> >> on a page with only read-only data.  In this case, the core file
> >> didn't include data for that page; it lets the debugger find that data
> >> from the executable.  
> > 
> > OK.  
> > 
> > I think I asked the wrong question.  Let's go back to what you described
> > in the commit log:
> >   
> >>    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.  
> > 
> > The question should have been -- why is there a !SEC_HAS_CONTENTS load segment
> > for the page that contains coremaker_ro in the core file, at all?  
> > Or rather, why did the kernel decide to output a .bss load segment with
> > no contents for that page?  If there wasn't one, then we wouldn't need this
> > heuristic.  This is looking like a kernel bug to me.  Like, if the mapping
> > was file backed and wasn't touched, then it should have been skipped.
> > If it was touched (or for some other reason that could justify dumping the
> > mapping), then the kernel should have included its current contents
> > in the dump, instead of indicating "no contents".  No?  
> 
> Blah, sorry, got distracted here while I write the above, and I didn't realize
> I still asked the wrong question.  I don't know why I talked
> about coremaker_ro above.  I meant to talk about those "Some of these sections
> have data that can (and should) be read from the executable.".  Why do we have
> segments for those in the core file?  Hopefully the rest of the
> comments make more sense now.
> 
> Thus, take 3:
> 
> The question should have been -- why are there SEC_ALLOC && !SEC_HAS_CONTENTS
> load segments for that data that should be read from the executable?
> Or rather, why did the kernel decide to output a .bss-like load segment with
> no contents for that mapping?  If there wasn't such a load segment
> in the core, then we wouldn't need this heuristic.  This is looking like a kernel
> bug to me.  Like, if the mapping was file backed and wasn't touched, then it
> should have been skipped.  If it was touched (or for some other reason
> that could justify dumping the mapping), then the kernel should have
> included its current contents in the dump, instead of
> indicating "no contents".  No?

The kernel simply dumps all of the memory that it knows about.  It
doesn't try to filter anything out.  Should it determine that an area
has a file based backing or that it was never written to (or several other
conditions: MADV_DONTDUMP, I/O areas, etc), it'll skip dumping the
contents, but will still dump a header.

If you want to look for yourself, see elf_core_dump() in
fs/binfmt_elf.c in the kernel sources.  This is one of the loops that
iterate through the linked list of vm_area_struct structs for the
process:

	for (i = 0, vma = first_vma(current, gate_vma); vma != NULL;
			vma = next_vma(vma, gate_vma)) {
		unsigned long dump_size;

		dump_size = vma_dump_size(vma, cprm->mm_flags);
		vma_filesz[i++] = dump_size;
		vma_data_size += dump_size;
	}

vma_dump_size contains the logic which decides whether an area's
contents will be dumped, but beyond that there's no filtering going
on.

Actually, it's better to look at the second (of three) loops; this one
writes the headers...

	/* Write program headers for segments dump */
	for (i = 0, vma = first_vma(current, gate_vma); vma != NULL;
			vma = next_vma(vma, gate_vma)) {
		struct elf_phdr phdr;

		phdr.p_type = PT_LOAD;
		phdr.p_offset = offset;
		phdr.p_vaddr = vma->vm_start;
		phdr.p_paddr = 0;
		phdr.p_filesz = vma_filesz[i++];
		phdr.p_memsz = vma->vm_end - vma->vm_start;
		offset += phdr.p_filesz;
		phdr.p_flags = vma->vm_flags & VM_READ ? PF_R : 0;
		if (vma->vm_flags & VM_WRITE)
			phdr.p_flags |= PF_W;
		if (vma->vm_flags & VM_EXEC)
			phdr.p_flags |= PF_X;
		phdr.p_align = ELF_EXEC_PAGESIZE;

		if (!dump_emit(cprm, &phdr, sizeof(phdr)))
			goto end_coredump;
	}

Kevin

Re: [PATCH v2 4/5] Provide access to non SEC_HAS_CONTENTS core file sections

[Pedro Alves]

On 5/21/20 4:09 PM, Kevin Buettner wrote:
> On Thu, 21 May 2020 15:23:00 +0100
> Pedro Alves <palves@redhat.com> wrote:
>>
>> Thus, take 3:
>>
>> The question should have been -- why are there SEC_ALLOC && !SEC_HAS_CONTENTS
>> load segments for that data that should be read from the executable?
>> Or rather, why did the kernel decide to output a .bss-like load segment with
>> no contents for that mapping?  If there wasn't such a load segment
>> in the core, then we wouldn't need this heuristic.  This is looking like a kernel
>> bug to me.  Like, if the mapping was file backed and wasn't touched, then it
>> should have been skipped.  If it was touched (or for some other reason
>> that could justify dumping the mapping), then the kernel should have
>> included its current contents in the dump, instead of
>> indicating "no contents".  No?
> 
> The kernel simply dumps all of the memory that it knows about.  It
> doesn't try to filter anything out.  Should it determine that an area
> has a file based backing or that it was never written to (or several other
> conditions: MADV_DONTDUMP, I/O areas, etc), it'll skip dumping the
> contents, but will still dump a header.

I see.  I thought I'd check what other OSs do, so I found a Solaris 11
box, and did a bit of testing with a simple 

  #include <stdlib.h>
  int main { abort (); }

program.

Solaris has this coreadm utility that lets you configure what kind of
data is dumped on the core:
  https://docs.oracle.com/cd/E19253-01/816-5166/coreadm-1m/index.html

Comparing dumps with
  $ coreadm -I none
vs 
  $ coreadm -I default

I see that Solaris dumps the exact same set of segments in both
cases, though in the "none" case, segments whose contents were not
dumped have headers with !CONTENTS.  Like:

 -  6 load2         00000000  0000000000400000  0000000000000000  00000000  2**0
 -                  ALLOC, READONLY, CODE
 +  6 load2         00001000  0000000000400000  0000000000000000  00003068  2**0
 +                  CONTENTS, ALLOC, LOAD, READONLY, CODE

So seems like this isn't a Linux-specific thing...

I think this means that we're probably going to be stuck with this
basically forever, and should do the best we can with the info we have.

Let me answer the other question about the info proc mappings idea in
response to your previous email.

Thanks,
Pedro Alves

Re: [PATCH v2 4/5] Provide access to non SEC_HAS_CONTENTS core file sections

[Pedro Alves]

On 5/21/20 8:50 AM, Kevin Buettner via Gdb-patches wrote:
> On Wed, 20 May 2020 17:45:28 +0100
> Pedro Alves <palves@redhat.com> wrote:
> On Linux, we're able to do "info proc mappings" when looking at a
> corefile.  Linux core files include a .note.linuxcore.file/pid section
> which contain the map data that you see when running "info proc
> mappings".
> 
> It may be possible to use these mappings to provide a more accurate
> view of memory contents when working with a core file.  In addition
> to file backed data that we already know about from the executable and
> shared libraries, there may be additional file backed data that can
> be found via files listed in the .note section mentioned earlier.  Or,
> as I found while investigating that one of those weird F27 mappings,
> there may be data from a shared library which can't be found in
> what GDB knows about when loading the shared library.  (In that
> particular case, the memory region in question contained some
> vtable data prior to being remapped - though I'm guessing about
> the remapping bit.)

Yeah, that's a good idea.

> 
> If you think it worth pursuing, I can look at using the core-provided
> proc mapping information to provide a more accurate view of memory
> when looking at a core dump.

I think it's worth pursuing.  If we have proc mappings data, we could
only read from the exec file first if the mapping is file-backed.
That sounds like would plug the ambiguity.

> 
> If I do this, is it still worth pushing this v2 series first?  Or should
> it wait until I attempt to incorporate info from the .note section?

I'd say give it a try, see if it would be too complicated.
If it isn't, then I think it would be nice to merge it all
together.

Thanks,
Pedro Alves