* [PATCH 1/3] Default gdb_bfd_open's fd parameter to -1
2020-05-17 18:04 [PATCH 0/3] Make exec-file-mismatch compare build IDs Pedro Alves
@ 2020-05-17 18:04 ` Pedro Alves
2020-05-17 18:04 ` [PATCH 2/3] Eliminate target_fileio_open_warn_if_slow Pedro Alves
2020-05-17 18:04 ` [PATCH 3/3] Make exec-file-mismatch compare build IDs Pedro Alves
2 siblings, 0 replies; 15+ messages in thread
From: Pedro Alves @ 2020-05-17 18:04 UTC (permalink / raw)
To: gdb-patches
A following patch will add one more defaulted parameter.
gdb/ChangeLog:
yyyy-mm-dd Pedro Alves <palves@redhat.com>
* gdb_bfd.h: (gdb_bfd_open): Default to 'fd' parameter to -1.
Adjust all callers.
---
gdb/build-id.c | 2 +-
gdb/compile/compile-object-load.c | 2 +-
gdb/dwarf2/read.c | 4 ++--
gdb/gdb_bfd.h | 3 ++-
gdb/machoread.c | 4 ++--
gdb/solib-darwin.c | 2 +-
gdb/symfile.c | 6 +++---
gdb/windows-nat.c | 2 +-
gdb/windows-tdep.c | 2 +-
9 files changed, 14 insertions(+), 13 deletions(-)
diff --git a/gdb/build-id.c b/gdb/build-id.c
index 6e1f8b0f0cc..2f1afbd8e18 100644
--- a/gdb/build-id.c
+++ b/gdb/build-id.c
@@ -94,7 +94,7 @@ build_id_to_debug_bfd_1 (const std::string &link, size_t build_id_len,
}
/* We expect to be silent on the non-existing files. */
- gdb_bfd_ref_ptr debug_bfd = gdb_bfd_open (filename.get (), gnutarget, -1);
+ gdb_bfd_ref_ptr debug_bfd = gdb_bfd_open (filename.get (), gnutarget);
if (debug_bfd == NULL)
{
diff --git a/gdb/compile/compile-object-load.c b/gdb/compile/compile-object-load.c
index 55a46a31dba..76a9418dac0 100644
--- a/gdb/compile/compile-object-load.c
+++ b/gdb/compile/compile-object-load.c
@@ -604,7 +604,7 @@ compile_object_load (const compile_file_names &file_names,
gdb::unique_xmalloc_ptr<char> filename
(tilde_expand (file_names.object_file ()));
- gdb_bfd_ref_ptr abfd (gdb_bfd_open (filename.get (), gnutarget, -1));
+ gdb_bfd_ref_ptr abfd (gdb_bfd_open (filename.get (), gnutarget));
if (abfd == NULL)
error (_("\"%s\": could not open as compiled module: %s"),
filename.get (), bfd_errmsg (bfd_get_error ()));
diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c
index 719051bc5b2..0c6182bbf3b 100644
--- a/gdb/dwarf2/read.c
+++ b/gdb/dwarf2/read.c
@@ -2115,7 +2115,7 @@ dwarf2_get_dwz_file (struct dwarf2_per_objfile *dwarf2_per_objfile)
/* First try the file name given in the section. If that doesn't
work, try to use the build-id instead. */
- gdb_bfd_ref_ptr dwz_bfd (gdb_bfd_open (filename, gnutarget, -1));
+ gdb_bfd_ref_ptr dwz_bfd (gdb_bfd_open (filename, gnutarget));
if (dwz_bfd != NULL)
{
if (!build_id_verify (dwz_bfd.get (), buildid_len, buildid))
@@ -2138,7 +2138,7 @@ dwarf2_get_dwz_file (struct dwarf2_per_objfile *dwarf2_per_objfile)
if (fd.get () >= 0)
{
/* File successfully retrieved from server. */
- dwz_bfd = gdb_bfd_open (alt_filename.get (), gnutarget, -1);
+ dwz_bfd = gdb_bfd_open (alt_filename.get (), gnutarget);
if (dwz_bfd == nullptr)
warning (_("File \"%s\" from debuginfod cannot be opened as bfd"),
diff --git a/gdb/gdb_bfd.h b/gdb/gdb_bfd.h
index ce72f78a23f..532520e0f7d 100644
--- a/gdb/gdb_bfd.h
+++ b/gdb/gdb_bfd.h
@@ -80,7 +80,8 @@ typedef gdb::ref_ptr<struct bfd, gdb_bfd_ref_policy> gdb_bfd_ref_ptr;
bfd_get_filename will not be exactly NAME but rather NAME with
TARGET_SYSROOT_PREFIX stripped. */
-gdb_bfd_ref_ptr gdb_bfd_open (const char *name, const char *target, int fd);
+gdb_bfd_ref_ptr gdb_bfd_open (const char *name, const char *target,
+ int fd = -1);
/* Mark the CHILD BFD as being a member of PARENT. Also, increment
the reference count of CHILD. Calling this function ensures that
diff --git a/gdb/machoread.c b/gdb/machoread.c
index 4655b67f118..fb4e205088a 100644
--- a/gdb/machoread.c
+++ b/gdb/machoread.c
@@ -639,7 +639,7 @@ macho_symfile_read_all_oso (std::vector<oso_el> *oso_vector_ptr,
/* Open the archive and check the format. */
gdb_bfd_ref_ptr archive_bfd (gdb_bfd_open (archive_name.c_str (),
- gnutarget, -1));
+ gnutarget));
if (archive_bfd == NULL)
{
warning (_("Could not open OSO archive file \"%s\""),
@@ -705,7 +705,7 @@ macho_symfile_read_all_oso (std::vector<oso_el> *oso_vector_ptr,
}
else
{
- gdb_bfd_ref_ptr abfd (gdb_bfd_open (oso->name, gnutarget, -1));
+ gdb_bfd_ref_ptr abfd (gdb_bfd_open (oso->name, gnutarget));
if (abfd == NULL)
warning (_("`%s': can't open to read symbols: %s."), oso->name,
bfd_errmsg (bfd_get_error ()));
diff --git a/gdb/solib-darwin.c b/gdb/solib-darwin.c
index 03d91d1690a..e740a41fa4f 100644
--- a/gdb/solib-darwin.c
+++ b/gdb/solib-darwin.c
@@ -436,7 +436,7 @@ darwin_get_dyld_bfd ()
return NULL;
/* Create a bfd for the interpreter. */
- gdb_bfd_ref_ptr dyld_bfd (gdb_bfd_open (interp_name, gnutarget, -1));
+ gdb_bfd_ref_ptr dyld_bfd (gdb_bfd_open (interp_name, gnutarget));
if (dyld_bfd != NULL)
{
gdb_bfd_ref_ptr sub
diff --git a/gdb/symfile.c b/gdb/symfile.c
index 946443f07a8..fc984d81a1d 100644
--- a/gdb/symfile.c
+++ b/gdb/symfile.c
@@ -1278,7 +1278,7 @@ separate_debug_file_exists (const std::string &name, unsigned long crc,
gdb_flush (gdb_stdout);
}
- gdb_bfd_ref_ptr abfd (gdb_bfd_open (name.c_str (), gnutarget, -1));
+ gdb_bfd_ref_ptr abfd (gdb_bfd_open (name.c_str (), gnutarget));
if (abfd == NULL)
{
@@ -2042,7 +2042,7 @@ generic_load (const char *args, int from_tty)
}
/* Open the file for loading. */
- gdb_bfd_ref_ptr loadfile_bfd (gdb_bfd_open (filename.get (), gnutarget, -1));
+ gdb_bfd_ref_ptr loadfile_bfd (gdb_bfd_open (filename.get (), gnutarget));
if (loadfile_bfd == NULL)
perror_with_name (filename.get ());
@@ -2527,7 +2527,7 @@ reread_symbols (void)
obfd_filename = bfd_get_filename (objfile->obfd);
/* Open the new BFD before freeing the old one, so that
the filename remains live. */
- gdb_bfd_ref_ptr temp (gdb_bfd_open (obfd_filename, gnutarget, -1));
+ gdb_bfd_ref_ptr temp (gdb_bfd_open (obfd_filename, gnutarget));
objfile->obfd = temp.release ();
if (objfile->obfd == NULL)
error (_("Can't open %s to read symbols."), obfd_filename);
diff --git a/gdb/windows-nat.c b/gdb/windows-nat.c
index f52af9a1274..3452f6c827f 100644
--- a/gdb/windows-nat.c
+++ b/gdb/windows-nat.c
@@ -834,7 +834,7 @@ windows_make_so (const char *name, LPVOID load_addr)
{
asection *text = NULL;
- gdb_bfd_ref_ptr abfd (gdb_bfd_open (so->so_name, "pei-i386", -1));
+ gdb_bfd_ref_ptr abfd (gdb_bfd_open (so->so_name, "pei-i386"));
if (abfd == NULL)
return so;
diff --git a/gdb/windows-tdep.c b/gdb/windows-tdep.c
index 086038af0d4..20a18e6b683 100644
--- a/gdb/windows-tdep.c
+++ b/gdb/windows-tdep.c
@@ -537,7 +537,7 @@ windows_xfer_shared_library (const char* so_name, CORE_ADDR load_addr,
if (!text_offset)
{
- gdb_bfd_ref_ptr dll (gdb_bfd_open (so_name, gnutarget, -1));
+ gdb_bfd_ref_ptr dll (gdb_bfd_open (so_name, gnutarget));
/* The following calls are OK even if dll is NULL.
The default value 0x1000 is returned by pe_text_section_offset
in that case. */
--
2.14.5
^ permalink raw reply [flat|nested] 15+ messages in thread* [PATCH 2/3] Eliminate target_fileio_open_warn_if_slow
2020-05-17 18:04 [PATCH 0/3] Make exec-file-mismatch compare build IDs Pedro Alves
2020-05-17 18:04 ` [PATCH 1/3] Default gdb_bfd_open's fd parameter to -1 Pedro Alves
@ 2020-05-17 18:04 ` Pedro Alves
2020-05-17 18:04 ` [PATCH 3/3] Make exec-file-mismatch compare build IDs Pedro Alves
2 siblings, 0 replies; 15+ messages in thread
From: Pedro Alves @ 2020-05-17 18:04 UTC (permalink / raw)
To: gdb-patches
This basically makes target_fileio_open_1 extern, renamed to
target_fileio_open, and eliminates the current
target_fileio_open_warn_if_slow and target_fileio_open.
A following parameter will want to change gdb_bfd_iovec_fileio_open,
the only caller of target_fileio_open_warn_if_slow, to pass down
"warn_if_slow" true/false from the caller, instead of hardcoding
"warn_if_slow" true.
gdb/ChangeLog:
yyyy-mm-dd Pedro Alves <palves@redhat.com>
* gdb_bfd.c (gdb_bfd_iovec_fileio_open): Adjust.
* target.c (target_fileio_open_1): Rename to target_fileio_open
and make extern. Use bool.
(target_fileio_open, target_fileio_open_warn_if_slow): Delete.
(target_fileio_read_alloc_1): Adjust.
* target.h (target_fileio_open): Add 'warn_if_slow' parameter.
(target_fileio_open_warn_if_slow): Delete declaration.
---
gdb/gdb_bfd.c | 9 ++++-----
gdb/target.c | 33 +++++----------------------------
gdb/target.h | 20 +++++++-------------
3 files changed, 16 insertions(+), 46 deletions(-)
diff --git a/gdb/gdb_bfd.c b/gdb/gdb_bfd.c
index 26262bfedf6..61872a0bf3d 100644
--- a/gdb/gdb_bfd.c
+++ b/gdb/gdb_bfd.c
@@ -284,11 +284,10 @@ gdb_bfd_iovec_fileio_open (struct bfd *abfd, void *inferior)
gdb_assert (is_target_filename (filename));
- fd = target_fileio_open_warn_if_slow ((struct inferior *) inferior,
- filename
- + strlen (TARGET_SYSROOT_PREFIX),
- FILEIO_O_RDONLY, 0,
- &target_errno);
+ fd = target_fileio_open ((struct inferior *) inferior,
+ filename + strlen (TARGET_SYSROOT_PREFIX),
+ FILEIO_O_RDONLY, 0, true,
+ &target_errno);
if (fd == -1)
{
errno = fileio_errno_to_host (target_errno);
diff --git a/gdb/target.c b/gdb/target.c
index 6982a806e3e..82c405a8491 100644
--- a/gdb/target.c
+++ b/gdb/target.c
@@ -2775,13 +2775,11 @@ target_ops::fileio_readlink (struct inferior *inf, const char *filename,
return {};
}
-/* Helper for target_fileio_open and
- target_fileio_open_warn_if_slow. */
+/* See target.h. */
-static int
-target_fileio_open_1 (struct inferior *inf, const char *filename,
- int flags, int mode, int warn_if_slow,
- int *target_errno)
+int
+target_fileio_open (struct inferior *inf, const char *filename,
+ int flags, int mode, bool warn_if_slow, int *target_errno)
{
for (target_ops *t = default_fileio_target (); t != NULL; t = t->beneath ())
{
@@ -2813,27 +2811,6 @@ target_fileio_open_1 (struct inferior *inf, const char *filename,
/* See target.h. */
-int
-target_fileio_open (struct inferior *inf, const char *filename,
- int flags, int mode, int *target_errno)
-{
- return target_fileio_open_1 (inf, filename, flags, mode, 0,
- target_errno);
-}
-
-/* See target.h. */
-
-int
-target_fileio_open_warn_if_slow (struct inferior *inf,
- const char *filename,
- int flags, int mode, int *target_errno)
-{
- return target_fileio_open_1 (inf, filename, flags, mode, 1,
- target_errno);
-}
-
-/* See target.h. */
-
int
target_fileio_pwrite (int fd, const gdb_byte *write_buf, int len,
ULONGEST offset, int *target_errno)
@@ -3036,7 +3013,7 @@ target_fileio_read_alloc_1 (struct inferior *inf, const char *filename,
int target_errno;
scoped_target_fd fd (target_fileio_open (inf, filename, FILEIO_O_RDONLY,
- 0700, &target_errno));
+ 0700, false, &target_errno));
if (fd.get () == -1)
return -1;
diff --git a/gdb/target.h b/gdb/target.h
index 9a1dd805af9..37bfb29882a 100644
--- a/gdb/target.h
+++ b/gdb/target.h
@@ -2135,21 +2135,15 @@ extern int target_search_memory (CORE_ADDR start_addr,
current_top_target ()->filesystem_is_local ()
/* Open FILENAME on the target, in the filesystem as seen by INF,
- using FLAGS and MODE. If INF is NULL, use the filesystem seen
- by the debugger (GDB or, for remote targets, the remote stub).
- Return a target file descriptor, or -1 if an error occurs (and
- set *TARGET_ERRNO). */
+ using FLAGS and MODE. If INF is NULL, use the filesystem seen by
+ the debugger (GDB or, for remote targets, the remote stub). Return
+ a target file descriptor, or -1 if an error occurs (and set
+ *TARGET_ERRNO). If WARN_IF_SLOW is true, print a warning message
+ if the file is being accessed over a link that may be slow. */
extern int target_fileio_open (struct inferior *inf,
const char *filename, int flags,
- int mode, int *target_errno);
-
-/* Like target_fileio_open, but print a warning message if the
- file is being accessed over a link that may be slow. */
-extern int target_fileio_open_warn_if_slow (struct inferior *inf,
- const char *filename,
- int flags,
- int mode,
- int *target_errno);
+ int mode, bool warn_if_slow,
+ int *target_errno);
/* Write up to LEN bytes from WRITE_BUF to FD on the target.
Return the number of bytes written, or -1 if an error occurs
--
2.14.5
^ permalink raw reply [flat|nested] 15+ messages in thread* [PATCH 3/3] Make exec-file-mismatch compare build IDs
2020-05-17 18:04 [PATCH 0/3] Make exec-file-mismatch compare build IDs Pedro Alves
2020-05-17 18:04 ` [PATCH 1/3] Default gdb_bfd_open's fd parameter to -1 Pedro Alves
2020-05-17 18:04 ` [PATCH 2/3] Eliminate target_fileio_open_warn_if_slow Pedro Alves
@ 2020-05-17 18:04 ` Pedro Alves
2020-05-17 18:25 ` Eli Zaretskii
2020-05-18 13:55 ` [PATCH v2 " Pedro Alves
2 siblings, 2 replies; 15+ messages in thread
From: Pedro Alves @ 2020-05-17 18:04 UTC (permalink / raw)
To: gdb-patches
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.
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.
* 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 | 23 +++++++++++++----------
gdb/NEWS | 15 +++++++--------
gdb/exec.c | 41 +++++++++++++++++++++++++++++++++++++++--
gdb/gdb_bfd.c | 23 ++++++++++++++++-------
gdb/gdb_bfd.h | 6 ++++--
5 files changed, 79 insertions(+), 29 deletions(-)
diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index 8f3301259af..aaddfa9b885 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -2909,22 +2909,24 @@ 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 build IDs are not
+present in either file, @value{GDBN} compares the file names. The
+@code{exec-file-mismatch} option specifies how to handle a mismatch.
@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 +20876,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..c05a1be1f1b 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, false));
+ 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"
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
--
2.14.5
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH 3/3] Make exec-file-mismatch compare build IDs
2020-05-17 18:04 ` [PATCH 3/3] Make exec-file-mismatch compare build IDs Pedro Alves
@ 2020-05-17 18:25 ` Eli Zaretskii
2020-05-18 13:52 ` Pedro Alves
2020-05-18 13:55 ` [PATCH v2 " Pedro Alves
1 sibling, 1 reply; 15+ messages in thread
From: Eli Zaretskii @ 2020-05-17 18:25 UTC (permalink / raw)
To: Pedro Alves; +Cc: gdb-patches
> From: Pedro Alves via Gdb-patches <gdb-patches@sourceware.org>
>
> +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 build IDs are not
> +present in either file, @value{GDBN} compares the file names. The
> +@code{exec-file-mismatch} option specifies how to handle a mismatch.
The last sentence is a repetition of what was already said a few
sentences back.
The documentation parts are okay with this nit fixed. Thanks.
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH 3/3] Make exec-file-mismatch compare build IDs
2020-05-17 18:25 ` Eli Zaretskii
@ 2020-05-18 13:52 ` Pedro Alves
0 siblings, 0 replies; 15+ messages in thread
From: Pedro Alves @ 2020-05-18 13:52 UTC (permalink / raw)
To: Eli Zaretskii; +Cc: gdb-patches
On 5/17/20 7:25 PM, Eli Zaretskii wrote:
>> From: Pedro Alves via Gdb-patches <gdb-patches@sourceware.org>
>>
>> +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 build IDs are not
>> +present in either file, @value{GDBN} compares the file names. The
>> +@code{exec-file-mismatch} option specifies how to handle a mismatch.
>
> The last sentence is a repetition of what was already said a few
> sentences back.
>
> The documentation parts are okay with this nit fixed. Thanks.
Thanks, I've removed that sentence.
Thanks,
Pedro Alves
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH v2 3/3] Make exec-file-mismatch compare build IDs
2020-05-17 18:04 ` [PATCH 3/3] Make exec-file-mismatch compare build IDs Pedro Alves
2020-05-17 18:25 ` Eli Zaretskii
@ 2020-05-18 13:55 ` Pedro Alves
2020-05-18 14:21 ` Philippe Waroquiers
2020-05-19 16:04 ` Tom Tromey
1 sibling, 2 replies; 15+ messages in thread
From: Pedro Alves @ 2020-05-18 13:55 UTC (permalink / raw)
To: gdb-patches
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
--
2.14.5
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH v2 3/3] Make exec-file-mismatch compare build IDs
2020-05-18 13:55 ` [PATCH v2 " Pedro Alves
@ 2020-05-18 14:21 ` Philippe Waroquiers
2020-05-19 15:43 ` Pedro Alves
2020-05-19 16:04 ` Tom Tromey
1 sibling, 1 reply; 15+ messages in thread
From: Philippe Waroquiers @ 2020-05-18 14:21 UTC (permalink / raw)
To: Pedro Alves, gdb-patches
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
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH v2 3/3] Make exec-file-mismatch compare build IDs
2020-05-18 14:21 ` Philippe Waroquiers
@ 2020-05-19 15:43 ` Pedro Alves
0 siblings, 0 replies; 15+ messages in thread
From: Pedro Alves @ 2020-05-19 15:43 UTC (permalink / raw)
To: Philippe Waroquiers, gdb-patches
On 5/18/20 3:21 PM, Philippe Waroquiers wrote:
>
> 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, completely forgot checking that!
I went with the removing "name" approach. I've applied this locally:
@@ -1250,8 +1250,8 @@ Set exec-file-mismatch handling (ask|warn|off)."),
_("\
Show exec-file-mismatch handling (ask|warn|off)."),
_("\
-Specifies how to handle a mismatch between the current exec-file name\n\
-loaded by GDB and the exec-file name automatically determined when attaching\n\
+Specifies how to handle a mismatch between the current exec-file\n\
+loaded by GDB and the exec-file automatically determined when attaching\n\
to a process:\n\n\
ask - warn the user and ask whether to load the determined exec-file.\n\
warn - warn the user, but do not change the exec-file.\n\
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2 3/3] Make exec-file-mismatch compare build IDs
2020-05-18 13:55 ` [PATCH v2 " Pedro Alves
2020-05-18 14:21 ` Philippe Waroquiers
@ 2020-05-19 16:04 ` Tom Tromey
2020-05-19 17:44 ` Pedro Alves
1 sibling, 1 reply; 15+ messages in thread
From: Tom Tromey @ 2020-05-19 16:04 UTC (permalink / raw)
To: Pedro Alves via Gdb-patches; +Cc: Pedro Alves
>>>>> "Pedro" == Pedro Alves via Gdb-patches <gdb-patches@sourceware.org> writes:
Pedro> 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.
Agreed.
Pedro> Here's an updated version that removes the filename comparison.
Pedro> I've kept the structure of the code the same, in case we
Pedro> add some form of fallback later on.
It makes sense to me.
I read these patches and they look good to me.
Tom
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2 3/3] Make exec-file-mismatch compare build IDs
2020-05-19 16:04 ` Tom Tromey
@ 2020-05-19 17:44 ` Pedro Alves
2020-05-21 13:32 ` Philippe Waroquiers
0 siblings, 1 reply; 15+ messages in thread
From: Pedro Alves @ 2020-05-19 17:44 UTC (permalink / raw)
To: Tom Tromey, Pedro Alves via Gdb-patches, Philippe Waroquiers
On 5/19/20 5:04 PM, Tom Tromey wrote:
>>>>>> "Pedro" == Pedro Alves via Gdb-patches <gdb-patches@sourceware.org> writes:
>
> Pedro> 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.
>
> Agreed.
>
> Pedro> Here's an updated version that removes the filename comparison.
> Pedro> I've kept the structure of the code the same, in case we
> Pedro> add some form of fallback later on.
>
> It makes sense to me.
> I read these patches and they look good to me.
Thanks all for the reviews and discussions. I've merged this now.
Pedro Alves
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2 3/3] Make exec-file-mismatch compare build IDs
2020-05-19 17:44 ` Pedro Alves
@ 2020-05-21 13:32 ` Philippe Waroquiers
2020-05-21 14:02 ` Pedro Alves
0 siblings, 1 reply; 15+ messages in thread
From: Philippe Waroquiers @ 2020-05-21 13:32 UTC (permalink / raw)
To: Pedro Alves, Tom Tromey, Pedro Alves via Gdb-patches
On Tue, 2020-05-19 at 18:44 +0100, Pedro Alves wrote:
> On 5/19/20 5:04 PM, Tom Tromey wrote:
> > > > > > > "Pedro" == Pedro Alves via Gdb-patches <gdb-patches@sourceware.org> writes:
> >
> > Pedro> 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.
> >
> > Agreed.
> >
> > Pedro> Here's an updated version that removes the filename comparison.
> > Pedro> I've kept the structure of the code the same, in case we
> > Pedro> add some form of fallback later on.
> >
> > It makes sense to me.
> > I read these patches and they look good to me.
>
> Thanks all for the reviews and discussions. I've merged this now.
Comparing build-id introduced a change of behaviour when GDB has
loaded a file, and the user recompiles this file followed by an attach.
Before this patch, when GDB had a file loaded and the user recompiled
the file and attached to a process using this recompiled file,
GDB used to reload the file automatically.
Now, GDB reports a mismatch, indicating a "mismatch"
instead of automatically re-loading the new file version.
Technically, GDB mismatch is correct, but I am wondering if this change
of behaviour is desirable. At least, it introduces a difference
of behaviour between run (that will just indicate the file has changed
and reload automatically) and attach (that will warn and ask).
(gdb) atta 10615
Attaching to program: /bd/home/philippe/gdb/git/build_moreaa/gdb/gdb, process 10615
[New LWP 10616]
[New LWP 10617]
[New LWP 10618]
[New LWP 10620]
[New LWP 10621]
[New LWP 10622]
[New LWP 10623]
warning: Mismatch between current exec-file /bd/home/philippe/gdb/git/build_moreaa/gdb/gdb
and automatically determined exec-file /bd/home/philippe/gdb/git/build_moreaa/gdb/gdb
exec-file-mismatch handling is currently "ask"
Load new symbol table from "/bd/home/philippe/gdb/git/build_moreaa/gdb/gdb"? (y or n)
while previously, GDB was doing:
(gdb) atta 14099
Attaching to program: /bd/home/philippe/gdb/git/build_moreaa/gdb/gdb, process 14099
[New LWP 14100]
[New LWP 14101]
[New LWP 14102]
[New LWP 14104]
[New LWP 14105]
[New LWP 14106]
[New LWP 14107]
`/bd/home/philippe/gdb/git/build_moreaa/gdb/gdb' has changed; re-reading symbols.
Philippe
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2 3/3] Make exec-file-mismatch compare build IDs
2020-05-21 13:32 ` Philippe Waroquiers
@ 2020-05-21 14:02 ` Pedro Alves
2020-05-21 14:23 ` Philippe Waroquiers
0 siblings, 1 reply; 15+ messages in thread
From: Pedro Alves @ 2020-05-21 14:02 UTC (permalink / raw)
To: Philippe Waroquiers, Tom Tromey, Pedro Alves via Gdb-patches
On 5/21/20 2:32 PM, Philippe Waroquiers wrote:
> Comparing build-id introduced a change of behaviour when GDB has
> loaded a file, and the user recompiles this file followed by an attach.
>
> Before this patch, when GDB had a file loaded and the user recompiled
> the file and attached to a process using this recompiled file,
> GDB used to reload the file automatically.
> Now, GDB reports a mismatch, indicating a "mismatch"
> instead of automatically re-loading the new file version.
>
> Technically, GDB mismatch is correct, but I am wondering if this change
> of behaviour is desirable. At least, it introduces a difference
> of behaviour between run (that will just indicate the file has changed
> and reload automatically) and attach (that will warn and ask).
>
>
> (gdb) atta 10615
> Attaching to program: /bd/home/philippe/gdb/git/build_moreaa/gdb/gdb, process 10615
> [New LWP 10616]
> [New LWP 10617]
> [New LWP 10618]
> [New LWP 10620]
> [New LWP 10621]
> [New LWP 10622]
> [New LWP 10623]
> warning: Mismatch between current exec-file /bd/home/philippe/gdb/git/build_moreaa/gdb/gdb
> and automatically determined exec-file /bd/home/philippe/gdb/git/build_moreaa/gdb/gdb
> exec-file-mismatch handling is currently "ask"
> Load new symbol table from "/bd/home/philippe/gdb/git/build_moreaa/gdb/gdb"? (y or n)
>
>
> while previously, GDB was doing:
> (gdb) atta 14099
> Attaching to program: /bd/home/philippe/gdb/git/build_moreaa/gdb/gdb, process 14099
> [New LWP 14100]
> [New LWP 14101]
> [New LWP 14102]
> [New LWP 14104]
> [New LWP 14105]
> [New LWP 14106]
> [New LWP 14107]
> `/bd/home/philippe/gdb/git/build_moreaa/gdb/gdb' has changed; re-reading symbols.
It looks to me like GDB is doing the validation too soon. It should figure out
that the local file changed, and thus reload it (the "re-reading symbols"), and only
after, should it do validation? After re-reading, the build ids should match again.
Or do the "has changed, re-reading" thing earlier, but not sure that's practical.
Thanks,
Pedro Alves
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2 3/3] Make exec-file-mismatch compare build IDs
2020-05-21 14:02 ` Pedro Alves
@ 2020-05-21 14:23 ` Philippe Waroquiers
2020-05-21 14:27 ` Pedro Alves
0 siblings, 1 reply; 15+ messages in thread
From: Philippe Waroquiers @ 2020-05-21 14:23 UTC (permalink / raw)
To: Pedro Alves, Tom Tromey, Pedro Alves via Gdb-patches
On Thu, 2020-05-21 at 15:02 +0100, Pedro Alves wrote:
> It looks to me like GDB is doing the validation too soon. It should figure out
> that the local file changed, and thus reload it (the "re-reading symbols"), and only
> after, should it do validation? After re-reading, the build ids should match again.
> Or do the "has changed, re-reading" thing earlier, but not sure that's practical.
Yes, doing the build-id comparison later should solve that behaviour.
If you want, I can take a look at this, sometime this week-end probably.
Thanks
Philippe
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2 3/3] Make exec-file-mismatch compare build IDs
2020-05-21 14:23 ` Philippe Waroquiers
@ 2020-05-21 14:27 ` Pedro Alves
0 siblings, 0 replies; 15+ messages in thread
From: Pedro Alves @ 2020-05-21 14:27 UTC (permalink / raw)
To: Philippe Waroquiers, Tom Tromey, Pedro Alves via Gdb-patches
On 5/21/20 3:23 PM, Philippe Waroquiers wrote:
> On Thu, 2020-05-21 at 15:02 +0100, Pedro Alves wrote:
>> It looks to me like GDB is doing the validation too soon. It should figure out
>> that the local file changed, and thus reload it (the "re-reading symbols"), and only
>> after, should it do validation? After re-reading, the build ids should match again.
>> Or do the "has changed, re-reading" thing earlier, but not sure that's practical.
>
> Yes, doing the build-id comparison later should solve that behaviour.
> If you want, I can take a look at this, sometime this week-end probably.
That would be super!
Thanks,
Pedro Alves
^ permalink raw reply [flat|nested] 15+ messages in thread