Re: [PATCH v2 3/3] Make exec-file-mismatch compare build IDs

[Philippe Waroquiers <philippe.waroquiers@skynet.be>]

Patch looks nice to me.

Just one small comment: the call
   add_setshow_enum_cmd ("exec-file-mismatch", class_support,
still references exec-file name in the help doc string.
The help doc should better reference the build ID, or alternatively
be generic by removing "name".

Thanks


Philippe

On Mon, 2020-05-18 at 14:55 +0100, Pedro Alves via Gdb-patches wrote:
> On 5/17/20 7:04 PM, Pedro Alves via Gdb-patches wrote:
> > The patch makes GDB first try exec-file-mismatch validation via build
> > IDs, and then if that isn't possible, fallback to validating using the
> > old method of comparing filenames.  I'd argue that we should remove
> > the filename validation for causing too many false positives, though.
> 
> I've argued about that yesterday over at gdb@, in this thread:
>   https://sourceware.org/pipermail/gdb/2020-May/048544.html
> 
> Here's an updated version that removes the filename comparison.
> I've kept the structure of the code the same, in case we
> add some form of fallback later on.
> 
> From 771c3a52fd88c351bd4e93491f591f9b942ce217 Mon Sep 17 00:00:00 2001
> From: Pedro Alves <palves@redhat.com>
> Date: Mon, 18 May 2020 14:49:34 +0100
> Subject: [PATCH] Make exec-file-mismatch compare build IDs
> 
> The patch makes GDB do exec-file-mismatch validation by comparing
> build IDs instead of the current method of comparing filenames.
> 
> Currently, the exec-file-mismatch feature simply compares filenames to
> decide whether the exec file loaded in gdb and the exec file the
> target reports is running match.  This causes false positives when
> remote debugging, because it'll often be the case that the paths in
> the host and the target won't match.  And of course misses the case of
> the files having the same name but being actually different files
> (e.g., different builds).
> 
> This also broke many testcases when running against gdbserver, causing
> tests to be skipped like (here native-extended-gdbserver):
> 
>   (gdb) run
>   Starting program: /home/pedro/gdb/binutils-gdb/build/gdb/testsuite/outputs/gdb.base/argv0-symlink/argv0-symlink-filelink
>   warning: Mismatch between current exec-file /home/pedro/gdb/binutils-gdb/build/gdb/testsuite/outputs/gdb.base/argv0-symlink/argv0-symlink-filelink
>   and automatically determined exec-file /home/pedro/gdb/binutils-gdb/build/gdb/testsuite/outputs/gdb.base/argv0-symlink/argv0-symlink
>   exec-file-mismatch handling is currently "ask"
>   Load new symbol table from "/home/pedro/gdb/binutils-gdb/build/gdb/testsuite/outputs/gdb.base/argv0-symlink/argv0-symlink"? (y or n) UNTESTED: gdb.base/argv0-symlink.exp: could not run to main
> 
> or to fail like (here native-gdbserver):
> 
>  (gdb) spawn /home/pedro/gdb/binutils-gdb/build/gdb/testsuite/../../gdbserver/gdbserver --once localhost:2346 /home/pedro/gdb/binutils-gdb/build/gdb/te
>  stsuite/outputs/gdb.btrace/buffer-size/skip_btrace_tests-19968.x
>  Process /home/pedro/gdb/binutils-gdb/build/gdb/testsuite/outputs/gdb.btrace/buffer-size/skip_btrace_tests-19968.x created; pid = 20040
>  Listening on port 2346
>  target remote localhost:2346
>  Remote debugging using localhost:2346
>  warning: Mismatch between current exec-file /home/pedro/gdb/binutils-gdb/build/gdb/testsuite/temp/19968/skip_btrace_tests-19968.x
>  and automatically determined exec-file /home/pedro/gdb/binutils-gdb/build/gdb/testsuite/outputs/gdb.btrace/buffer-size/skip_btrace_tests-19968.x
>  exec-file-mismatch handling is currently "ask"
>  Load new symbol table from "/home/pedro/gdb/binutils-gdb/build/gdb/testsuite/outputs/gdb.btrace/buffer-size/skip_btrace_tests-19968.x"? (y or n) Quit
>  (gdb) UNSUPPORTED: gdb.btrace/buffer-size.exp: target does not support record-btrace
> 
> The former case is about GDB not realizing the two files are the same,
> because one of the them is a symlink to the other.  The latter case is
> about GDB realizing that one file is a copy of the other.
> 
> Over the years, the toolchain has settled on build ID matching being
> the canonical method to match core dumps to executables, and
> executables with no debug info to their debug info.
> 
> This patch makes us use build IDs to match the running image of a
> binary with its version loaded in gdb, which may or may not have debug
> info.  This is very much like the core dump/executable matching.
> 
> The change to gdb_bfd_open is necessary to get rid of the "transfers
> from remote targets can be slow" warning when we open the remote file
> to read its build ID:
> 
>  (gdb) r
>  Starting program: /home/pedro/gdb/binutils-gdb/build/gdb/testsuite/outputs/gdb.base/break/break
>  Reading /home/pedro/gdb/binutils-gdb/build/gdb/testsuite/outputs/gdb.base/argv0-symlink/argv0-symlink from remote target...
>  warning: File transfers from remote targets can be slow. Use "set sysroot" to access files locally instead.
>  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>  warning: Mismatch between current exec-file /home/pedro/gdb/binutils-gdb/build/gdb/testsuite/outputs/gdb.base/break/break
>  and automatically determined exec-file /home/pedro/gdb/binutils-gdb/build/gdb/testsuite/outputs/gdb.base/argv0-symlink/argv0-symlink
>  exec-file-mismatch handling is currently "ask"
>  Load new symbol table from "/home/pedro/gdb/binutils-gdb/build/gdb/testsuite/outputs/gdb.base/argv0-symlink/argv0-symlink"? (y or n)
> 
> While trying this out, I was worried that bfd would read a lot of
> stuff from the binary in order to extract the build ID, making it
> potentially slow, but turns out we don't read all that much.  Maybe a
> couple hundred bytes, and most of it seemingly is the read-ahead
> cache.  So I'm not worried about that.  Otherwise I'd consider whether
> a new qXfer:buildid:read would be better.  But I'm happy that we
> seemingly don't need to worry about it.
> 
> gdb/ChangeLog:
> yyyy-mm-dd  Pedro Alves  <palves@redhat.com>
> 
> 	* NEWS (set exec-file-mismatch): Adjust entry.
> 	* exec.c: Include "build-id.h".
> 	(validate_exec_file): Try to match build IDs instead of filenames.
> 	* gdb_bfd.c (struct gdb_bfd_open_closure): New.
> 	(gdb_bfd_iovec_fileio_open): Adjust to use gdb_bfd_open_closure
> 	and pass down 'warn_if_slow'.
> 	(gdb_bfd_open): Add 'warn_if_slow' parameter.  Use
> 	gdb_bfd_open_closure to pass it down.
> 	* gdb_bfd.h (gdb_bfd_open): Add 'warn_if_slow' parameter.
> 
> gdb/doc/ChangeLog:
> yyyy-mm-dd  Pedro Alves  <palves@redhat.com>
> 
> 	* gdb.texinfo (Attach): Update exec-file-mismatch description to
> 	mention build IDs.
> 	(Separate Debug Files): Add "build id" anchor.
> ---
>  gdb/doc/gdb.texinfo | 21 +++++++++++----------
>  gdb/NEWS            | 15 +++++++--------
>  gdb/exec.c          | 49 ++++++++++++++++++++++++++++++++++++++++++-------
>  gdb/gdb_bfd.c       | 23 ++++++++++++++++-------
>  gdb/gdb_bfd.h       |  6 ++++--
>  5 files changed, 80 insertions(+), 34 deletions(-)
> 
> diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
> index 8f3301259af..64181628495 100644
> --- a/gdb/doc/gdb.texinfo
> +++ b/gdb/doc/gdb.texinfo
> @@ -2909,22 +2909,22 @@ the @code{file} command to load the program.  @xref{Files, ,Commands to
>  Specify Files}.
>  
>  @anchor{set exec-file-mismatch}
> -If the debugger can determine the name of the executable file running
> -in the process it is attaching to, and this file name does not match
> -the name of the current exec-file loaded by @value{GDBN}, the option
> -@code{exec-file-mismatch} specifies how to handle the mismatch.
> +If the debugger can determine that the executable file running in the
> +process it is attaching to does not match the current exec-file loaded
> +by @value{GDBN}, the option @code{exec-file-mismatch} specifies how to
> +handle the mismatch.  @value{GDBN} tries to compare the files by
> +comparing their build IDs (@pxref{build ID}), if available.
>  
>  @table @code
>  @kindex exec-file-mismatch
>  @cindex set exec-file-mismatch
>  @item set exec-file-mismatch @samp{ask|warn|off}
>  
> -Whether to detect mismatch between the name of the current executable
> -file loaded by @value{GDBN} and the name of the executable file used to
> -start the process.  If @samp{ask}, the default, display a warning
> -and ask the user whether to load the process executable file; if
> -@samp{warn}, just display a warning; if @samp{off}, don't attempt to
> -detect a mismatch.
> +Whether to detect mismatch between the current executable file loaded
> +by @value{GDBN} and the executable file used to start the process.  If
> +@samp{ask}, the default, display a warning and ask the user whether to
> +load the process executable file; if @samp{warn}, just display a
> +warning; if @samp{off}, don't attempt to detect a mismatch.
>  
>  @cindex show exec-file-mismatch
>  @item show exec-file-mismatch
> @@ -20874,6 +20874,7 @@ checksum for the debug file, which @value{GDBN} uses to validate that
>  the executable and the debug file came from the same build.
>  
>  @item
> +@anchor{build ID}
>  The executable contains a @dfn{build ID}, a unique bit string that is
>  also present in the corresponding debug info file.  (This is supported
>  only on some operating systems, when using the ELF or PE file formats
> diff --git a/gdb/NEWS b/gdb/NEWS
> index a059fc7aa0e..2a9c8b8ee19 100644
> --- a/gdb/NEWS
> +++ b/gdb/NEWS
> @@ -54,14 +54,13 @@
>  
>  set exec-file-mismatch -- Set exec-file-mismatch handling (ask|warn|off).
>  show exec-file-mismatch -- Show exec-file-mismatch handling (ask|warn|off).
> -  Set or show the option 'exec-file-mismatch'.  When GDB attaches to
> -  a running process and can determine the name of the executable file
> -  the process runs, this new option indicates whether to detect mismatch
> -  between the name of the current executable file loaded by GDB
> -  and the name of the executable file used to start the process.
> -  If 'ask', the default, display a warning and ask the user
> -  whether to load the process executable file; if 'warn', just display
> -  a warning; if 'off', don't attempt to detect a mismatch.
> +  Set or show the option 'exec-file-mismatch'.  When GDB attaches to a
> +  running process, this new option indicates whether to detect
> +  a mismatch between the current executable file loaded by GDB and the
> +  executable file used to start the process.  If 'ask', the default,
> +  display a warning and ask the user whether to load the process
> +  executable file; if 'warn', just display a warning; if 'off', don't
> +  attempt to detect a mismatch.
>  
>  tui new-layout NAME WINDOW WEIGHT [WINDOW WEIGHT]...
>    Define a new TUI layout, specifying its name and the windows that
> diff --git a/gdb/exec.c b/gdb/exec.c
> index a2added5e22..8b89828ddda 100644
> --- a/gdb/exec.c
> +++ b/gdb/exec.c
> @@ -37,6 +37,7 @@
>  #include "gdb_bfd.h"
>  #include "gcore.h"
>  #include "source.h"
> +#include "build-id.h"
>  
>  #include <fcntl.h>
>  #include "readline/tilde.h"
> @@ -247,20 +248,54 @@ validate_exec_file (int from_tty)
>    struct inferior *inf = current_inferior ();
>    /* Try to determine a filename from the process itself.  */
>    const char *pid_exec_file = target_pid_to_exec_file (inf->pid);
> +  bool build_id_mismatch = false;
>  
> -  /* If wee cannot validate the exec file, return.  */
> +  /* If we cannot validate the exec file, return.  */
>    if (current_exec_file == NULL || pid_exec_file == NULL)
>      return;
>  
> -  std::string exec_file_target (pid_exec_file);
> +  /* Try validating via build-id, if available.  This is the most
> +     reliable check.  */
> +  const bfd_build_id *exec_file_build_id = build_id_bfd_get (exec_bfd);
> +  if (exec_file_build_id != nullptr)
> +    {
> +      /* Prepend the target prefix, to force gdb_bfd_open to open the
> +	 file on the remote file system (if indeed remote).  */
> +      std::string target_pid_exec_file
> +	= std::string (TARGET_SYSROOT_PREFIX) + pid_exec_file;
> +
> +      gdb_bfd_ref_ptr abfd (gdb_bfd_open (target_pid_exec_file.c_str (),
> +					  gnutarget, -1, false));
> +      if (abfd != nullptr)
> +	{
> +	  const bfd_build_id *target_exec_file_build_id
> +	    = build_id_bfd_get (abfd.get ());
>  
> -  /* In case the exec file is not local, exec_file_target has to point at
> -     the target file system.  */
> -  if (is_target_filename (current_exec_file) && !target_filesystem_is_local ())
> -    exec_file_target = TARGET_SYSROOT_PREFIX + exec_file_target;
> +	  if (target_exec_file_build_id != nullptr)
> +	    {
> +	      if (exec_file_build_id->size == target_exec_file_build_id->size
> +		  && memcmp (exec_file_build_id->data,
> +			     target_exec_file_build_id->data,
> +			     exec_file_build_id->size) == 0)
> +		{
> +		  /* Match.  */
> +		  return;
> +		}
> +	      else
> +		build_id_mismatch = true;
> +	    }
> +	}
> +    }
>  
> -  if (exec_file_target != current_exec_file)
> +  if (build_id_mismatch)
>      {
> +      std::string exec_file_target (pid_exec_file);
> +
> +      /* In case the exec file is not local, exec_file_target has to point at
> +	 the target file system.  */
> +      if (is_target_filename (current_exec_file) && !target_filesystem_is_local ())
> +	exec_file_target = TARGET_SYSROOT_PREFIX + exec_file_target;
> +
>        warning
>  	(_("Mismatch between current exec-file %ps\n"
>  	   "and automatically determined exec-file %ps\n"
> diff --git a/gdb/gdb_bfd.c b/gdb/gdb_bfd.c
> index 61872a0bf3d..25e0178a8b8 100644
> --- a/gdb/gdb_bfd.c
> +++ b/gdb/gdb_bfd.c
> @@ -271,22 +271,29 @@ fileio_errno_to_host (int errnum)
>    return -1;
>  }
>  
> +/* bfd_openr_iovec OPEN_CLOSURE data for gdb_bfd_open.  */
> +struct gdb_bfd_open_closure
> +{
> +  inferior *inf;
> +  bool warn_if_slow;
> +};
> +
>  /* Wrapper for target_fileio_open suitable for passing as the
> -   OPEN_FUNC argument to gdb_bfd_openr_iovec.  The supplied
> -   OPEN_CLOSURE is unused.  */
> +   OPEN_FUNC argument to gdb_bfd_openr_iovec.  */
>  
>  static void *
> -gdb_bfd_iovec_fileio_open (struct bfd *abfd, void *inferior)
> +gdb_bfd_iovec_fileio_open (struct bfd *abfd, void *open_closure)
>  {
>    const char *filename = bfd_get_filename (abfd);
>    int fd, target_errno;
>    int *stream;
> +  gdb_bfd_open_closure *oclosure = (gdb_bfd_open_closure *) open_closure;
>  
>    gdb_assert (is_target_filename (filename));
>  
> -  fd = target_fileio_open ((struct inferior *) inferior,
> +  fd = target_fileio_open (oclosure->inf,
>  			   filename + strlen (TARGET_SYSROOT_PREFIX),
> -			   FILEIO_O_RDONLY, 0, true,
> +			   FILEIO_O_RDONLY, 0, oclosure->warn_if_slow,
>  			   &target_errno);
>    if (fd == -1)
>      {
> @@ -378,7 +385,8 @@ gdb_bfd_iovec_fileio_fstat (struct bfd *abfd, void *stream,
>  /* See gdb_bfd.h.  */
>  
>  gdb_bfd_ref_ptr
> -gdb_bfd_open (const char *name, const char *target, int fd)
> +gdb_bfd_open (const char *name, const char *target, int fd,
> +	      bool warn_if_slow)
>  {
>    hashval_t hash;
>    void **slot;
> @@ -392,9 +400,10 @@ gdb_bfd_open (const char *name, const char *target, int fd)
>  	{
>  	  gdb_assert (fd == -1);
>  
> +	  gdb_bfd_open_closure open_closure { current_inferior (), warn_if_slow };
>  	  return gdb_bfd_openr_iovec (name, target,
>  				      gdb_bfd_iovec_fileio_open,
> -				      current_inferior (),
> +				      &open_closure,
>  				      gdb_bfd_iovec_fileio_pread,
>  				      gdb_bfd_iovec_fileio_close,
>  				      gdb_bfd_iovec_fileio_fstat);
> diff --git a/gdb/gdb_bfd.h b/gdb/gdb_bfd.h
> index 532520e0f7d..a941b79fe73 100644
> --- a/gdb/gdb_bfd.h
> +++ b/gdb/gdb_bfd.h
> @@ -78,10 +78,12 @@ typedef gdb::ref_ptr<struct bfd, gdb_bfd_ref_policy> gdb_bfd_ref_ptr;
>     If the BFD was not accessed using target fileio operations then the
>     filename associated with the BFD and accessible with
>     bfd_get_filename will not be exactly NAME but rather NAME with
> -   TARGET_SYSROOT_PREFIX stripped.  */
> +   TARGET_SYSROOT_PREFIX stripped.  If WARN_IF_SLOW is true, print a
> +   warning message if the file is being accessed over a link that may
> +   be slow.  */
>  
>  gdb_bfd_ref_ptr gdb_bfd_open (const char *name, const char *target,
> -			      int fd = -1);
> +			      int fd = -1, bool warn_if_slow = true);
>  
>  /* Mark the CHILD BFD as being a member of PARENT.  Also, increment
>     the reference count of CHILD.  Calling this function ensures that
> 
> base-commit: 7cfd74cfc6e14034779e6cc048c68877b7a08f88
> prerequisite-patch-id: b945b532dec51eb5ae7fdb9b8fe9007deca8d276
> prerequisite-patch-id: 6730601601e24970f31193c533a7c257c7e1c48c