Work around incorrect/broken pathnames in NT_FILE note

Message ID 20200807222044.2252664-1-kevinb@redhat.com
State New
Headers show
Series
  • Work around incorrect/broken pathnames in NT_FILE note
Related show

Commit Message

Hannes Domani via Gdb-patches Aug. 7, 2020, 10:20 p.m.
Luis Machado reported some regressions after I pushed recent core file
related patches fixing BZ 25631:

    FAIL: gdb.base/corefile.exp: backtrace in corefile.exp
    FAIL: gdb.base/corefile.exp: core-file warning-free
    FAIL: gdb.base/corefile.exp: print func2::coremaker_local
    FAIL: gdb.base/corefile.exp: up in corefile.exp
    FAIL: gdb.base/corefile.exp: up in corefile.exp (reinit)

While I haven't been able to reproduce these failures, I think I
understand why they're happening.  It is my hope that this commit will
fix those regressions.

Luis is testing in a docker container which is using the AUFS storage
driver.  It turns out that the kernel is placing docker host paths in
the NT_FILE note instead of paths within the container.

I've made a similar docker environment (though apparently not similar
enough to reproduce the regressions).  This is one of the paths that
I see mentioned in the warning messages printed while loading the
core file during NT_FILE note processing - note that I've shortened
the path component starting with "d07c4":

/var/lib/docker/aufs/diff/d07c4...21/lib/x86_64-linux-gnu/ld-2.27.so

This is a path on the docker host; it does not exist in the
container.  In the docker container, this is the path:

/lib/x86_64-linux-gnu/ld-2.27.so

My first thought was to disable all NT_FILE mappings when any path was
found to be bad.  This would have caused GDB to fall back to accessing
memory using the file stratum as it did before I added the NT_FILE
note loading code.  After further consideration, I realized that we
could do better than this.  For file-backed memory access, we can
still use the NT_FILE mappings when available, and then attempt to
access memory using the file stratum constrainted to those address
ranges corresponding to the "broken" mappings.

In order to test it, I made some additions to corefile2.exp in which
the test case's executable is renamed.  The core file is then loaded;
due to the fact that the executable has been renamed, those mappings
will be unavailable.  After loading the core file, the executable is
renamed back to its original name at which point it is loaded using
GDB's "file" command.  The "interesting" tests are then run.  These
tests will print out values in file-backed memory regions along with
mmap'd regions placed within/over the file-backed regions.  Despite
the fact that the executable could not be found during the NT_FILE
note processing, these tests still work correctly due to the fact that
memory is availble from the file stratum combined with the fact that
the broken NT_FILE mappings are used to prevent file-backed access
outside of the "broken" mappings.

gdb/ChangeLog:

	* corelow.c (class core_target): Add field
	'm_core_unavailable_mappings'.
	(core_target::build_file_mappings): Print only one warning
	per inaccessible file.  Add unavailable/broken mappings
	to m_core_unavailable_mappings.
	(core_target::xfer_partial): Call...
	(core_target::xfer_memory_via_mappings): New method.

gdb/testsuite/ChangeLog:

	* gdb.base/corefile2.exp (renamed binfile): New tests.
---
 gdb/corelow.c                        | 84 +++++++++++++++++++++++++---
 gdb/testsuite/gdb.base/corefile2.exp | 35 ++++++++++++
 2 files changed, 111 insertions(+), 8 deletions(-)

-- 
2.26.2

Comments

Hannes Domani via Gdb-patches Aug. 8, 2020, 10:19 a.m. | #1
Hi Kevin,

Thanks for working on this. I gave this a try and the failures below are 
gone, but there is a new one that showed up for gdb.base/corefile.exp:

FAIL: gdb.base/corefile.exp: core-file warning-free

I'll take a look at it later.

gdb.base/corefile2.exp came out clean though.

On 8/7/20 7:20 PM, Kevin Buettner wrote:
> Luis Machado reported some regressions after I pushed recent core file

> related patches fixing BZ 25631:

> 

>      FAIL: gdb.base/corefile.exp: backtrace in corefile.exp

>      FAIL: gdb.base/corefile.exp: core-file warning-free

>      FAIL: gdb.base/corefile.exp: print func2::coremaker_local

>      FAIL: gdb.base/corefile.exp: up in corefile.exp

>      FAIL: gdb.base/corefile.exp: up in corefile.exp (reinit)

> 

> While I haven't been able to reproduce these failures, I think I

> understand why they're happening.  It is my hope that this commit will

> fix those regressions.

> 

> Luis is testing in a docker container which is using the AUFS storage

> driver.  It turns out that the kernel is placing docker host paths in

> the NT_FILE note instead of paths within the container.

> 

> I've made a similar docker environment (though apparently not similar

> enough to reproduce the regressions).  This is one of the paths that

> I see mentioned in the warning messages printed while loading the

> core file during NT_FILE note processing - note that I've shortened

> the path component starting with "d07c4":

> 

> /var/lib/docker/aufs/diff/d07c4...21/lib/x86_64-linux-gnu/ld-2.27.so

> 

> This is a path on the docker host; it does not exist in the

> container.  In the docker container, this is the path:

> 

> /lib/x86_64-linux-gnu/ld-2.27.so

> 

> My first thought was to disable all NT_FILE mappings when any path was

> found to be bad.  This would have caused GDB to fall back to accessing

> memory using the file stratum as it did before I added the NT_FILE

> note loading code.  After further consideration, I realized that we

> could do better than this.  For file-backed memory access, we can

> still use the NT_FILE mappings when available, and then attempt to

> access memory using the file stratum constrainted to those address

> ranges corresponding to the "broken" mappings.

> 

> In order to test it, I made some additions to corefile2.exp in which

> the test case's executable is renamed.  The core file is then loaded;

> due to the fact that the executable has been renamed, those mappings

> will be unavailable.  After loading the core file, the executable is

> renamed back to its original name at which point it is loaded using

> GDB's "file" command.  The "interesting" tests are then run.  These

> tests will print out values in file-backed memory regions along with

> mmap'd regions placed within/over the file-backed regions.  Despite

> the fact that the executable could not be found during the NT_FILE

> note processing, these tests still work correctly due to the fact that

> memory is availble from the file stratum combined with the fact that

> the broken NT_FILE mappings are used to prevent file-backed access

> outside of the "broken" mappings.

> 

> gdb/ChangeLog:

> 

> 	* corelow.c (class core_target): Add field

> 	'm_core_unavailable_mappings'.

> 	(core_target::build_file_mappings): Print only one warning

> 	per inaccessible file.  Add unavailable/broken mappings

> 	to m_core_unavailable_mappings.

> 	(core_target::xfer_partial): Call...

> 	(core_target::xfer_memory_via_mappings): New method.

> 

> gdb/testsuite/ChangeLog:

> 

> 	* gdb.base/corefile2.exp (renamed binfile): New tests.

> ---

>   gdb/corelow.c                        | 84 +++++++++++++++++++++++++---

>   gdb/testsuite/gdb.base/corefile2.exp | 35 ++++++++++++

>   2 files changed, 111 insertions(+), 8 deletions(-)

> 

> diff --git a/gdb/corelow.c b/gdb/corelow.c

> index b6ee219f57..2e6be74a24 100644

> --- a/gdb/corelow.c

> +++ b/gdb/corelow.c

> @@ -131,9 +131,21 @@ class core_target final : public process_stratum_target

>        information about memory mapped files.  */

>     target_section_table m_core_file_mappings {};

>   

> +  /* Unavailable mappings.  These correspond to pathnames which either

> +     weren't found or could not be opened.  Knowing these addresses can

> +     still be useful.  */

> +  std::vector<mem_range> m_core_unavailable_mappings;

> +

>     /* Build m_core_file_mappings.  Called from the constructor.  */

>     void build_file_mappings ();

>   

> +  /* Helper method for xfer_partial.  */

> +  enum target_xfer_status xfer_memory_via_mappings (gdb_byte *readbuf,

> +                                                    const gdb_byte *writebuf,

> +						    ULONGEST offset,

> +						    ULONGEST len,

> +						    ULONGEST *xfered_len);

> +

>     /* FIXME: kettenis/20031023: Eventually this field should

>        disappear.  */

>     struct gdbarch *m_core_gdbarch = NULL;

> @@ -182,6 +194,7 @@ void

>   core_target::build_file_mappings ()

>   {

>     std::unordered_map<std::string, struct bfd *> bfd_map;

> +  std::unordered_map<std::string, bool> unavailable_paths;

>   

>     /* See linux_read_core_file_mappings() in linux-tdep.c for an example

>        read_core_file_mappings method.  */

> @@ -216,9 +229,13 @@ core_target::build_file_mappings ()

>   	      = exec_file_find (filename, NULL);

>   	    if (expanded_fname == nullptr)

>   	      {

> -		warning (_("Can't open file %s during file-backed mapping "

> -			   "note processing"),

> -			 filename);

> +		m_core_unavailable_mappings.emplace_back (start, end - start);

> +		if (!unavailable_paths[filename])

> +		  warning (_("Can't open file %s during file-backed mapping "

> +			     "note processing"),

> +			   filename);

> +		/* Print just one warning per path.  */

> +		unavailable_paths[filename] = true;

>   		return;

>   	      }

>   

> @@ -227,6 +244,7 @@ core_target::build_file_mappings ()

>   

>   	    if (bfd == nullptr || !bfd_check_format (bfd, bfd_object))

>   	      {

> +		m_core_unavailable_mappings.emplace_back (start, end - start);

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

> @@ -268,6 +286,8 @@ core_target::build_file_mappings ()

>   	ts->owner = nullptr;

>   	ts->the_bfd_section = sec;

>         });

> +

> +  normalize_mem_ranges (&m_core_unavailable_mappings);

>   }

>   

>   static void add_to_thread_list (bfd *, asection *, void *);

> @@ -728,6 +748,57 @@ core_target::files_info ()

>     print_section_info (&m_core_section_table, core_bfd);

>   }

>   

> +/* Helper method for core_target::xfer_partial.  */

> +

> +enum target_xfer_status

> +core_target::xfer_memory_via_mappings (gdb_byte *readbuf,

> +				       const gdb_byte *writebuf,

> +				       ULONGEST offset, ULONGEST len,

> +				       ULONGEST *xfered_len)

> +{

> +  enum target_xfer_status xfer_status;

> +

> +  xfer_status = section_table_xfer_memory_partial

> +		  (readbuf, writebuf,

> +		   offset, len, xfered_len,

> +		   m_core_file_mappings.sections,

> +		   m_core_file_mappings.sections_end);

> +

> +  if (xfer_status == TARGET_XFER_OK || m_core_unavailable_mappings.empty ())

> +    return xfer_status;

> +

> +  /* There are instances - e.g. when debugging within a docker

> +     container using the AUFS storage driver - where the pathnames

> +     obtained from the note section are incorrect.  Despite the path

> +     being wrong, just knowing the start and end addresses of the

> +     mappings is still useful; we can attempt an access of the file

> +     stratum constrained to the address ranges corresponding to the

> +     unavailable mappings.  */

> +

> +  ULONGEST memaddr = offset;

> +  ULONGEST memend = offset + len;

> +

> +  for (const auto &mr : m_core_unavailable_mappings)

> +    {

> +      if (address_in_mem_range (memaddr, &mr))

> +        {

> +	  if (!address_in_mem_range (memend, &mr))

> +	    len = mr.start + mr.length - memaddr;

> +

> +	  xfer_status = this->beneath ()->xfer_partial (TARGET_OBJECT_MEMORY,

> +							NULL,

> +							readbuf,

> +							writebuf,

> +							offset,

> +							len,

> +							xfered_len);

> +	  break;

> +	}

> +    }

> +

> +  return xfer_status;

> +}

> +

>   enum target_xfer_status

>   core_target::xfer_partial (enum target_object object, const char *annex,

>   			   gdb_byte *readbuf, const gdb_byte *writebuf,

> @@ -761,11 +832,8 @@ core_target::xfer_partial (enum target_object object, const char *annex,

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

> +	  xfer_status = xfer_memory_via_mappings (readbuf, writebuf, offset,

> +						  len, xfered_len);

>   	else

>   	  xfer_status = this->beneath ()->xfer_partial (object, annex, readbuf,

>   							writebuf, offset, len,

> diff --git a/gdb/testsuite/gdb.base/corefile2.exp b/gdb/testsuite/gdb.base/corefile2.exp

> index 5de7ead4d4..11915a6185 100644

> --- a/gdb/testsuite/gdb.base/corefile2.exp

> +++ b/gdb/testsuite/gdb.base/corefile2.exp

> @@ -143,6 +143,41 @@ gdb_test_multiple $test "" {

>       }

>   }

>   

> +# Test again with executable renamed during loading of core file.

> +

> +with_test_prefix "renamed binfile" {

> +    # We don't use clean_restart here since we want to defer loading

> +    # of $binfile until after the core file has been loaded.  (BFD

> +    # will complain that $binfile has disappeared after the rename

> +    # if it's loaded first.)

> +    gdb_exit

> +    gdb_start

> +    gdb_reinitialize_dir $srcdir/$subdir

> +

> +    # Rename $binfile so that it won't be found during loading of

> +    # the core file.

> +    set hide_binfile [standard_output_file "${testfile}.hide"]

> +    remote_exec host "mv -f $binfile $hide_binfile"

> +

> +    # Load core file - check that a warning is printed.

> +    global xfail

> +    if { $xfail } { setup_xfail "*-*-*" }

> +    gdb_test_multiple "core-file $corefile" $test {

> +	-re "warning: Can't open file.*during.* note processing.*Core was generated by .*\#0  .*\(\).*\r\n$gdb_prompt $" {

> +	    pass $test

> +	}

> +	-re "\r\n$gdb_prompt $" {

> +	    fail $test

> +	}

> +    }

> +

> +    # Restore $binfile and then load it.

> +    remote_exec host "mv -f $hide_binfile $binfile"

> +    gdb_load ${binfile}

> +

> +    do_tests

> +}

> +

>   # Restart and run to the abort call.

>   

>   clean_restart $binfile

>
Hannes Domani via Gdb-patches Aug. 9, 2020, 8:19 a.m. | #2
Hi Luis,

That new failure is expected in your docker environment.  It might
be possible to XFAIL it.  We need to be able to detect that we're
running in docker with the AUFS storage driver.  Since we know that
the kernel is leaking docker host paths, we should see these when the
"info proc mappings" command is used with a loaded core file.  I'll
give it a try...

Thanks for testing out my patch.

Kevin

On Sat, 8 Aug 2020 07:19:23 -0300
Luis Machado <luis.machado@linaro.org> wrote:

> Hi Kevin,

> 

> Thanks for working on this. I gave this a try and the failures below are 

> gone, but there is a new one that showed up for gdb.base/corefile.exp:

> 

> FAIL: gdb.base/corefile.exp: core-file warning-free

> 

> I'll take a look at it later.

> 

> gdb.base/corefile2.exp came out clean though.

> 

> On 8/7/20 7:20 PM, Kevin Buettner wrote:

> > Luis Machado reported some regressions after I pushed recent core file

> > related patches fixing BZ 25631:

> > 

> >      FAIL: gdb.base/corefile.exp: backtrace in corefile.exp

> >      FAIL: gdb.base/corefile.exp: core-file warning-free

> >      FAIL: gdb.base/corefile.exp: print func2::coremaker_local

> >      FAIL: gdb.base/corefile.exp: up in corefile.exp

> >      FAIL: gdb.base/corefile.exp: up in corefile.exp (reinit)

> > 

> > While I haven't been able to reproduce these failures, I think I

> > understand why they're happening.  It is my hope that this commit will

> > fix those regressions.

> > 

> > Luis is testing in a docker container which is using the AUFS storage

> > driver.  It turns out that the kernel is placing docker host paths in

> > the NT_FILE note instead of paths within the container.

> > 

> > I've made a similar docker environment (though apparently not similar

> > enough to reproduce the regressions).  This is one of the paths that

> > I see mentioned in the warning messages printed while loading the

> > core file during NT_FILE note processing - note that I've shortened

> > the path component starting with "d07c4":

> > 

> > /var/lib/docker/aufs/diff/d07c4...21/lib/x86_64-linux-gnu/ld-2.27.so

> > 

> > This is a path on the docker host; it does not exist in the

> > container.  In the docker container, this is the path:

> > 

> > /lib/x86_64-linux-gnu/ld-2.27.so

> > 

> > My first thought was to disable all NT_FILE mappings when any path was

> > found to be bad.  This would have caused GDB to fall back to accessing

> > memory using the file stratum as it did before I added the NT_FILE

> > note loading code.  After further consideration, I realized that we

> > could do better than this.  For file-backed memory access, we can

> > still use the NT_FILE mappings when available, and then attempt to

> > access memory using the file stratum constrainted to those address

> > ranges corresponding to the "broken" mappings.

> > 

> > In order to test it, I made some additions to corefile2.exp in which

> > the test case's executable is renamed.  The core file is then loaded;

> > due to the fact that the executable has been renamed, those mappings

> > will be unavailable.  After loading the core file, the executable is

> > renamed back to its original name at which point it is loaded using

> > GDB's "file" command.  The "interesting" tests are then run.  These

> > tests will print out values in file-backed memory regions along with

> > mmap'd regions placed within/over the file-backed regions.  Despite

> > the fact that the executable could not be found during the NT_FILE

> > note processing, these tests still work correctly due to the fact that

> > memory is availble from the file stratum combined with the fact that

> > the broken NT_FILE mappings are used to prevent file-backed access

> > outside of the "broken" mappings.

> > 

> > gdb/ChangeLog:

> > 

> > 	* corelow.c (class core_target): Add field

> > 	'm_core_unavailable_mappings'.

> > 	(core_target::build_file_mappings): Print only one warning

> > 	per inaccessible file.  Add unavailable/broken mappings

> > 	to m_core_unavailable_mappings.

> > 	(core_target::xfer_partial): Call...

> > 	(core_target::xfer_memory_via_mappings): New method.

> > 

> > gdb/testsuite/ChangeLog:

> > 

> > 	* gdb.base/corefile2.exp (renamed binfile): New tests.

> > ---

> >   gdb/corelow.c                        | 84 +++++++++++++++++++++++++---

> >   gdb/testsuite/gdb.base/corefile2.exp | 35 ++++++++++++

> >   2 files changed, 111 insertions(+), 8 deletions(-)

> > 

> > diff --git a/gdb/corelow.c b/gdb/corelow.c

> > index b6ee219f57..2e6be74a24 100644

> > --- a/gdb/corelow.c

> > +++ b/gdb/corelow.c

> > @@ -131,9 +131,21 @@ class core_target final : public process_stratum_target

> >        information about memory mapped files.  */

> >     target_section_table m_core_file_mappings {};

> >   

> > +  /* Unavailable mappings.  These correspond to pathnames which either

> > +     weren't found or could not be opened.  Knowing these addresses can

> > +     still be useful.  */

> > +  std::vector<mem_range> m_core_unavailable_mappings;

> > +

> >     /* Build m_core_file_mappings.  Called from the constructor.  */

> >     void build_file_mappings ();

> >   

> > +  /* Helper method for xfer_partial.  */

> > +  enum target_xfer_status xfer_memory_via_mappings (gdb_byte *readbuf,

> > +                                                    const gdb_byte *writebuf,

> > +						    ULONGEST offset,

> > +						    ULONGEST len,

> > +						    ULONGEST *xfered_len);

> > +

> >     /* FIXME: kettenis/20031023: Eventually this field should

> >        disappear.  */

> >     struct gdbarch *m_core_gdbarch = NULL;

> > @@ -182,6 +194,7 @@ void

> >   core_target::build_file_mappings ()

> >   {

> >     std::unordered_map<std::string, struct bfd *> bfd_map;

> > +  std::unordered_map<std::string, bool> unavailable_paths;

> >   

> >     /* See linux_read_core_file_mappings() in linux-tdep.c for an example

> >        read_core_file_mappings method.  */

> > @@ -216,9 +229,13 @@ core_target::build_file_mappings ()

> >   	      = exec_file_find (filename, NULL);

> >   	    if (expanded_fname == nullptr)

> >   	      {

> > -		warning (_("Can't open file %s during file-backed mapping "

> > -			   "note processing"),

> > -			 filename);

> > +		m_core_unavailable_mappings.emplace_back (start, end - start);

> > +		if (!unavailable_paths[filename])

> > +		  warning (_("Can't open file %s during file-backed mapping "

> > +			     "note processing"),

> > +			   filename);

> > +		/* Print just one warning per path.  */

> > +		unavailable_paths[filename] = true;

> >   		return;

> >   	      }

> >   

> > @@ -227,6 +244,7 @@ core_target::build_file_mappings ()

> >   

> >   	    if (bfd == nullptr || !bfd_check_format (bfd, bfd_object))

> >   	      {

> > +		m_core_unavailable_mappings.emplace_back (start, end - start);

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

> > @@ -268,6 +286,8 @@ core_target::build_file_mappings ()

> >   	ts->owner = nullptr;

> >   	ts->the_bfd_section = sec;

> >         });

> > +

> > +  normalize_mem_ranges (&m_core_unavailable_mappings);

> >   }

> >   

> >   static void add_to_thread_list (bfd *, asection *, void *);

> > @@ -728,6 +748,57 @@ core_target::files_info ()

> >     print_section_info (&m_core_section_table, core_bfd);

> >   }

> >   

> > +/* Helper method for core_target::xfer_partial.  */

> > +

> > +enum target_xfer_status

> > +core_target::xfer_memory_via_mappings (gdb_byte *readbuf,

> > +				       const gdb_byte *writebuf,

> > +				       ULONGEST offset, ULONGEST len,

> > +				       ULONGEST *xfered_len)

> > +{

> > +  enum target_xfer_status xfer_status;

> > +

> > +  xfer_status = section_table_xfer_memory_partial

> > +		  (readbuf, writebuf,

> > +		   offset, len, xfered_len,

> > +		   m_core_file_mappings.sections,

> > +		   m_core_file_mappings.sections_end);

> > +

> > +  if (xfer_status == TARGET_XFER_OK || m_core_unavailable_mappings.empty ())

> > +    return xfer_status;

> > +

> > +  /* There are instances - e.g. when debugging within a docker

> > +     container using the AUFS storage driver - where the pathnames

> > +     obtained from the note section are incorrect.  Despite the path

> > +     being wrong, just knowing the start and end addresses of the

> > +     mappings is still useful; we can attempt an access of the file

> > +     stratum constrained to the address ranges corresponding to the

> > +     unavailable mappings.  */

> > +

> > +  ULONGEST memaddr = offset;

> > +  ULONGEST memend = offset + len;

> > +

> > +  for (const auto &mr : m_core_unavailable_mappings)

> > +    {

> > +      if (address_in_mem_range (memaddr, &mr))

> > +        {

> > +	  if (!address_in_mem_range (memend, &mr))

> > +	    len = mr.start + mr.length - memaddr;

> > +

> > +	  xfer_status = this->beneath ()->xfer_partial (TARGET_OBJECT_MEMORY,

> > +							NULL,

> > +							readbuf,

> > +							writebuf,

> > +							offset,

> > +							len,

> > +							xfered_len);

> > +	  break;

> > +	}

> > +    }

> > +

> > +  return xfer_status;

> > +}

> > +

> >   enum target_xfer_status

> >   core_target::xfer_partial (enum target_object object, const char *annex,

> >   			   gdb_byte *readbuf, const gdb_byte *writebuf,

> > @@ -761,11 +832,8 @@ core_target::xfer_partial (enum target_object object, const char *annex,

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

> > +	  xfer_status = xfer_memory_via_mappings (readbuf, writebuf, offset,

> > +						  len, xfered_len);

> >   	else

> >   	  xfer_status = this->beneath ()->xfer_partial (object, annex, readbuf,

> >   							writebuf, offset, len,

> > diff --git a/gdb/testsuite/gdb.base/corefile2.exp b/gdb/testsuite/gdb.base/corefile2.exp

> > index 5de7ead4d4..11915a6185 100644

> > --- a/gdb/testsuite/gdb.base/corefile2.exp

> > +++ b/gdb/testsuite/gdb.base/corefile2.exp

> > @@ -143,6 +143,41 @@ gdb_test_multiple $test "" {

> >       }

> >   }

> >   

> > +# Test again with executable renamed during loading of core file.

> > +

> > +with_test_prefix "renamed binfile" {

> > +    # We don't use clean_restart here since we want to defer loading

> > +    # of $binfile until after the core file has been loaded.  (BFD

> > +    # will complain that $binfile has disappeared after the rename

> > +    # if it's loaded first.)

> > +    gdb_exit

> > +    gdb_start

> > +    gdb_reinitialize_dir $srcdir/$subdir

> > +

> > +    # Rename $binfile so that it won't be found during loading of

> > +    # the core file.

> > +    set hide_binfile [standard_output_file "${testfile}.hide"]

> > +    remote_exec host "mv -f $binfile $hide_binfile"

> > +

> > +    # Load core file - check that a warning is printed.

> > +    global xfail

> > +    if { $xfail } { setup_xfail "*-*-*" }

> > +    gdb_test_multiple "core-file $corefile" $test {

> > +	-re "warning: Can't open file.*during.* note processing.*Core was generated by .*\#0  .*\(\).*\r\n$gdb_prompt $" {

> > +	    pass $test

> > +	}

> > +	-re "\r\n$gdb_prompt $" {

> > +	    fail $test

> > +	}

> > +    }

> > +

> > +    # Restore $binfile and then load it.

> > +    remote_exec host "mv -f $hide_binfile $binfile"

> > +    gdb_load ${binfile}

> > +

> > +    do_tests

> > +}

> > +

> >   # Restart and run to the abort call.

> >   

> >   clean_restart $binfile

> >   

>
Hannes Domani via Gdb-patches Aug. 11, 2020, 5:02 p.m. | #3
On Sun, 9 Aug 2020 01:19:43 -0700
Kevin Buettner via Gdb-patches <gdb-patches@sourceware.org> wrote:

> That new failure is expected in your docker environment.  It might

> be possible to XFAIL it.  We need to be able to detect that we're

> running in docker with the AUFS storage driver.  Since we know that

> the kernel is leaking docker host paths, we should see these when the

> "info proc mappings" command is used with a loaded core file.  I'll

> give it a try...


It turned out that there's an even easier way to XFAIL this case.
See below...

From 23452e31502820e15c1c72a3935360f7f1932ed9 Mon Sep 17 00:00:00 2001
From: Kevin Buettner <kevinb@redhat.com>

Date: Tue, 11 Aug 2020 09:36:12 -0700
Subject: [PATCH] corefile.exp: XFAIL warning-free test when testing on docker

When testing on docker using the AUFS storage driver, loading a core
file will often print a number of warnings.  Here's an example (with
the pathname shortened somewhat):

warning: Can't open file /var/lib/docker/aufs/diff/d07..e21/lib/x86_64-linux-gnu/libc-2.27.so during file-backed mapping note processing

The "warning-free" test in gdb.base/corefile.exp will fail if any
warnings are printed, but this particular warning is unavoidable when
running in the docker environment.  Fortunately, the path mentions
both "docker" and "aufs", making it easy to XFAIL this case.

gdb/testsuite/ChangeLog:

	* gdb.base/corefile.exp (warning-free): XFAIL test when running
	on docker w/ AUFS storage driver.
---
 gdb/testsuite/gdb.base/corefile.exp | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/gdb/testsuite/gdb.base/corefile.exp b/gdb/testsuite/gdb.base/corefile.exp
index 8abf62b51f..b1022dd16f 100644
--- a/gdb/testsuite/gdb.base/corefile.exp
+++ b/gdb/testsuite/gdb.base/corefile.exp
@@ -324,11 +324,18 @@ corefile_test_attach
 # Test warning-free core file load.  E.g., a Linux vDSO used to
 # trigger this warning:
 #     warning: Can't read pathname for load map: Input/output error.
+#
+# When testing in a docker container using the AUFS storage driver,
+# the kernel places host paths in the core file's NT_FILE note.  XFAIL
+# this case since these paths make no sense in the container.
 
 clean_restart ${testfile}
 
 set test "core-file warning-free"
 gdb_test_multiple "core-file $corefile" $test {
+    -re "warning: Can\'t open file.*\/docker\/aufs\/.*\r\n$gdb_prompt $" {
+	xfail $test
+    }
     -re "warning: .*\r\n.*\r\n$gdb_prompt $" {
 	fail $test
     }
Hannes Domani via Gdb-patches Aug. 11, 2020, 5:21 p.m. | #4
Kevin,

On 8/11/20 2:02 PM, Kevin Buettner wrote:
> On Sun, 9 Aug 2020 01:19:43 -0700

> Kevin Buettner via Gdb-patches <gdb-patches@sourceware.org> wrote:

> 

>> That new failure is expected in your docker environment.  It might

>> be possible to XFAIL it.  We need to be able to detect that we're

>> running in docker with the AUFS storage driver.  Since we know that

>> the kernel is leaking docker host paths, we should see these when the

>> "info proc mappings" command is used with a loaded core file.  I'll

>> give it a try...

> 

> It turned out that there's an even easier way to XFAIL this case.

> See below...


Great! I tried this and it looks sane. I get one XFAIL in 
gdb.base/corefile.exp and full passes on gdb.base/corefile2.exp.

Thanks for addressing this.
Hannes Domani via Gdb-patches Aug. 11, 2020, 7:57 p.m. | #5
On Tue, 11 Aug 2020 14:21:53 -0300
Luis Machado <luis.machado@linaro.org> wrote:

> On 8/11/20 2:02 PM, Kevin Buettner wrote:

> > On Sun, 9 Aug 2020 01:19:43 -0700

> > Kevin Buettner via Gdb-patches <gdb-patches@sourceware.org> wrote:

> >   

> >> That new failure is expected in your docker environment.  It might

> >> be possible to XFAIL it.  We need to be able to detect that we're

> >> running in docker with the AUFS storage driver.  Since we know that

> >> the kernel is leaking docker host paths, we should see these when the

> >> "info proc mappings" command is used with a loaded core file.  I'll

> >> give it a try...  

> > 

> > It turned out that there's an even easier way to XFAIL this case.

> > See below...  

> 

> Great! I tried this and it looks sane. I get one XFAIL in 

> gdb.base/corefile.exp and full passes on gdb.base/corefile2.exp.


Excellent.  Thanks for testing it in your environment.

Kevin
Pedro Alves Aug. 20, 2020, 2:57 p.m. | #6
Hi Kevin,

This LGTM, with a few minor issues pointed out below addressed.

On 8/7/20 11:20 PM, Kevin Buettner via Gdb-patches wrote:
> Luis Machado reported some regressions after I pushed recent core file

> related patches fixing BZ 25631:

> 

>     FAIL: gdb.base/corefile.exp: backtrace in corefile.exp

>     FAIL: gdb.base/corefile.exp: core-file warning-free

>     FAIL: gdb.base/corefile.exp: print func2::coremaker_local

>     FAIL: gdb.base/corefile.exp: up in corefile.exp

>     FAIL: gdb.base/corefile.exp: up in corefile.exp (reinit)

> 

> While I haven't been able to reproduce these failures, I think I

> understand why they're happening.  It is my hope that this commit will

> fix those regressions.

> 

> Luis is testing in a docker container which is using the AUFS storage

> driver.  It turns out that the kernel is placing docker host paths in

> the NT_FILE note instead of paths within the container.

> 

> I've made a similar docker environment (though apparently not similar

> enough to reproduce the regressions).  This is one of the paths that

> I see mentioned in the warning messages printed while loading the

> core file during NT_FILE note processing - note that I've shortened

> the path component starting with "d07c4":

> 

> /var/lib/docker/aufs/diff/d07c4...21/lib/x86_64-linux-gnu/ld-2.27.so

> 

> This is a path on the docker host; it does not exist in the

> container.  In the docker container, this is the path:

> 

> /lib/x86_64-linux-gnu/ld-2.27.so

> 

> My first thought was to disable all NT_FILE mappings when any path was

> found to be bad.  This would have caused GDB to fall back to accessing

> memory using the file stratum as it did before I added the NT_FILE

> note loading code.  After further consideration, I realized that we

> could do better than this.  For file-backed memory access, we can

> still use the NT_FILE mappings when available, and then attempt to

> access memory using the file stratum constrainted to those address

> ranges corresponding to the "broken" mappings.

> 

> In order to test it, I made some additions to corefile2.exp in which

> the test case's executable is renamed.  The core file is then loaded;

> due to the fact that the executable has been renamed, those mappings

> will be unavailable.  After loading the core file, the executable is

> renamed back to its original name at which point it is loaded using

> GDB's "file" command.  The "interesting" tests are then run.  These

> tests will print out values in file-backed memory regions along with

> mmap'd regions placed within/over the file-backed regions.  Despite

> the fact that the executable could not be found during the NT_FILE

> note processing, these tests still work correctly due to the fact that

> memory is availble from the file stratum combined with the fact that


"availAble"

> the broken NT_FILE mappings are used to prevent file-backed access

> outside of the "broken" mappings.

> 

> gdb/ChangeLog:

> 

> 	* corelow.c (class core_target): Add field

> 	'm_core_unavailable_mappings'.

> 	(core_target::build_file_mappings): Print only one warning

> 	per inaccessible file.  Add unavailable/broken mappings

> 	to m_core_unavailable_mappings.

> 	(core_target::xfer_partial): Call...

> 	(core_target::xfer_memory_via_mappings): New method.

> 

> gdb/testsuite/ChangeLog:

> 

> 	* gdb.base/corefile2.exp (renamed binfile): New tests.

> ---

>  gdb/corelow.c                        | 84 +++++++++++++++++++++++++---

>  gdb/testsuite/gdb.base/corefile2.exp | 35 ++++++++++++

>  2 files changed, 111 insertions(+), 8 deletions(-)

> 

> diff --git a/gdb/corelow.c b/gdb/corelow.c

> index b6ee219f57..2e6be74a24 100644

> --- a/gdb/corelow.c

> +++ b/gdb/corelow.c

> @@ -131,9 +131,21 @@ class core_target final : public process_stratum_target

>       information about memory mapped files.  */

>    target_section_table m_core_file_mappings {};

>  

> +  /* Unavailable mappings.  These correspond to pathnames which either

> +     weren't found or could not be opened.  Knowing these addresses can

> +     still be useful.  */

> +  std::vector<mem_range> m_core_unavailable_mappings;

> +

>    /* Build m_core_file_mappings.  Called from the constructor.  */

>    void build_file_mappings ();

>  

> +  /* Helper method for xfer_partial.  */

> +  enum target_xfer_status xfer_memory_via_mappings (gdb_byte *readbuf,

> +                                                    const gdb_byte *writebuf,

> +						    ULONGEST offset,

> +						    ULONGEST len,

> +						    ULONGEST *xfered_len);


Seems like a tabs vs spaces mixup here.

> +

>    /* FIXME: kettenis/20031023: Eventually this field should

>       disappear.  */

>    struct gdbarch *m_core_gdbarch = NULL;

> @@ -182,6 +194,7 @@ void

>  core_target::build_file_mappings ()

>  {

>    std::unordered_map<std::string, struct bfd *> bfd_map;

> +  std::unordered_map<std::string, bool> unavailable_paths;

>  


And unordered_map to store a boolean that means "I've seen it"
sounds like the "wrong" data structure.  A more natural fit
would be:

  std::unordered_set<std::string> unavailable_paths;

Then either the string is in the set or is isn't.

>    /* See linux_read_core_file_mappings() in linux-tdep.c for an example

>       read_core_file_mappings method.  */

> @@ -216,9 +229,13 @@ core_target::build_file_mappings ()

>  	      = exec_file_find (filename, NULL);

>  	    if (expanded_fname == nullptr)

>  	      {

> -		warning (_("Can't open file %s during file-backed mapping "

> -			   "note processing"),

> -			 filename);

> +		m_core_unavailable_mappings.emplace_back (start, end - start);

> +		if (!unavailable_paths[filename])

> +		  warning (_("Can't open file %s during file-backed mapping "

> +			     "note processing"),

> +			   filename);

> +		/* Print just one warning per path.  */

> +		unavailable_paths[filename] = true;


and then:

		/* Print just one warning per path.  */
		if (unavailable_paths.insert (filename).second)
		  warning (_("Can't open file %s during file-backed mapping "
			     "note processing"),
			   filename);

unordered_set::insert does not insert if the element already exists,
and it returns a pair where you can check on the second field whether
something was inserted or not.

>  		return;

>  	      }

>  

> @@ -227,6 +244,7 @@ core_target::build_file_mappings ()

>  

>  	    if (bfd == nullptr || !bfd_check_format (bfd, bfd_object))

>  	      {

> +		m_core_unavailable_mappings.emplace_back (start, end - start);

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


Now that we're expecting to reach here, the comment about an internal
error seems stale.

> @@ -268,6 +286,8 @@ core_target::build_file_mappings ()

>  	ts->owner = nullptr;

>  	ts->the_bfd_section = sec;

>        });

> +

> +  normalize_mem_ranges (&m_core_unavailable_mappings);

>  }

>  

>  static void add_to_thread_list (bfd *, asection *, void *);

> @@ -728,6 +748,57 @@ core_target::files_info ()

>    print_section_info (&m_core_section_table, core_bfd);

>  }

>  

> +/* Helper method for core_target::xfer_partial.  */

> +

> +enum target_xfer_status

> +core_target::xfer_memory_via_mappings (gdb_byte *readbuf,

> +				       const gdb_byte *writebuf,

> +				       ULONGEST offset, ULONGEST len,

> +				       ULONGEST *xfered_len)

> +{

> +  enum target_xfer_status xfer_status;

> +

> +  xfer_status = section_table_xfer_memory_partial

> +		  (readbuf, writebuf,

> +		   offset, len, xfered_len,

> +		   m_core_file_mappings.sections,

> +		   m_core_file_mappings.sections_end);


Wrap in parens so that emacs will indent this correctly:

  xfer_status = (section_table_xfer_memory_partial
		 (readbuf, writebuf,
		  offset, len, xfered_len,
		  m_core_file_mappings.sections,
		  m_core_file_mappings.sections_end));


That's what the GNU standards say to do.


> +

> +  if (xfer_status == TARGET_XFER_OK || m_core_unavailable_mappings.empty ())

> +    return xfer_status;

> +

> +  /* There are instances - e.g. when debugging within a docker

> +     container using the AUFS storage driver - where the pathnames

> +     obtained from the note section are incorrect.  Despite the path

> +     being wrong, just knowing the start and end addresses of the

> +     mappings is still useful; we can attempt an access of the file

> +     stratum constrained to the address ranges corresponding to the

> +     unavailable mappings.  */

> +

> +  ULONGEST memaddr = offset;

> +  ULONGEST memend = offset + len;

> +

> +  for (const auto &mr : m_core_unavailable_mappings)

> +    {

> +      if (address_in_mem_range (memaddr, &mr))

> +        {

> +	  if (!address_in_mem_range (memend, &mr))

> +	    len = mr.start + mr.length - memaddr;

> +

> +	  xfer_status = this->beneath ()->xfer_partial (TARGET_OBJECT_MEMORY,

> +							NULL,

> +							readbuf,

> +							writebuf,

> +							offset,

> +							len,

> +							xfered_len);

> +	  break;

> +	}

> +    }

> +

> +  return xfer_status;

> +}

> +

>  enum target_xfer_status

>  core_target::xfer_partial (enum target_object object, const char *annex,

>  			   gdb_byte *readbuf, const gdb_byte *writebuf,

> @@ -761,11 +832,8 @@ core_target::xfer_partial (enum target_object object, const char *annex,

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

> +	  xfer_status = xfer_memory_via_mappings (readbuf, writebuf, offset,

> +						  len, xfered_len);

>  	else

>  	  xfer_status = this->beneath ()->xfer_partial (object, annex, readbuf,

>  							writebuf, offset, len,

> diff --git a/gdb/testsuite/gdb.base/corefile2.exp b/gdb/testsuite/gdb.base/corefile2.exp

> index 5de7ead4d4..11915a6185 100644

> --- a/gdb/testsuite/gdb.base/corefile2.exp

> +++ b/gdb/testsuite/gdb.base/corefile2.exp

> @@ -143,6 +143,41 @@ gdb_test_multiple $test "" {

>      }

>  }

>  

> +# Test again with executable renamed during loading of core file.

> +

> +with_test_prefix "renamed binfile" {

> +    # We don't use clean_restart here since we want to defer loading

> +    # of $binfile until after the core file has been loaded.  (BFD

> +    # will complain that $binfile has disappeared after the rename

> +    # if it's loaded first.)

> +    gdb_exit

> +    gdb_start

> +    gdb_reinitialize_dir $srcdir/$subdir


Please still call clean_restart, but just don't pass it any argument.
In that case, it skips loading any file.


> +

> +    # Rename $binfile so that it won't be found during loading of

> +    # the core file.

> +    set hide_binfile [standard_output_file "${testfile}.hide"]

> +    remote_exec host "mv -f $binfile $hide_binfile"

> +

> +    # Load core file - check that a warning is printed.

> +    global xfail

> +    if { $xfail } { setup_xfail "*-*-*" }

> +    gdb_test_multiple "core-file $corefile" $test {


$test here is still "maint print core-file-backed-mappings".

> +	-re "warning: Can't open file.*during.* note processing.*Core was generated by .*\#0  .*\(\).*\r\n$gdb_prompt $" {

> +	    pass $test

> +	}

> +	-re "\r\n$gdb_prompt $" {

> +	    fail $test

> +	}


This match is already done internally by gdb_test_multiple.
That suggests that this gdb_test_multiple call could be a
gdb_test call instead.

Thanks,
Pedro Alves


> +    }

> +

> +    # Restore $binfile and then load it.

> +    remote_exec host "mv -f $hide_binfile $binfile"

> +    gdb_load ${binfile}

> +

> +    do_tests

> +}

> +

>  # Restart and run to the abort call.

>  

>  clean_restart $binfile

>
Hannes Domani via Gdb-patches Aug. 26, 2020, 6:02 p.m. | #7
Hi,

On 8/11/20 4:57 PM, Kevin Buettner wrote:
> On Tue, 11 Aug 2020 14:21:53 -0300

> Luis Machado <luis.machado@linaro.org> wrote:

> 

>> On 8/11/20 2:02 PM, Kevin Buettner wrote:

>>> On Sun, 9 Aug 2020 01:19:43 -0700

>>> Kevin Buettner via Gdb-patches <gdb-patches@sourceware.org> wrote:

>>>    

>>>> That new failure is expected in your docker environment.  It might

>>>> be possible to XFAIL it.  We need to be able to detect that we're

>>>> running in docker with the AUFS storage driver.  Since we know that

>>>> the kernel is leaking docker host paths, we should see these when the

>>>> "info proc mappings" command is used with a loaded core file.  I'll

>>>> give it a try...

>>>

>>> It turned out that there's an even easier way to XFAIL this case.

>>> See below...

>>

>> Great! I tried this and it looks sane. I get one XFAIL in

>> gdb.base/corefile.exp and full passes on gdb.base/corefile2.exp.

> 

> Excellent.  Thanks for testing it in your environment.


Just for the record, this also fixes a couple failures I noticed today 
in gdb.threads/corethreads.exp, where GDB doesn't print the thread id 
properly. This is another fallout of the broken pathnames situation.
Hannes Domani via Gdb-patches Aug. 29, 2020, 3:25 a.m. | #8
Hi Pedro,

I've made all of the changes you suggested except for this one...

> > @@ -227,6 +244,7 @@ core_target::build_file_mappings ()

> >  

> >  	    if (bfd == nullptr || !bfd_check_format (bfd, bfd_object))

> >  	      {

> > +		m_core_unavailable_mappings.emplace_back (start, end - start);

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

> 

> Now that we're expecting to reach here, the comment about an internal

> error seems stale.


IMO, we'll (still) only reach the code in question due either to an
internal error or due to the condition mentioned later in the comment. 
Bear in mind that we've already checked for the existence of the file
and have even opened the file (though we're discarding the result) in
the call to exec_file_find().  The test with accompanying warning message
following the call to exec_file_find() should handle most if not all cases
where the path name is bad / broken.

The calls to bfd_openr() and bfd_check_format() _should_ be successful,
but we still need to check the result, hence second block containing the
comment and the warning, etc.

Below is an updated commit log plus patch...

Work around incorrect/broken pathnames in NT_FILE note

Luis Machado reported some regressions after I pushed recent core file
related patches fixing BZ 25631:

    FAIL: gdb.base/corefile.exp: backtrace in corefile.exp
    FAIL: gdb.base/corefile.exp: core-file warning-free
    FAIL: gdb.base/corefile.exp: print func2::coremaker_local
    FAIL: gdb.base/corefile.exp: up in corefile.exp
    FAIL: gdb.base/corefile.exp: up in corefile.exp (reinit)

This commit fixes these regressions.  Thanks to Luis for testing
an earlier version of the patch.  (I was unable to reproduce these
regressions in various test environments that I created.)

Luis is testing in a docker container which is using the AUFS storage
driver.  It turns out that the kernel is placing docker host paths in
the NT_FILE note instead of paths within the container.

I've made a similar docker environment (though apparently not similar
enough to reproduce the regressions).  This is one of the paths that
I see mentioned in the warning messages printed while loading the
core file during NT_FILE note processing - note that I've shortened
the path component starting with "d07c4":

/var/lib/docker/aufs/diff/d07c4...21/lib/x86_64-linux-gnu/ld-2.27.so

This is a path on the docker host; it does not exist in the
container.  In the docker container, this is the path:

/lib/x86_64-linux-gnu/ld-2.27.so

My first thought was to disable all NT_FILE mappings when any path was
found to be bad.  This would have caused GDB to fall back to accessing
memory using the file stratum as it did before I added the NT_FILE
note loading code.  After further consideration, I realized that we
could do better than this.  For file-backed memory access, we can
still use the NT_FILE mappings when available, and then attempt to
access memory using the file stratum constrained to those address
ranges corresponding to the "broken" mappings.

In order to test it, I made some additions to corefile2.exp in which
the test case's executable is renamed.  The core file is then loaded;
due to the fact that the executable has been renamed, those mappings
will be unavailable.  After loading the core file, the executable is
renamed back to its original name at which point it is loaded using
GDB's "file" command.  The "interesting" tests are then run.  These
tests will print out values in file-backed memory regions along with
mmap'd regions placed within/over the file-backed regions.  Despite
the fact that the executable could not be found during the NT_FILE
note processing, these tests still work correctly due to the fact that
memory is available from the file stratum combined with the fact that
the broken NT_FILE mappings are used to prevent file-backed access
outside of the "broken" mappings.

gdb/ChangeLog:

	* corelow.c (unordered_set): Include.
	(class core_target): Add field 'm_core_unavailable_mappings'.
	(core_target::build_file_mappings): Print only one warning
	per inaccessible file.  Add unavailable/broken mappings
	to m_core_unavailable_mappings.
	(core_target::xfer_partial): Call...
	(core_target::xfer_memory_via_mappings): New method.

gdb/testsuite/ChangeLog:

	* gdb.base/corefile2.exp (renamed binfile): New tests.
---
 gdb/corelow.c                        | 84 ++++++++++++++++++++++++++++++++----
 gdb/testsuite/gdb.base/corefile2.exp | 27 ++++++++++++
 2 files changed, 103 insertions(+), 8 deletions(-)

diff --git a/gdb/corelow.c b/gdb/corelow.c
index b6ee219f57..96ec739c62 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 <unordered_set>
 #include "gdbcmd.h"
 
 #ifndef O_LARGEFILE
@@ -131,9 +132,21 @@ class core_target final : public process_stratum_target
      information about memory mapped files.  */
   target_section_table m_core_file_mappings {};
 
+  /* Unavailable mappings.  These correspond to pathnames which either
+     weren't found or could not be opened.  Knowing these addresses can
+     still be useful.  */
+  std::vector<mem_range> m_core_unavailable_mappings;
+
   /* Build m_core_file_mappings.  Called from the constructor.  */
   void build_file_mappings ();
 
+  /* Helper method for xfer_partial.  */
+  enum target_xfer_status xfer_memory_via_mappings (gdb_byte *readbuf,
+						    const gdb_byte *writebuf,
+						    ULONGEST offset,
+						    ULONGEST len,
+						    ULONGEST *xfered_len);
+
   /* FIXME: kettenis/20031023: Eventually this field should
      disappear.  */
   struct gdbarch *m_core_gdbarch = NULL;
@@ -182,6 +195,7 @@ void
 core_target::build_file_mappings ()
 {
   std::unordered_map<std::string, struct bfd *> bfd_map;
+  std::unordered_set<std::string> unavailable_paths;
 
   /* See linux_read_core_file_mappings() in linux-tdep.c for an example
      read_core_file_mappings method.  */
@@ -216,9 +230,12 @@ core_target::build_file_mappings ()
 	      = exec_file_find (filename, NULL);
 	    if (expanded_fname == nullptr)
 	      {
-		warning (_("Can't open file %s during file-backed mapping "
-			   "note processing"),
-			 filename);
+		m_core_unavailable_mappings.emplace_back (start, end - start);
+		/* Print just one warning per path.  */
+		if (unavailable_paths.insert (filename).second)
+		  warning (_("Can't open file %s during file-backed mapping "
+			     "note processing"),
+			   filename);
 		return;
 	      }
 
@@ -227,6 +244,7 @@ core_target::build_file_mappings ()
 
 	    if (bfd == nullptr || !bfd_check_format (bfd, bfd_object))
 	      {
+		m_core_unavailable_mappings.emplace_back (start, end - start);
 		/* 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
@@ -268,6 +286,8 @@ core_target::build_file_mappings ()
 	ts->owner = nullptr;
 	ts->the_bfd_section = sec;
       });
+
+  normalize_mem_ranges (&m_core_unavailable_mappings);
 }
 
 static void add_to_thread_list (bfd *, asection *, void *);
@@ -728,6 +748,57 @@ core_target::files_info ()
   print_section_info (&m_core_section_table, core_bfd);
 }
 
+/* Helper method for core_target::xfer_partial.  */
+
+enum target_xfer_status
+core_target::xfer_memory_via_mappings (gdb_byte *readbuf,
+				       const gdb_byte *writebuf,
+				       ULONGEST offset, ULONGEST len,
+				       ULONGEST *xfered_len)
+{
+  enum target_xfer_status xfer_status;
+
+  xfer_status = (section_table_xfer_memory_partial
+		   (readbuf, writebuf,
+		    offset, len, xfered_len,
+		    m_core_file_mappings.sections,
+		    m_core_file_mappings.sections_end));
+
+  if (xfer_status == TARGET_XFER_OK || m_core_unavailable_mappings.empty ())
+    return xfer_status;
+
+  /* There are instances - e.g. when debugging within a docker
+     container using the AUFS storage driver - where the pathnames
+     obtained from the note section are incorrect.  Despite the path
+     being wrong, just knowing the start and end addresses of the
+     mappings is still useful; we can attempt an access of the file
+     stratum constrained to the address ranges corresponding to the
+     unavailable mappings.  */
+
+  ULONGEST memaddr = offset;
+  ULONGEST memend = offset + len;
+
+  for (const auto &mr : m_core_unavailable_mappings)
+    {
+      if (address_in_mem_range (memaddr, &mr))
+        {
+	  if (!address_in_mem_range (memend, &mr))
+	    len = mr.start + mr.length - memaddr;
+
+	  xfer_status = this->beneath ()->xfer_partial (TARGET_OBJECT_MEMORY,
+							NULL,
+							readbuf,
+							writebuf,
+							offset,
+							len,
+							xfered_len);
+	  break;
+	}
+    }
+
+  return xfer_status;
+}
+
 enum target_xfer_status
 core_target::xfer_partial (enum target_object object, const char *annex,
 			   gdb_byte *readbuf, const gdb_byte *writebuf,
@@ -761,11 +832,8 @@ core_target::xfer_partial (enum target_object object, const char *annex,
 	   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);
+	  xfer_status = xfer_memory_via_mappings (readbuf, writebuf, offset,
+						  len, xfered_len);
 	else
 	  xfer_status = this->beneath ()->xfer_partial (object, annex, readbuf,
 							writebuf, offset, len,
diff --git a/gdb/testsuite/gdb.base/corefile2.exp b/gdb/testsuite/gdb.base/corefile2.exp
index 5de7ead4d4..38b8eb770b 100644
--- a/gdb/testsuite/gdb.base/corefile2.exp
+++ b/gdb/testsuite/gdb.base/corefile2.exp
@@ -143,6 +143,33 @@ gdb_test_multiple $test "" {
     }
 }
 
+# Test again with executable renamed during loading of core file.
+
+with_test_prefix "renamed binfile" {
+    # Don't load $binfile in this call to clean_restart.  (BFD will
+    # complain that $binfile has disappeared after the rename if it's
+    # loaded first.)
+    clean_restart
+
+    # Rename $binfile so that it won't be found during loading of
+    # the core file.
+    set hide_binfile [standard_output_file "${testfile}.hide"]
+    remote_exec host "mv -f $binfile $hide_binfile"
+
+    # Load core file - check that a warning is printed.
+    global xfail
+    if { $xfail } { setup_xfail "*-*-*" }
+    gdb_test "core-file $corefile" \
+	"warning: Can't open file.*during.* note processing.*Core was generated by .*\#0  .*\(\)" \
+	"load core file without having first loaded binfile"
+
+    # Restore $binfile and then load it.
+    remote_exec host "mv -f $hide_binfile $binfile"
+    gdb_load ${binfile}
+
+    do_tests
+}
+
 # Restart and run to the abort call.
 
 clean_restart $binfile
Pedro Alves Aug. 31, 2020, 2:50 p.m. | #9
Hi Kevin,

On 8/29/20 4:25 AM, Kevin Buettner wrote:
> Hi Pedro,

> 

> I've made all of the changes you suggested except for this one...

> 

>>> @@ -227,6 +244,7 @@ core_target::build_file_mappings ()

>>>  

>>>  	    if (bfd == nullptr || !bfd_check_format (bfd, bfd_object))

>>>  	      {

>>> +		m_core_unavailable_mappings.emplace_back (start, end - start);

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

>>

>> Now that we're expecting to reach here, the comment about an internal

>> error seems stale.

> 

> IMO, we'll (still) only reach the code in question due either to an

> internal error or due to the condition mentioned later in the comment. 

> Bear in mind that we've already checked for the existence of the file

> and have even opened the file (though we're discarding the result) in

> the call to exec_file_find().  The test with accompanying warning message

> following the call to exec_file_find() should handle most if not all cases

> where the path name is bad / broken.

> 

> The calls to bfd_openr() and bfd_check_format() _should_ be successful,

> but we still need to check the result, hence second block containing the

> comment and the warning, etc.

> 


I see, thanks.

This version LGTM.

Pedro Alves
Hannes Domani via Gdb-patches Sept. 1, 2020, 2:03 a.m. | #10
On Tue, 11 Aug 2020 10:02:10 -0700
Kevin Buettner <kevinb@redhat.com> wrote:

> From 23452e31502820e15c1c72a3935360f7f1932ed9 Mon Sep 17 00:00:00 2001

> From: Kevin Buettner <kevinb@redhat.com>

> Date: Tue, 11 Aug 2020 09:36:12 -0700

> Subject: [PATCH] corefile.exp: XFAIL warning-free test when testing on docker

> 

> When testing on docker using the AUFS storage driver, loading a core

> file will often print a number of warnings.  Here's an example (with

> the pathname shortened somewhat):

> 

> warning: Can't open file /var/lib/docker/aufs/diff/d07..e21/lib/x86_64-linux-gnu/libc-2.27.so during file-backed mapping note processing

> 

> The "warning-free" test in gdb.base/corefile.exp will fail if any

> warnings are printed, but this particular warning is unavoidable when

> running in the docker environment.  Fortunately, the path mentions

> both "docker" and "aufs", making it easy to XFAIL this case.

> 

> gdb/testsuite/ChangeLog:

> 

> 	* gdb.base/corefile.exp (warning-free): XFAIL test when running

> 	on docker w/ AUFS storage driver.


I've pushed this change.

Kevin
Hannes Domani via Gdb-patches Sept. 1, 2020, 2:04 a.m. | #11
On Mon, 31 Aug 2020 15:50:07 +0100
Pedro Alves <pedro@palves.net> wrote:

> Hi Kevin,

> 

> On 8/29/20 4:25 AM, Kevin Buettner wrote:

> > Hi Pedro,

> > 

> > I've made all of the changes you suggested except for this one...

> >   

> >>> @@ -227,6 +244,7 @@ core_target::build_file_mappings ()

> >>>  

> >>>  	    if (bfd == nullptr || !bfd_check_format (bfd, bfd_object))

> >>>  	      {

> >>> +		m_core_unavailable_mappings.emplace_back (start, end - start);

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

> >>

> >> Now that we're expecting to reach here, the comment about an internal

> >> error seems stale.  

> > 

> > IMO, we'll (still) only reach the code in question due either to an

> > internal error or due to the condition mentioned later in the comment. 

> > Bear in mind that we've already checked for the existence of the file

> > and have even opened the file (though we're discarding the result) in

> > the call to exec_file_find().  The test with accompanying warning message

> > following the call to exec_file_find() should handle most if not all cases

> > where the path name is bad / broken.

> > 

> > The calls to bfd_openr() and bfd_check_format() _should_ be successful,

> > but we still need to check the result, hence second block containing the

> > comment and the warning, etc.

> >   

> 

> I see, thanks.

> 

> This version LGTM.

> 

> Pedro Alves

> 


Thanks for the reviews.  I've pushed this version.

Kevin

Patch

diff --git a/gdb/corelow.c b/gdb/corelow.c
index b6ee219f57..2e6be74a24 100644
--- a/gdb/corelow.c
+++ b/gdb/corelow.c
@@ -131,9 +131,21 @@  class core_target final : public process_stratum_target
      information about memory mapped files.  */
   target_section_table m_core_file_mappings {};
 
+  /* Unavailable mappings.  These correspond to pathnames which either
+     weren't found or could not be opened.  Knowing these addresses can
+     still be useful.  */
+  std::vector<mem_range> m_core_unavailable_mappings;
+
   /* Build m_core_file_mappings.  Called from the constructor.  */
   void build_file_mappings ();
 
+  /* Helper method for xfer_partial.  */
+  enum target_xfer_status xfer_memory_via_mappings (gdb_byte *readbuf,
+                                                    const gdb_byte *writebuf,
+						    ULONGEST offset,
+						    ULONGEST len,
+						    ULONGEST *xfered_len);
+
   /* FIXME: kettenis/20031023: Eventually this field should
      disappear.  */
   struct gdbarch *m_core_gdbarch = NULL;
@@ -182,6 +194,7 @@  void
 core_target::build_file_mappings ()
 {
   std::unordered_map<std::string, struct bfd *> bfd_map;
+  std::unordered_map<std::string, bool> unavailable_paths;
 
   /* See linux_read_core_file_mappings() in linux-tdep.c for an example
      read_core_file_mappings method.  */
@@ -216,9 +229,13 @@  core_target::build_file_mappings ()
 	      = exec_file_find (filename, NULL);
 	    if (expanded_fname == nullptr)
 	      {
-		warning (_("Can't open file %s during file-backed mapping "
-			   "note processing"),
-			 filename);
+		m_core_unavailable_mappings.emplace_back (start, end - start);
+		if (!unavailable_paths[filename])
+		  warning (_("Can't open file %s during file-backed mapping "
+			     "note processing"),
+			   filename);
+		/* Print just one warning per path.  */
+		unavailable_paths[filename] = true;
 		return;
 	      }
 
@@ -227,6 +244,7 @@  core_target::build_file_mappings ()
 
 	    if (bfd == nullptr || !bfd_check_format (bfd, bfd_object))
 	      {
+		m_core_unavailable_mappings.emplace_back (start, end - start);
 		/* 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
@@ -268,6 +286,8 @@  core_target::build_file_mappings ()
 	ts->owner = nullptr;
 	ts->the_bfd_section = sec;
       });
+
+  normalize_mem_ranges (&m_core_unavailable_mappings);
 }
 
 static void add_to_thread_list (bfd *, asection *, void *);
@@ -728,6 +748,57 @@  core_target::files_info ()
   print_section_info (&m_core_section_table, core_bfd);
 }
 
+/* Helper method for core_target::xfer_partial.  */
+
+enum target_xfer_status
+core_target::xfer_memory_via_mappings (gdb_byte *readbuf,
+				       const gdb_byte *writebuf,
+				       ULONGEST offset, ULONGEST len,
+				       ULONGEST *xfered_len)
+{
+  enum target_xfer_status xfer_status;
+
+  xfer_status = section_table_xfer_memory_partial
+		  (readbuf, writebuf,
+		   offset, len, xfered_len,
+		   m_core_file_mappings.sections,
+		   m_core_file_mappings.sections_end);
+
+  if (xfer_status == TARGET_XFER_OK || m_core_unavailable_mappings.empty ())
+    return xfer_status;
+
+  /* There are instances - e.g. when debugging within a docker
+     container using the AUFS storage driver - where the pathnames
+     obtained from the note section are incorrect.  Despite the path
+     being wrong, just knowing the start and end addresses of the
+     mappings is still useful; we can attempt an access of the file
+     stratum constrained to the address ranges corresponding to the
+     unavailable mappings.  */
+
+  ULONGEST memaddr = offset;
+  ULONGEST memend = offset + len;
+
+  for (const auto &mr : m_core_unavailable_mappings)
+    {
+      if (address_in_mem_range (memaddr, &mr))
+        {
+	  if (!address_in_mem_range (memend, &mr))
+	    len = mr.start + mr.length - memaddr;
+
+	  xfer_status = this->beneath ()->xfer_partial (TARGET_OBJECT_MEMORY,
+							NULL,
+							readbuf,
+							writebuf,
+							offset,
+							len,
+							xfered_len);
+	  break;
+	}
+    }
+
+  return xfer_status;
+}
+
 enum target_xfer_status
 core_target::xfer_partial (enum target_object object, const char *annex,
 			   gdb_byte *readbuf, const gdb_byte *writebuf,
@@ -761,11 +832,8 @@  core_target::xfer_partial (enum target_object object, const char *annex,
 	   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);
+	  xfer_status = xfer_memory_via_mappings (readbuf, writebuf, offset,
+						  len, xfered_len);
 	else
 	  xfer_status = this->beneath ()->xfer_partial (object, annex, readbuf,
 							writebuf, offset, len,
diff --git a/gdb/testsuite/gdb.base/corefile2.exp b/gdb/testsuite/gdb.base/corefile2.exp
index 5de7ead4d4..11915a6185 100644
--- a/gdb/testsuite/gdb.base/corefile2.exp
+++ b/gdb/testsuite/gdb.base/corefile2.exp
@@ -143,6 +143,41 @@  gdb_test_multiple $test "" {
     }
 }
 
+# Test again with executable renamed during loading of core file.
+
+with_test_prefix "renamed binfile" {
+    # We don't use clean_restart here since we want to defer loading
+    # of $binfile until after the core file has been loaded.  (BFD
+    # will complain that $binfile has disappeared after the rename
+    # if it's loaded first.)
+    gdb_exit
+    gdb_start
+    gdb_reinitialize_dir $srcdir/$subdir
+
+    # Rename $binfile so that it won't be found during loading of
+    # the core file.
+    set hide_binfile [standard_output_file "${testfile}.hide"]
+    remote_exec host "mv -f $binfile $hide_binfile"
+
+    # Load core file - check that a warning is printed.
+    global xfail
+    if { $xfail } { setup_xfail "*-*-*" }
+    gdb_test_multiple "core-file $corefile" $test {
+	-re "warning: Can't open file.*during.* note processing.*Core was generated by .*\#0  .*\(\).*\r\n$gdb_prompt $" {
+	    pass $test
+	}
+	-re "\r\n$gdb_prompt $" {
+	    fail $test
+	}
+    }
+
+    # Restore $binfile and then load it.
+    remote_exec host "mv -f $hide_binfile $binfile"
+    gdb_load ${binfile}
+
+    do_tests
+}
+
 # Restart and run to the abort call.
 
 clean_restart $binfile