Re: exec-file-mismatch and native-gdbserver testing

[Pedro Alves <palves@redhat.com>]

On 4/8/20 3:02 PM, Metzger, Markus T via Gdb wrote:
> Hello,
> 
> I noticed an issue when running tests that use standard_temp_file with the native-gdbserver board.
> 
> In gdb_remote_download, when called without tofile argument, as it is, for example, when starting gdbserver via gdb_reload, we set the filename to standard_output_file [file tail $fromfile] and copy the file.
> 
> GDB and gdbserver now use a copy of the same file at different locations.
> 
> This triggers an exec-file-mismatch warning and, with the default “ask” setting, a user prompt that isn’t handled by the tests and eventually leads to a timeout.  This can be seen with all tests that use gdb_simple_compile, e.g. via skip_*_test calls.  An example would be gdb.btrace/*.exp.
> 
> In exec.c:validate_exec_file (), we check the filenames and, if they differ, print a warning and re-load the symbol file.
> 
> Should validate_exec_file () check more than just the filenames?
> Should gdb_simple_compile use standard_output_file instead of standard_temp_file?

It probably can't hurt to make gdb_simple_compile use standard_output_file
instead of standard_temp_file.  I've tried it locally and ran the testsuite
with that, and it indeed fixes most of the problem.  It avoids copying
the binary when testing with local gdbserver boards, so seems like a win
regardless.

Looking at the testresults made me realize that the new feature exposed
at least one bug, fixed here:
  https://sourceware.org/pipermail/gdb-patches/2020-May/168680.html

So I've been looking at the remaining failures that the gdb_simple_compile
change wouldn't fix, to look for other bugs.

The next one I'm looking at is gdb.base/argv0-symlink.exp.  When tested
with native-extended-gdbserver, we get:

(gdb) run 
Starting program: /home/pedro/brno/pedro/gdb/binutils-gdb/build/gdb/testsuite/outputs/gdb.base/argv0-symlink/argv0-symlink-filelink 
warning: Mismatch between current exec-file /home/pedro/brno/pedro/gdb/binutils-gdb/build/gdb/testsuite/outputs/gdb.base/argv0-symlink/argv0-symlink-filelink
and automatically determined exec-file /home/pedro/brno/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/brno/pedro/gdb/binutils-gdb/build/gdb/testsuite/outputs/gdb.base/argv0-symlink/argv0-symlink"? (y or n) 

I first thought that a quick fix at least for local boards could be to
make validate_exec_file resolve symlinks before comparing the filename.

But then, I thought -- how hard can it be to validate the build ids?

So I cooked up something.  Below's the resulting preliminary patch.

Seems to work nicely -- it fixes gdb.base/argv0-symlink.exp at least.
I haven't run the testsuite yet.

There's (at least) one issue that I'll need to fix.  It's to
get rid of the "transfers from remote targets can be slow" warning
when we open the remote file to read the build id:

 (gdb) r
 Starting program: /home/pedro/brno/pedro/gdb/binutils-gdb/build/gdb/testsuite/outputs/gdb.base/break/break 
 Reading /home/pedro/brno/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/brno/pedro/gdb/binutils-gdb/build/gdb/testsuite/outputs/gdb.base/break/break
 and automatically determined exec-file /home/pedro/brno/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/brno/pedro/gdb/binutils-gdb/build/gdb/testsuite/outputs/gdb.base/argv0-symlink/argv0-symlink"? (y or n) 

For that, I'll need to pass down a struct with a warn_if_slow field
as closure to gdb_bfd_openr_iovec, I think.

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 is the read-ahead cache, I think.  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 that.

From 51124c1ae7dde0c7ba033b4f841777b7d523c864 Mon Sep 17 00:00:00 2001
From: Pedro Alves <palves@redhat.com>
Date: Sat, 16 May 2020 20:56:54 +0100
Subject: [PATCH] validate_exec_file & build-id

---
 gdb/exec.c | 41 +++++++++++++++++++++++++++++++++++++++--
 1 file changed, 39 insertions(+), 2 deletions(-)

diff --git a/gdb/exec.c b/gdb/exec.c
index a2added5e22..04d94994894 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,11 +248,45 @@ 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;
 
+  /* 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));
+      if (abfd != nullptr)
+	{
+	  const bfd_build_id *target_exec_file_build_id
+	    = build_id_bfd_get (abfd.get ());
+
+	  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;
+	    }
+	}
+    }
+
   std::string exec_file_target (pid_exec_file);
 
   /* In case the exec file is not local, exec_file_target has to point at
@@ -259,7 +294,9 @@ validate_exec_file (int from_tty)
   if (is_target_filename (current_exec_file) && !target_filesystem_is_local ())
     exec_file_target = TARGET_SYSROOT_PREFIX + exec_file_target;
 
-  if (exec_file_target != current_exec_file)
+  /* If build-id validation wasn't possible, fallback to validating by
+     filename.  */
+  if (build_id_mismatch || exec_file_target != current_exec_file)
     {
       warning
 	(_("Mismatch between current exec-file %ps\n"

base-commit: 7cfd74cfc6e14034779e6cc048c68877b7a08f88
-- 
2.14.5