diff mbox

[v2] Make only user-specified executable and symbol filenames sticky

Message ID 1433503346-22517-1-git-send-email-gbenson@redhat.com
State New
Headers show

Commit Message

Gary Benson June 5, 2015, 11:22 a.m. UTC
Joel Brobecker wrote:
> After quickly going through the discussion, I tend to agree.
> How about:
>   (1) Provide a way to recover the situation _after_ the "attach";
>   (2) Print a warning if the attach notices that the executable name
>       does not match the one given by the user, and include the
>       procedure for fixing it if the user made an error.
> ?
> 
> We could do this in two steps:
> 
>   a. Push Gary's patch today, after having enhanced it to print
>      a warning when discrepancies are discovered;
> 
>   b. Work on a way to achieve (1), and then enhance the warning
>      to mention how to resolve this issue.
> 
> That way, we could have (a) in time for 7.10, and work for (b)
> without delaying 7.10. Whether (b) makes it to 7.10 will then
> depend on how quickly we find a solution, but it's OK IMO for it
> to provide that in a later release.

Ok, in the interests of getting this into 7.10, here's an updated
patch.  I'm not sure I've addressed everything but I think it's
what you're after right now.  Main changes from v1 are:

 - I renamed user_supplied_exec_file_p as exec_file_is_user_supplied
   so it has the same "exec_file_" prefix as other exec-file-related
   values.

 - I introduced another field, symfile_objfile_is_user_supplied,
   which does much the same as exec_file_is_user_supplied but for
   the symbol file.

 - I added warnings if GDB detects that the wrong filename is being
   used for either the executable file or the symbol file.

Don's query about follow-exec-mode is still outstanding (I don't know
where to look to check this).

Built and regtested on RHEL6.6 x86_64.

Should I commit?

Thanks,
Gary

--
On attach, GDB will only attempt to determine the main executable's
filename if one is not already set.  This causes problems if you
attach to one process and then attach to another: GDB will not attempt
to discover the main executable on the second attach.  If the two
processes have different main executable files then the symbols will
now be wrong.  This is PR gdb/17626.

In GDB some filenames are supplied by the user (e.g. using the "file"
or "symbol-file" commands) and some are determined by GDB (e.g. while
processing an "attach" command).  This commit updates GDB to track
which filenames were supplied by the user.  When GDB might attempt to
determine an executable filename and one is already set, filenames
determined by GDB may be overridden but user-supplied filenames will
not.

gdb/ChangeLog:

	PR gdb/17626
	* progspace.h (struct program_space)
	<pspace_exec_file_is_user_supplied>: New field.
	<symfile_object_file_is_user_supplied>: Likewise.
	(symfile_objfile_is_user_supplied): New macro.
	* exec.h (exec_file_is_user_supplied): Likewise.
	* exec.c (exec_close): Clear exec_file_is_user_supplied.
	(exec_file_locate_attach): Remove get_exec_file check.
	Do not replace user-supplied executable or symbol files.
	Warn if user-supplied executable or symbol files do not
	match discovered file.
	(exec_file_command): Set exec_file_is_user_supplied.
	* symfile.c (symbol_file_clear): Clear
	symfile_objfile_is_user_supplied.
	(symbol_file_command): Set symfile_objfile_is_user_supplied.
	* inferior.c (add_inferior_command): Set
	exec_file_is_user_supplied and symfile_objfile_is_user_supplied.
	* main.c (captured_main): Likewise.
	* infcmd.c (attach_command_post_wait): Always call
	exec_file_locate_attach.  Only call reopen_exec_file and
	reread_symbols if exec_file_is_user_supplied.
	* remote.c (remote_add_inferior): Remove get_exec_file check
	around exec_file_locate_attach.
---
 gdb/ChangeLog   |   26 ++++++++++++++++++++++++++
 gdb/exec.c      |   30 +++++++++++++++++++++++-------
 gdb/exec.h      |    2 ++
 gdb/infcmd.c    |   13 ++++++++-----
 gdb/inferior.c  |    3 +++
 gdb/main.c      |   24 ++++++++++++++++--------
 gdb/progspace.h |   17 +++++++++++++++++
 gdb/remote.c    |    9 ++++++---
 gdb/symfile.c   |    3 +++
 9 files changed, 104 insertions(+), 23 deletions(-)

Comments

Philippe Waroquiers June 7, 2015, 12:03 p.m. UTC | #1
On Fri, 2015-06-05 at 12:22 +0100, Gary Benson wrote:
> Built and regtested on RHEL6.6 x86_64.
I also tested the patch (unmodified, i.e. keeping the !fake_pid_p test) 
with 'native' attach/detach.
I have encountered some problems.

Here is the test I am doing:
In one terminal, I launch
  /bin/sleep 10000
In another terminal, I am launching
 ./gdbserver_tests/sleepers 1000000 1000000 1000000 BSBSBSBS
(sleepers is a program used for testing Valgrind gdbsrv).

Then I used the patched GDB to attach first to sleep, then detach,
then attach to sleepers, then detach, then attach again to sleep.
The 3 attach does not work properly: it believes it has to use
the sleepers executable, asks to switch to this symbol file,
and then GDB locks, and I have to kill it.

Philippe


(gdb) atta 27434
Attaching to process 27434
Reading symbols from /bin/sleep...(no debugging symbols found)...done.
warning: Cannot call inferior functions on this system - Linux kernel
with broken i386 NX (non-executable pages) support detected!
Reading symbols from /lib/libc.so.6...(no debugging symbols
found)...done.
Reading symbols from /lib/ld-linux.so.2...(no debugging symbols
found)...done.
0x00dab416 in __kernel_vsyscall ()
(gdb) detach
Detaching from program: /bin/sleep, process 27434
(gdb) attach 27393
Attaching to program: /bin/sleep, process 27393
Reading symbols
from /home/philippe/valgrind/trunk_untouched/gdbserver_tests/sleepers...done.
Reading symbols from /lib/libpthread.so.0...(no debugging symbols
found)...done.
[New LWP 27396]
[New LWP 27395]
[New LWP 27394]
[Thread debugging using libthread_db enabled]
Using host libthread_db library "/lib/libthread_db.so.1".
Reading symbols from /lib/libc.so.6...(no debugging symbols
found)...done.
Reading symbols from /lib/ld-linux.so.2...(no debugging symbols
found)...done.
0x00843416 in __kernel_vsyscall ()
(gdb) detach
Detaching from
program: /home/philippe/valgrind/trunk_untouched/gdbserver_tests/sleepers, process 27393
(gdb) atta 27434
Attaching to
program: /home/philippe/valgrind/trunk_untouched/gdbserver_tests/sleepers, process 27434
Load new symbol table from "/bin/sleep"? (y or n) 
//////////// here GDB does not respond anymore.
Philippe Waroquiers June 7, 2015, 12:13 p.m. UTC | #2
On Fri, 2015-06-05 at 12:22 +0100, Gary Benson wrote:
> Built and regtested on RHEL6.6 x86_64.
Finally, here are some tests with GDB gdbserver.
There is also something strange.

I launch 2 gdbserver:
  gdbserver :1234 /bin/sleep 10000
and
  gdbserver :1235 ./gdbserver_tests/sleepers 1000000 1000000 1000000 BSBSBSBS

Then :
(gdb) tar rem :1234
Remote debugging using :1234
Reading symbols from target:/bin/sleep...(no debugging symbols found)...done.
Reading symbols from target:/lib/ld-linux.so.2...(no debugging symbols found)...done.
0x001f2850 in _start () from target:/lib/ld-linux.so.2
(gdb) detach
Detaching from program: target:/bin/sleep, process 27572
Ending remote debugging.
(gdb) tar rem :1235
`target:/bin/sleep' has disappeared; keeping its symbols.
Remote debugging using :1235
Reading symbols from target:/home/philippe/valgrind/trunk_untouched/gdbserver_tests/sleepers...Can't read symbols from target:/home/philippe/valgrind/trunk_untouched/gdbserver_tests/sleepers: Invalid argument
(gdb) 


So, as you can see, GDB cannot read the symbols for the second attach
(while it has properly discovered the exec-file).

Philippe
diff mbox

Patch

diff --git a/gdb/exec.c b/gdb/exec.c
index 8a4ab6f..11e5db0 100644
--- a/gdb/exec.c
+++ b/gdb/exec.c
@@ -102,6 +102,7 @@  exec_close (void)
 
       xfree (exec_filename);
       exec_filename = NULL;
+      exec_file_is_user_supplied = 0;
     }
 }
 
@@ -142,11 +143,6 @@  exec_file_locate_attach (int pid, int from_tty)
 {
   char *exec_file, *full_exec_path = NULL;
 
-  /* Do nothing if we already have an executable filename.  */
-  exec_file = (char *) get_exec_file (0);
-  if (exec_file != NULL)
-    return;
-
   /* Try to determine a filename from the process itself.  */
   exec_file = target_pid_to_exec_file (pid);
   if (exec_file == NULL)
@@ -171,8 +167,27 @@  exec_file_locate_attach (int pid, int from_tty)
 	full_exec_path = xstrdup (exec_file);
     }
 
-  exec_file_attach (full_exec_path, from_tty);
-  symbol_file_add_main (full_exec_path, from_tty);
+  if (exec_file_is_user_supplied)
+    {
+      if (strcmp (full_exec_path, exec_filename) != 0)
+	warning (_("Process %d has executable file %s,"
+		   " but executable file is currently set to %s"),
+		 pid, full_exec_path, exec_filename);
+    }
+  else
+    exec_file_attach (full_exec_path, from_tty);
+
+  if (symfile_objfile_is_user_supplied)
+    {
+      const char *symbol_filename = objfile_filename (symfile_objfile);
+
+      if (strcmp (full_exec_path, symbol_filename) != 0)
+	warning (_("Process %d has symbol file %s,"
+		   " but symbol file is currently set to %s"),
+		 pid, full_exec_path, symbol_filename);
+    }
+  else
+    symbol_file_add_main (full_exec_path, from_tty);
 }
 
 /* Set FILENAME as the new exec file.
@@ -376,6 +391,7 @@  exec_file_command (char *args, int from_tty)
       filename = tilde_expand (*argv);
       make_cleanup (xfree, filename);
       exec_file_attach (filename, from_tty);
+      exec_file_is_user_supplied = 1;
 
       do_cleanups (cleanups);
     }
diff --git a/gdb/exec.h b/gdb/exec.h
index c7f3b56..d984e97 100644
--- a/gdb/exec.h
+++ b/gdb/exec.h
@@ -32,6 +32,8 @@  struct objfile;
 #define exec_bfd current_program_space->ebfd
 #define exec_bfd_mtime current_program_space->ebfd_mtime
 #define exec_filename current_program_space->pspace_exec_filename
+#define exec_file_is_user_supplied \
+  current_program_space->pspace_exec_file_is_user_supplied
 
 /* Builds a section table, given args BFD, SECTABLE_PTR, SECEND_PTR.
    Returns 0 if OK, 1 on error.  */
diff --git a/gdb/infcmd.c b/gdb/infcmd.c
index 7e2484b..440871f 100644
--- a/gdb/infcmd.c
+++ b/gdb/infcmd.c
@@ -2467,11 +2467,14 @@  attach_command_post_wait (char *args, int from_tty, int async_exec)
   inferior = current_inferior ();
   inferior->control.stop_soon = NO_STOP_QUIETLY;
 
-  /* If no exec file is yet known, try to determine it from the
-     process itself.  */
-  if (get_exec_file (0) == NULL)
-    exec_file_locate_attach (ptid_get_pid (inferior_ptid), from_tty);
-  else
+  /* Attempt to open the file that was executed to create this
+     inferior.  If the user has explicitly specified executable
+     and/or symbol files then warn the user if their choices do
+     not match.  Otherwise, set exec_file and symfile_objfile to
+     the new file.  */
+  exec_file_locate_attach (ptid_get_pid (inferior_ptid), from_tty);
+
+  if (exec_file_is_user_supplied)
     {
       reopen_exec_file ();
       reread_symbols ();
diff --git a/gdb/inferior.c b/gdb/inferior.c
index ba320b5..30e1036 100644
--- a/gdb/inferior.c
+++ b/gdb/inferior.c
@@ -888,7 +888,10 @@  add_inferior_command (char *args, int from_tty)
 	  switch_to_thread (null_ptid);
 
 	  exec_file_attach (exec, from_tty);
+	  exec_file_is_user_supplied = 1;
+
 	  symbol_file_add_main (exec, from_tty);
+	  symfile_objfile_is_user_supplied = 1;
 	}
     }
 
diff --git a/gdb/main.c b/gdb/main.c
index 477fd68..3a68b0b 100644
--- a/gdb/main.c
+++ b/gdb/main.c
@@ -1051,17 +1051,25 @@  captured_main (void *data)
          catch_command_errors returns non-zero on success!  */
       if (catch_command_errors_const (exec_file_attach, execarg,
 				      !batch_flag))
-	catch_command_errors_const (symbol_file_add_main, symarg,
-				    !batch_flag);
+	{
+	  exec_file_is_user_supplied = 1;
+
+	  if (catch_command_errors_const (symbol_file_add_main, symarg,
+					  !batch_flag))
+	    symfile_objfile_is_user_supplied = 1;
+	}
     }
   else
     {
-      if (execarg != NULL)
-	catch_command_errors_const (exec_file_attach, execarg,
-				    !batch_flag);
-      if (symarg != NULL)
-	catch_command_errors_const (symbol_file_add_main, symarg,
-				    !batch_flag);
+      if (execarg != NULL
+	  && catch_command_errors_const (exec_file_attach, execarg,
+					 !batch_flag))
+	exec_file_is_user_supplied = 1;
+
+      if (symarg != NULL
+	  && catch_command_errors_const (symbol_file_add_main, symarg,
+					 !batch_flag))
+	symfile_objfile_is_user_supplied = 1;
     }
 
   if (corearg && pidarg)
diff --git a/gdb/progspace.h b/gdb/progspace.h
index f960093..561e093 100644
--- a/gdb/progspace.h
+++ b/gdb/progspace.h
@@ -154,6 +154,12 @@  struct program_space
        It needs to be freed by xfree.  It is not NULL iff EBFD is not NULL.  */
     char *pspace_exec_filename;
 
+    /* Nonzero if pspace_exec_filename was supplied by the user,
+       either at startup (on the command-line) or via the "file"
+       or "add-inferior -exec" commands.  Zero if
+       pspace_exec_filename is unset or was discovered by GDB.  */
+    int pspace_exec_file_is_user_supplied;
+
     /* The address space attached to this program space.  More than one
        program space may be bound to the same address space.  In the
        traditional unix-like debugging scenario, this will usually
@@ -183,6 +189,12 @@  struct program_space
        (e.g. the argument to the "symbol-file" or "file" command).  */
     struct objfile *symfile_object_file;
 
+    /* Nonzero if symfile_object_file was supplied by the user,
+       either at startup (on the command-line) or via the "file",
+       "symbol-file" or "add-inferior -exec" commands.  Zero if
+       symfile_object_file is unset or was discovered by GDB.  */
+    int symfile_object_file_is_user_supplied;
+
     /* All known objfiles are kept in a linked list.  This points to
        the head of this list.  */
     struct objfile *objfiles;
@@ -215,6 +227,11 @@  struct program_space
 
 #define symfile_objfile current_program_space->symfile_object_file
 
+/* See program_space->symfile_object_file_is_user_supplied.  */
+
+#define symfile_objfile_is_user_supplied \
+  current_program_space->symfile_object_file_is_user_supplied
+
 /* All known objfiles are kept in a linked list.  This points to the
    root of this list.  */
 #define object_files current_program_space->objfiles
diff --git a/gdb/remote.c b/gdb/remote.c
index 099ddbb..af6b8d8 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -1556,9 +1556,12 @@  remote_add_inferior (int fake_pid_p, int pid, int attached,
   inf->attach_flag = attached;
   inf->fake_pid_p = fake_pid_p;
 
-  /* If no main executable is currently open then attempt to
-     open the file that was executed to create this inferior.  */
-  if (try_open_exec && !fake_pid_p && get_exec_file (0) == NULL)
+  /* Attempt to open the file that was executed to create this
+     inferior.  If the user has explicitly specified executable
+     and/or symbol files then warn the user if their choices do
+     not match.  Otherwise, set exec_file and symfile_objfile to
+     the new file.  */
+  if (try_open_exec && !fake_pid_p)
     exec_file_locate_attach (pid, 1);
 
   return inf;
diff --git a/gdb/symfile.c b/gdb/symfile.c
index 0c35ffa..d44cb7d 100644
--- a/gdb/symfile.c
+++ b/gdb/symfile.c
@@ -1347,6 +1347,8 @@  symbol_file_clear (int from_tty)
   gdb_assert (symfile_objfile == NULL);
   if (from_tty)
     printf_unfiltered (_("No symbol file now.\n"));
+
+  symfile_objfile_is_user_supplied = 0;
 }
 
 static int
@@ -1664,6 +1666,7 @@  symbol_file_command (char *args, int from_tty)
 	  else
 	    {
 	      symbol_file_add_main_1 (*argv, from_tty, flags);
+	      symfile_objfile_is_user_supplied = 1;
 	      name = *argv;
 	    }