Re: [PATCH] Work around incorrect/broken pathnames in NT_FILE note

[Luis Machado <luis.machado@linaro.org>]

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);
>   }
>   \f
> +/* 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
>