RFC: Don't kill the program after "file"

RFC: Don't kill the program after "file"

[Daniel Jacobowitz]

The "file" command currently prompts if a program is running:

(gdb) file /bin/cat
A program is being debugged already.  Kill it? (y or n) 

I've encountered a couple problems with this:

1. If you say "no", it aborts the "file" command.  So there is no way
to deliberately change the file.
2. If you say yes, it doesn't just do the equivalent of "kill", but
"kill" and "disconnect".  For target extended-remote, this means you
come unexpectedly disconnected from your target.

This patch completely removes the prompt.  Instead, we simply allow it.
It's occasionally useful, even during native debugging (at least I know
I've attached to a running program with the wrong file specified
before).  It's more useful during remote debugging, where I've seen
users frequently confused that "target remote; file" doesn't work even
though "file; target remote" does.

I talked with Jim about this.  He wasn't entirely happy with discarding
the prompt for native debugging, where it traditionally makes sense; we
didn't quite make it to an agreement on what ought to happen.  He
suggested adding a "process-oriented" flag to target vectors.  For a
process oriented target (i.e. one which can handle "run" and "kill" and
so forth) it makes sense to offer to kill at this point; for
board oriented targets it makes less sense.  Another thing which just
occured to me would be to make the file command succeed if you say "n"
at the query.

Any comments?  I'm definitely not planning to commit this patch without
more discussion.  Tested on x86_64-pc-linux-gnu.

-- 
Daniel Jacobowitz
CodeSourcery

2006-06-13  Daniel Jacobowitz  <dan@codesourcery.com>

	* corefile.c (reopen_exec_file): Only check for an open exec file.
	Use exec_file_attach.
	* exec.c (exec_open): Make static.
	(exec_file_command): Don't use target_preopen.
	* gdbcore.h (exec_open): Remove prototype.

2006-06-13  Daniel Jacobowitz  <dan@codesourcery.com>

	* gdb.base/completion.exp: Remove support for kill prompt after
	"file".
	* gdb.stabs/weird.exp: Likewise.
	* lib/mi-support.exp (mi_gdb_file_cmd): Likewise.
	* lib/gdb.exp (gdb_file_cmd): Likewise.  Kill the program explicitly.

Index: corefile.c
===================================================================
RCS file: /cvs/src/src/gdb/corefile.c,v
retrieving revision 1.37
diff -u -p -r1.37 corefile.c
--- corefile.c	10 Jan 2006 23:01:44 -0000	1.37
+++ corefile.c	13 Jun 2006 20:35:35 -0000
@@ -156,8 +156,8 @@ reopen_exec_file (void)
   struct stat st;
   long mtime;
 
-  /* Don't do anything if the current target isn't exec. */
-  if (exec_bfd == NULL || strcmp (target_shortname, "exec") != 0)
+  /* Don't do anything if there isn't an exec.  */
+  if (exec_bfd == NULL)
     return;
 
   /* If the timestamp of the exec file has changed, reopen it. */
@@ -167,9 +167,7 @@ reopen_exec_file (void)
   res = stat (filename, &st);
 
   if (mtime && mtime != st.st_mtime)
-    {
-      exec_open (filename, 0);
-    }
+    exec_file_attach (filename, 0);
 #endif
 }
 \f
Index: exec.c
===================================================================
RCS file: /cvs/src/src/gdb/exec.c,v
retrieving revision 1.61
diff -u -p -r1.61 exec.c
--- exec.c	18 Apr 2006 19:20:06 -0000	1.61
+++ exec.c	13 Jun 2006 20:35:35 -0000
@@ -85,7 +85,7 @@ show_write_files (struct ui_file *file, 
 
 struct vmap *vmap;
 
-void
+static void
 exec_open (char *args, int from_tty)
 {
   target_preopen (from_tty);
@@ -287,8 +287,6 @@ exec_file_command (char *args, int from_
   char **argv;
   char *filename;
   
-  target_preopen (from_tty);
-
   if (args)
     {
       /* Scan through the args and pick up the first non option arg
Index: gdbcore.h
===================================================================
RCS file: /cvs/src/src/gdb/gdbcore.h,v
retrieving revision 1.20
diff -u -p -r1.20 gdbcore.h
--- gdbcore.h	17 Dec 2005 22:34:00 -0000	1.20
+++ gdbcore.h	13 Jun 2006 20:35:35 -0000
@@ -123,8 +123,6 @@ extern int write_files;
 
 extern void core_file_command (char *filename, int from_tty);
 
-extern void exec_open (char *filename, int from_tty);
-
 extern void exec_file_attach (char *filename, int from_tty);
 
 extern void exec_file_clear (int from_tty);
Index: testsuite/gdb.base/completion.exp
===================================================================
RCS file: /cvs/src/src/gdb/testsuite/gdb.base/completion.exp,v
retrieving revision 1.22
diff -u -p -r1.22 completion.exp
--- testsuite/gdb.base/completion.exp	7 Mar 2006 15:23:32 -0000	1.22
+++ testsuite/gdb.base/completion.exp	13 Jun 2006 20:35:35 -0000
@@ -671,19 +671,10 @@ sleep 1
 gdb_expect  {
         -re "^file ./gdb.base/completion\\.exp $"\
             { send_gdb "\n"
-              gdb_expect {
-                      -re "\r\nA program is being debugged already\\.  Kill it\\? \\(y or n\\) $"
-                      { send_gdb "n\n"
-                        gdb_expect {
-                                -re "\r\nProgram not killed\\.\r\n$gdb_prompt $"\
-                                        { pass "complete 'file ./gdb.base/complet'"}
-                                -re ".*$gdb_prompt $" { fail "complete 'file ./gdb.base/complet'"}
-                                timeout           {fail "(timeout) complete 'file ./gdb.base/complet'"}
-                               }
-                      }
-                      -re ".*$gdb_prompt $" { fail "complete 'file ./gdb.base/complet'"}
-                      timeout           {fail "(timeout) complete 'file ./gdb.base/complet'"}
-                     }
+	      # Ignore the exact error message.
+	      gdb_test_multiple "" "complete 'file ./gdb.base/complet'" {
+		  -re ".*$gdb_prompt $" { pass "complete 'file ./gdb.base/complet'" }
+	      }
             }
         -re ".*$gdb_prompt $"       { fail "complete 'file ./gdb.base/complet'" }
         timeout         { fail "(timeout) complete 'file ./gdb.base/complet'" }
Index: testsuite/gdb.stabs/weird.exp
===================================================================
RCS file: /cvs/src/src/gdb/testsuite/gdb.stabs/weird.exp,v
retrieving revision 1.8
diff -u -p -r1.8 weird.exp
--- testsuite/gdb.stabs/weird.exp	22 Apr 2004 17:55:46 -0000	1.8
+++ testsuite/gdb.stabs/weird.exp	13 Jun 2006 20:35:35 -0000
@@ -313,10 +313,6 @@ gdb_expect 60 {
     -re "^file (\[^ \]| +\008)*\r*\n" {
 	exp_continue
     }
-    -re "A program is being debugged already.  Kill it\\? \\(y or n\\)" {
-	send_gdb "y\n"
-	exp_continue
-    }
     -re "^Reading symbols from .*$binfile\\.\\.\\.done\.(|\r\nUsing host libthread_db library .*libthread_db.so.*\\.)\r\n$gdb_prompt $" {
 	pass "weirdx.o read without error"
     }
Index: testsuite/lib/gdb.exp
===================================================================
RCS file: /cvs/src/src/gdb/testsuite/lib/gdb.exp,v
retrieving revision 1.66
diff -u -p -r1.66 gdb.exp
--- testsuite/lib/gdb.exp	13 Jun 2006 13:20:25 -0000	1.66
+++ testsuite/lib/gdb.exp	13 Jun 2006 20:35:36 -0000
@@ -1008,6 +1008,20 @@ proc gdb_file_cmd { arg } {
 	}
     }
 
+    # The file command used to kill the remote target.  For the benefit
+    # of the testsuite, preserve this behavior.
+    send_gdb "kill\n"
+    gdb_expect 120 {
+        -re "Kill the program being debugged. .y or n. $" {
+            send_gdb "y\n"
+	    verbose "\t\tKilling previous program being debugged"
+            exp_continue
+        }
+        -re "$gdb_prompt $" {
+	    # OK.
+        }
+    }
+
     send_gdb "file $arg\n"
     gdb_expect 120 {
 	-re "Reading symbols from.*no debugging symbols found.*done.*$gdb_prompt $" {
@@ -1020,11 +1034,6 @@ proc gdb_file_cmd { arg } {
 	    set gdb_file_cmd_debug_info "debug"
 	    return 0
         }
-        -re "A program is being debugged already.*Kill it.*y or n. $" {
-            send_gdb "y\n"
-	    verbose "\t\tKilling previous program being debugged"
-            exp_continue
-        }
         -re "Load new symbol table from \".*\".*y or n. $" {
             send_gdb "y\n"
             gdb_expect 120 {
Index: testsuite/lib/mi-support.exp
===================================================================
RCS file: /cvs/src/src/gdb/testsuite/lib/mi-support.exp,v
retrieving revision 1.32
diff -u -p -r1.32 mi-support.exp
--- testsuite/lib/mi-support.exp	27 Sep 2005 22:39:04 -0000	1.32
+++ testsuite/lib/mi-support.exp	13 Jun 2006 20:35:36 -0000
@@ -411,11 +411,6 @@ proc mi_gdb_file_cmd { arg } {
             perror "$arg wasn't compiled with \"-g\""
             return -1
         }
-        -re "A program is being debugged already.*Kill it.*y or n. $" {
-            send_gdb "y\n"
-                verbose "\t\tKilling previous program being debugged"
-            exp_continue
-        }
         -re "Load new symbol table from \".*\".*y or n. $" {
             send_gdb "y\n"
             gdb_expect 120 {

Re: RFC: Don't kill the program after "file"

[Jim Blandy]

Daniel Jacobowitz <drow@false.org> writes:
> I talked with Jim about this.  He wasn't entirely happy with discarding
> the prompt for native debugging, where it traditionally makes sense; we
> didn't quite make it to an agreement on what ought to happen.  He
> suggested adding a "process-oriented" flag to target vectors.  For a
> process oriented target (i.e. one which can handle "run" and "kill" and
> so forth) it makes sense to offer to kill at this point; for
> board oriented targets it makes less sense.  Another thing which just
> occured to me would be to make the file command succeed if you say "n"
> at the query.

I don't think this patch should be blocked on reaching a consensus on
this larger design issue, but I would like to expand on the thought a
bit.

We certainly want GDB's commands to work consistently, regardless of
architecture, whether we're working remotely or locally, and so on.
However, the distinction between a target that creates a new process
each time you 'run' (like native Unix debugging) versus a target like
a simple board without a kernel or memory management, where you're
simply manipulating registers and memory that are always there, can't
really be concealed from the user.

In the 'process' case, there really is no register set for GDB to
operate on until you start the program running.  Commands like 'print
$pc' and 'where' need to produce an error.

    $ gdb hello
    GNU gdb Red Hat Linux (6.3.0.0-1.84rh) ...
    (gdb) print $pc
    No registers.
    (gdb) where
    No stack.
    (gdb) 

Commands like 'print a', where a is a global or static variable, read
their data directly to the executable file when there's no process, so
they can work even before a 'run'.

But in the non-process case, where GDB is talking to a system whose
state persists between program runs, there are always registers and
memory to play with; a 'load' just writes to memory to put the program
in place, and does some initialization like setting the PC.  There's
no reason to prohibit things like register manipulation and stack
backtraces until something like a 'run' is done.

Note that this is not really a "native vs. remote" distinction.  If
the remote end were a gdbserver running on a Linux machine, it would
make perfect sense for that remote target to employ a 'process' model.
(I think 'extended remote' mode works this way, essentially, but there
are details I'm confused about.)  The distinction has to do with how
programs are run, not with our communications protocol.

GDB must expose this distinction the user; we can't unify the two
cases.  To make the persistent case behave exactly like the process
case, we would need to prohibit well-defined, everyday operations.
The purpose of consistency is to make things simpler for the user, but
there's no good way to replace the lost functionality.  And making the
process case behave like the persistent case doesn't make sense
either; there really are no registers until you start the program.

But at the moment, GDB is in kind of a suboptimal state: the
distinction is visible, but not exploited where it should be, and
there are a few attempts to ignore it that cause annoying behavior.
The behavior Daniel's dealing with is an example: if you're talking to
a remote target, the 'exec-file' command will currently disconnect you
from the target; this is consistent with the native behavior (where we
kill the inferior process), but not actually useful to people
debugging native targets.

I haven't tried to think through yet how to make this concrete --- how
commands should behave, and so on.  I'm sure doing so would change my
understanding.

Re: RFC: Don't kill the program after "file"

[Jim Blandy]

Daniel Jacobowitz <drow@false.org> writes:

> The "file" command currently prompts if a program is running:
>
> (gdb) file /bin/cat
> A program is being debugged already.  Kill it? (y or n) 
>
> I've encountered a couple problems with this:
>
> 1. If you say "no", it aborts the "file" command.  So there is no way
> to deliberately change the file.
> 2. If you say yes, it doesn't just do the equivalent of "kill", but
> "kill" and "disconnect".  For target extended-remote, this means you
> come unexpectedly disconnected from your target.
>
> This patch completely removes the prompt.  Instead, we simply allow it.
> It's occasionally useful, even during native debugging (at least I know
> I've attached to a running program with the wrong file specified
> before).  It's more useful during remote debugging, where I've seen
> users frequently confused that "target remote; file" doesn't work even
> though "file; target remote" does.
>
> I talked with Jim about this.  He wasn't entirely happy with discarding
> the prompt for native debugging, where it traditionally makes sense; we
> didn't quite make it to an agreement on what ought to happen.  He
> suggested adding a "process-oriented" flag to target vectors.  For a
> process oriented target (i.e. one which can handle "run" and "kill" and
> so forth) it makes sense to offer to kill at this point; for
> board oriented targets it makes less sense.  Another thing which just
> occured to me would be to make the file command succeed if you say "n"
> at the query.

I think something like that is workable.  What if we simply left the
killing to the 'kill' and 'run' commands, and let the prompt say:

    A program is being debugged already.
    Are you sure you want to change the file?  (y or n)

Saying 'y' would not kill the program.

This would mean that 'exec-file; run' when a program is running would
ask the user two questions: one to confirm the file change, and then
another to confirm the kill before restarting.

Re: RFC: Don't kill the program after "file"

[Kevin Buettner]

On Wed, 14 Jun 2006 13:06:40 -0700
Jim Blandy <jimb@codesourcery.com> wrote:

> > Another thing which just
> > occured to me would be to make the file command succeed if you say "n"
> > at the query.
> 
> I think something like that is workable.  What if we simply left the
> killing to the 'kill' and 'run' commands, and let the prompt say:
> 
>     A program is being debugged already.
>     Are you sure you want to change the file?  (y or n)
> 
> Saying 'y' would not kill the program.
> 
> This would mean that 'exec-file; run' when a program is running would
> ask the user two questions: one to confirm the file change, and then
> another to confirm the kill before restarting.

Jim's proposal sounds reasonable to me.

Kevin

Re: RFC: Don't kill the program after "file"

[Daniel Jacobowitz]

On Mon, Jun 26, 2006 at 12:00:20PM -0700, Kevin Buettner wrote:
> > > Another thing which just
> > > occured to me would be to make the file command succeed if you say "n"
> > > at the query.
> > 
> > I think something like that is workable.  What if we simply left the
> > killing to the 'kill' and 'run' commands, and let the prompt say:
> > 
> >     A program is being debugged already.
> >     Are you sure you want to change the file?  (y or n)
> > 
> > Saying 'y' would not kill the program.
> > 
> > This would mean that 'exec-file; run' when a program is running would
> > ask the user two questions: one to confirm the file change, and then
> > another to confirm the kill before restarting.
> 
> Jim's proposal sounds reasonable to me.

Me too!  Here is an implementation.  It gives the prompt suggested by
Jim if there is a TTY, calls error if the user declines (which prevents
"file" from trying to do "symbol-file" afterwards), and otherwise
succeeds.  "file" never kills the running program.

Any opinions on this patch?  Shall I commit it?  Tested on
x86_64-pc-linux-gnu.

-- 
Daniel Jacobowitz
CodeSourcery

2006-07-12  Daniel Jacobowitz  <dan@codesourcery.com>

	* corefile.c (reopen_exec_file): Only check for an open exec file.
	Use exec_file_attach.
	* exec.c (exec_open): Make static.
	(exec_file_command): Don't use target_preopen.  Query directly about
	changing the file.
	* gdbcore.h (exec_open): Remove prototype.

2006-07-12  Daniel Jacobowitz  <dan@codesourcery.com>

	* gdb.base/completion.exp: Update for change in "file" behavior.
	* gdb.stabs/weird.exp: Likewise.
	* lib/mi-support.exp (mi_gdb_file_cmd): Likewise.
	* lib/gdb.exp (gdb_file_cmd): Likewise.  Kill the program explicitly.

Index: corefile.c
===================================================================
RCS file: /cvs/src/src/gdb/corefile.c,v
retrieving revision 1.37
diff -u -p -r1.37 corefile.c
--- corefile.c	10 Jan 2006 23:01:44 -0000	1.37
+++ corefile.c	12 Jul 2006 20:12:06 -0000
@@ -156,8 +156,8 @@ reopen_exec_file (void)
   struct stat st;
   long mtime;
 
-  /* Don't do anything if the current target isn't exec. */
-  if (exec_bfd == NULL || strcmp (target_shortname, "exec") != 0)
+  /* Don't do anything if there isn't an exec file. */
+  if (exec_bfd == NULL)
     return;
 
   /* If the timestamp of the exec file has changed, reopen it. */
@@ -167,9 +167,7 @@ reopen_exec_file (void)
   res = stat (filename, &st);
 
   if (mtime && mtime != st.st_mtime)
-    {
-      exec_open (filename, 0);
-    }
+    exec_file_attach (filename, 0);
 #endif
 }
 \f
Index: exec.c
===================================================================
RCS file: /cvs/src/src/gdb/exec.c,v
retrieving revision 1.61
diff -u -p -r1.61 exec.c
--- exec.c	18 Apr 2006 19:20:06 -0000	1.61
+++ exec.c	12 Jul 2006 20:12:06 -0000
@@ -85,7 +85,7 @@ show_write_files (struct ui_file *file, 
 
 struct vmap *vmap;
 
-void
+static void
 exec_open (char *args, int from_tty)
 {
   target_preopen (from_tty);
@@ -286,8 +286,11 @@ exec_file_command (char *args, int from_
 {
   char **argv;
   char *filename;
-  
-  target_preopen (from_tty);
+
+  if (from_tty && target_has_execution
+      && !query (_("A program is being debugged already.\n"
+		   "Are you sure you want to change the file? ")))
+    error (_("File not changed."));
 
   if (args)
     {
Index: gdbcore.h
===================================================================
RCS file: /cvs/src/src/gdb/gdbcore.h,v
retrieving revision 1.20
diff -u -p -r1.20 gdbcore.h
--- gdbcore.h	17 Dec 2005 22:34:00 -0000	1.20
+++ gdbcore.h	12 Jul 2006 20:12:06 -0000
@@ -123,8 +123,6 @@ extern int write_files;
 
 extern void core_file_command (char *filename, int from_tty);
 
-extern void exec_open (char *filename, int from_tty);
-
 extern void exec_file_attach (char *filename, int from_tty);
 
 extern void exec_file_clear (int from_tty);
Index: testsuite/gdb.base/completion.exp
===================================================================
RCS file: /cvs/src/src/gdb/testsuite/gdb.base/completion.exp,v
retrieving revision 1.22
diff -u -p -r1.22 completion.exp
--- testsuite/gdb.base/completion.exp	7 Mar 2006 15:23:32 -0000	1.22
+++ testsuite/gdb.base/completion.exp	12 Jul 2006 20:12:06 -0000
@@ -671,19 +671,14 @@ sleep 1
 gdb_expect  {
         -re "^file ./gdb.base/completion\\.exp $"\
             { send_gdb "\n"
-              gdb_expect {
-                      -re "\r\nA program is being debugged already\\.  Kill it\\? \\(y or n\\) $"
-                      { send_gdb "n\n"
-                        gdb_expect {
-                                -re "\r\nProgram not killed\\.\r\n$gdb_prompt $"\
-                                        { pass "complete 'file ./gdb.base/complet'"}
-                                -re ".*$gdb_prompt $" { fail "complete 'file ./gdb.base/complet'"}
-                                timeout           {fail "(timeout) complete 'file ./gdb.base/complet'"}
-                               }
-                      }
-                      -re ".*$gdb_prompt $" { fail "complete 'file ./gdb.base/complet'"}
-                      timeout           {fail "(timeout) complete 'file ./gdb.base/complet'"}
-                     }
+	      # Ignore the exact error message.
+	      gdb_test_multiple "" "complete 'file ./gdb.base/complet'" {
+		  -re "\r\nA program is being debugged already\\.\[\r\n\]+Are you sure you want to change the file\\? \\(y or n\\) $" {
+		      send_gdb "n\n"
+		      exp_continue
+		  }
+		  -re ".*$gdb_prompt $" { pass "complete 'file ./gdb.base/complet'" }
+	      }
             }
         -re ".*$gdb_prompt $"       { fail "complete 'file ./gdb.base/complet'" }
         timeout         { fail "(timeout) complete 'file ./gdb.base/complet'" }
Index: testsuite/gdb.stabs/weird.exp
===================================================================
RCS file: /cvs/src/src/gdb/testsuite/gdb.stabs/weird.exp,v
retrieving revision 1.8
diff -u -p -r1.8 weird.exp
--- testsuite/gdb.stabs/weird.exp	22 Apr 2004 17:55:46 -0000	1.8
+++ testsuite/gdb.stabs/weird.exp	12 Jul 2006 20:12:07 -0000
@@ -313,7 +313,7 @@ gdb_expect 60 {
     -re "^file (\[^ \]| +\008)*\r*\n" {
 	exp_continue
     }
-    -re "A program is being debugged already.  Kill it\\? \\(y or n\\)" {
+    -re "A program is being debugged already.\[\r\n\]+Are you sure you want to change the file\\? \\(y or n\\)" {
 	send_gdb "y\n"
 	exp_continue
     }
Index: testsuite/lib/gdb.exp
===================================================================
RCS file: /cvs/src/src/gdb/testsuite/lib/gdb.exp,v
retrieving revision 1.68
diff -u -p -r1.68 gdb.exp
--- testsuite/lib/gdb.exp	22 Jun 2006 19:17:09 -0000	1.68
+++ testsuite/lib/gdb.exp	12 Jul 2006 20:12:07 -0000
@@ -1008,6 +1008,20 @@ proc gdb_file_cmd { arg } {
 	}
     }
 
+    # The file command used to kill the remote target.  For the benefit
+    # of the testsuite, preserve this behavior.
+    send_gdb "kill\n"
+    gdb_expect 120 {
+	-re "Kill the program being debugged. .y or n. $" {
+	    send_gdb "y\n"
+	    verbose "\t\tKilling previous program being debugged"
+	    exp_continue
+	}
+	-re "$gdb_prompt $" {
+	    # OK.
+	}
+    }
+
     send_gdb "file $arg\n"
     gdb_expect 120 {
 	-re "Reading symbols from.*no debugging symbols found.*done.*$gdb_prompt $" {
@@ -1020,11 +1034,6 @@ proc gdb_file_cmd { arg } {
 	    set gdb_file_cmd_debug_info "debug"
 	    return 0
         }
-        -re "A program is being debugged already.*Kill it.*y or n. $" {
-            send_gdb "y\n"
-	    verbose "\t\tKilling previous program being debugged"
-            exp_continue
-        }
         -re "Load new symbol table from \".*\".*y or n. $" {
             send_gdb "y\n"
             gdb_expect 120 {
Index: testsuite/lib/mi-support.exp
===================================================================
RCS file: /cvs/src/src/gdb/testsuite/lib/mi-support.exp,v
retrieving revision 1.32
diff -u -p -r1.32 mi-support.exp
--- testsuite/lib/mi-support.exp	27 Sep 2005 22:39:04 -0000	1.32
+++ testsuite/lib/mi-support.exp	12 Jul 2006 20:12:07 -0000
@@ -411,11 +411,6 @@ proc mi_gdb_file_cmd { arg } {
             perror "$arg wasn't compiled with \"-g\""
             return -1
         }
-        -re "A program is being debugged already.*Kill it.*y or n. $" {
-            send_gdb "y\n"
-                verbose "\t\tKilling previous program being debugged"
-            exp_continue
-        }
         -re "Load new symbol table from \".*\".*y or n. $" {
             send_gdb "y\n"
             gdb_expect 120 {

Re: RFC: Don't kill the program after "file"

[Daniel Jacobowitz]

On Wed, Jul 12, 2006 at 04:16:09PM -0400, Daniel Jacobowitz wrote:
> > > This would mean that 'exec-file; run' when a program is running would
> > > ask the user two questions: one to confirm the file change, and then
> > > another to confirm the kill before restarting.
> > 
> > Jim's proposal sounds reasonable to me.
> 
> Me too!  Here is an implementation.  It gives the prompt suggested by
> Jim if there is a TTY, calls error if the user declines (which prevents
> "file" from trying to do "symbol-file" afterwards), and otherwise
> succeeds.  "file" never kills the running program.
> 
> Any opinions on this patch?  Shall I commit it?  Tested on
> x86_64-pc-linux-gnu.

I've committed this.

-- 
Daniel Jacobowitz
CodeSourcery

Re: RFC: Don't kill the program after "file"

[Fred Fish]

I'm currently getting new failures with a mips-elf toolchain, and
other toolchains, when using the simulator for testing.  I'm not
entirely sure that they are due to this change, but it seems likely.

Here is a snippet from the gdb.log file:

(gdb) dir /src/latest/trunk/src/gdb/gdb/testsuite/gdb.base
Source directories searched: /src/latest/trunk/src/gdb/gdb/testsuite/gdb.base:$cdir:$cwd
(gdb) kill
The program is not being run.
(gdb) file /links/build/latest/trunk/mips-elf/gdb/gdb/testsuite/gdb.base/opaque
A program is being debugged already.
Are you sure you want to change the file? (y or n) ERROR: couldn't load /links/build/latest/trunk/mips-elf/gdb/gdb/testsuite/gdb.base/opaque into /links/build/latest/trunk/mips-elf/gdb/gdb/testsuite/../../gdb/gdb (timed out).
delete breakpoints
Please answer y or n.
A program is being debugged already.
Are you sure you want to change the file? (y or n) ERROR: Delete all breakpoints in delete_breakpoints (timeout)
break main
Please answer y or n.
A program is being debugged already.
Are you sure you want to change the file? (y or n) UNRESOLVED: gdb.base/opaque.exp: setting breakpoint at main (timeout)
ERROR: cannot run to breakpoint at main
ERROR: Got interactive prompt.
UNRESOLVED: gdb.base/opaque.exp: whatis on opaque struct pointer (dynamically)
ERROR: Got interactive prompt.
UNRESOLVED: gdb.base/opaque.exp: ptype on opaque struct pointer (dynamically) 1
ERROR: Got interactive prompt.

Re: RFC: Don't kill the program after "file"

[Daniel Jacobowitz]

On Tue, Aug 01, 2006 at 04:17:28PM -0400, Fred Fish wrote:
> I'm currently getting new failures with a mips-elf toolchain, and
> other toolchains, when using the simulator for testing.  I'm not
> entirely sure that they are due to this change, but it seems likely.
> 
> Here is a snippet from the gdb.log file:

> (gdb) kill
> The program is not being run.
> (gdb) file /links/build/latest/trunk/mips-elf/gdb/gdb/testsuite/gdb.base/opaque
> A program is being debugged already.
> Are you sure you want to change the file? (y or n) ERROR: couldn't load /links/build/latest/trunk/mips-elf/gdb/gdb/testsuite/gdb.base/opaque into /links/build/latest/trunk/mips-elf/gdb/gdb/testsuite/../../gdb/gdb (timed out).

The simulator is being a bit schizophrenic.  It is not running, yet it
is.  This happens because inferior_ptid is set to null_ptid after kill
or before execution, and the sim target is left on the stack, but
to_has_execution is always set for "target sim".

As it happens I have patches for a similar issue with target
extended-remote.  The remote patches are substantial, but I can
probably break out the simulator related bits.  I'll try to do that.

They change target_has_execution from "is capable of running" to
"is running right now".

-- 
Daniel Jacobowitz
CodeSourcery

Re: RFC: Don't kill the program after "file"

[Fred Fish]

On Tuesday 01 August 2006 18:11, Daniel Jacobowitz wrote:
> As it happens I have patches for a similar issue with target
> extended-remote.  The remote patches are substantial, but I can
> probably break out the simulator related bits.  I'll try to do that.

Here's the patch I'm temporarily using to allow me to continue build
and testing toolchains with the simulators.

-Fred

Index: gdb/gdb/testsuite/lib/gdb.exp
===================================================================
RCS file: /cvsroots/latest/src/gdb/gdb/testsuite/lib/gdb.exp,v
retrieving revision 1.1.1.6
diff -u -p -r1.1.1.6 gdb.exp
--- gdb/gdb/testsuite/lib/gdb.exp	31 Jul 2006 20:28:17 -0000	1.1.1.6
+++ gdb/gdb/testsuite/lib/gdb.exp	7 Aug 2006 10:33:19 -0000
@@ -1017,6 +1017,9 @@ proc gdb_file_cmd { arg } {
 	    verbose "\t\tKilling previous program being debugged"
 	    exp_continue
 	}
+	-re "The program is not being run.*$gdb_prompt $" {
+	    # OK.
+	}
 	-re "$gdb_prompt $" {
 	    # OK.
 	}
@@ -1024,6 +1027,11 @@ proc gdb_file_cmd { arg } {
 
     send_gdb "file $arg\n"
     gdb_expect 120 {
+	-re "A program is being debugged already..*Are you sure you want to change the file.*\\(y or n\\) $"\
+	    { send_gdb "y\n"
+		verbose "\t\tKilling previous program being debugged"
+	    exp_continue
+	}
 	-re "Reading symbols from.*no debugging symbols found.*done.*$gdb_prompt $" {
 	    verbose "\t\tLoaded $arg into the $GDB with no debugging symbols"
 	    set gdb_file_cmd_debug_info "nodebug"

Re: RFC: Don't kill the program after "file"

[Daniel Jacobowitz]

Sorry for the delay getting back to this.  It was breaking our
nightly simulator testing, too.

On Tue, Aug 01, 2006 at 06:11:05PM -0400, Daniel Jacobowitz wrote:
> The simulator is being a bit schizophrenic.  It is not running, yet it
> is.  This happens because inferior_ptid is set to null_ptid after kill
> or before execution, and the sim target is left on the stack, but
> to_has_execution is always set for "target sim".
> 
> As it happens I have patches for a similar issue with target
> extended-remote.  The remote patches are substantial, but I can
> probably break out the simulator related bits.  I'll try to do that.
> 
> They change target_has_execution from "is capable of running" to
> "is running right now".

Here's what I meant.  This is a bit complicated, so I added a section
about it to the internals manual (and a little framework for
documenting the rest of struct target_ops, as people get inspired).
I'll just quote the new documentation:

  A target vector can be completely inactive (not pushed on the target
  stack), active but not running (pushed, but not connected to a fully
  manifested inferior), or completely active (pushed, with an accessible
  inferior).  Most targets are only completely inactive or completely
  active, but some support persistant connections to a target even
  when the target has exited or not yet started.
  
  For example, connecting to the simulator using @code{target sim} does
  not create a running program.  Neither registers nor memory are
  accessible until @code{run}.  Similarly, after @code{kill}, the
  program can not continue executing.  But in both cases @value{GDBN}
  remains connected to the simulator, and target-specific commands
  are directed to the simulator.
  
  A target which only supports complete activation should push itself
  onto the stack in its @code{to_open} routine (by calling
  @code{push_target}), and unpush itself from the stack in its
  @code{to_mourn_inferior} routine (by calling @code{unpush_target}).
  
  A target which supports both partial and complete activation should
  still call @code{push_target} in @code{to_open}, but not call
  @code{unpush_target} in @code{to_mourn_inferior}.  Instead, it should
  call one of @code{target_mark_running} and @code{target_mark_exited}
  in its @code{to_open}, depending on whether the target is fully active
  after connection.  It should also call @code{target_mark_running} any
  time the inferior becomes fully active (e.g.@: in
  @code{to_create_inferior} and @code{to_attach}), and
  @code{target_mark_exited} when the inferior becomes inactive (in
  @code{to_mourn_inferior}).  The target should also make sure to call
  @code{target_mourn_inferior} from its @code{to_kill}, to return the
  target to inactive state.

This is a bit different than we had before, but I think it's fairly
clear and more useful.  I tested this on arm-sim, where it removes a
bunch of ERRORs, by fixing "target sim; kill; file" - Fred reported
that "file" in that case warned that the target was already running,
even though we'd just issued a "kill".

I'll be using this approach for the extended-remote target, or a
similar variant, soon.

Any comments on this patch?  Shall I commit it?  Eli, do the
gdbint.texinfo changes look good to you?

-- 
Daniel Jacobowitz
CodeSourcery

2006-08-16  Daniel Jacobowitz  <dan@codesourcery.com>

	* gdbint.texinfo (Target Vector Definition): Move most
	content into Existing Targets.  Add a menu.
	(Existing Targets): New section, moved from Target Vector
	Definition.  Use @subsection.
	(Managing Execution State): New section.

2006-08-16  Daniel Jacobowitz  <dan@codesourcery.com>

	* remote-sim.c (gdbsim_kill): Call target_mourn_inferior.
	(gdbsim_load): Don't bother to adjust inferior_ptid here.
	(gdbsim_create_inferior): Mark the simulator as running.
	(gdbsim_open): Don't bother fetching registers.  Mark
	the target as not running.
	(gdbsim_xfer): When the program is not running, pass memory
	requests down.
	(gdbsim_mourn_inferior): Mark the target as not running.
	* target.c (target_mark_running, target_mark_exited): New.
	* target.h (target_has_execution): Update the comment.
	(target_mark_running, target_mark_exited): New prototypes.

Index: gdb-mainline/gdb/doc/gdbint.texinfo
===================================================================
--- gdb-mainline.orig/gdb/doc/gdbint.texinfo	2006-05-15 00:20:01.000000000 -0700
+++ gdb-mainline/gdb/doc/gdbint.texinfo	2006-08-16 19:02:19.000000000 -0700
@@ -4405,11 +4405,56 @@ actually exercises control over a proces
 @value{GDBN} includes some 30-40 different target vectors; however,
 each configuration of @value{GDBN} includes only a few of them.
 
-@section File Targets
+@menu
+* Managing Execution State::
+* Existing Targets::
+@end menu
+
+@node Managing Execution State
+@section Managing Execution State
+@cindex execution state
+
+A target vector can be completely inactive (not pushed on the target
+stack), active but not running (pushed, but not connected to a fully
+manifested inferior), or completely active (pushed, with an accessible
+inferior).  Most targets are only completely inactive or completely
+active, but some support persistant connections to a target even
+when the target has exited or not yet started.
+
+For example, connecting to the simulator using @code{target sim} does
+not create a running program.  Neither registers nor memory are
+accessible until @code{run}.  Similarly, after @code{kill}, the
+program can not continue executing.  But in both cases @value{GDBN}
+remains connected to the simulator, and target-specific commands
+are directed to the simulator.
+
+A target which only supports complete activation should push itself
+onto the stack in its @code{to_open} routine (by calling
+@code{push_target}), and unpush itself from the stack in its
+@code{to_mourn_inferior} routine (by calling @code{unpush_target}).
+
+A target which supports both partial and complete activation should
+still call @code{push_target} in @code{to_open}, but not call
+@code{unpush_target} in @code{to_mourn_inferior}.  Instead, it should
+call one of @code{target_mark_running} and @code{target_mark_exited}
+in its @code{to_open}, depending on whether the target is fully active
+after connection.  It should also call @code{target_mark_running} any
+time the inferior becomes fully active (e.g.@: in
+@code{to_create_inferior} and @code{to_attach}), and
+@code{target_mark_exited} when the inferior becomes inactive (in
+@code{to_mourn_inferior}).  The target should also make sure to call
+@code{target_mourn_inferior} from its @code{to_kill}, to return the
+target to inactive state.
+
+@node Existing Targets
+@section Existing Targets
+@cindex targets
+
+@subsection File Targets
 
 Both executables and core files have target vectors.
 
-@section Standard Protocol and Remote Stubs
+@subsection Standard Protocol and Remote Stubs
 
 @value{GDBN}'s file @file{remote.c} talks a serial protocol to code
 that runs in the target system.  @value{GDBN} provides several sample
@@ -4454,13 +4499,13 @@ of the debugger/stub.
 From reading the stub, it's probably not obvious how breakpoints work.
 They are simply done by deposit/examine operations from @value{GDBN}.
 
-@section ROM Monitor Interface
+@subsection ROM Monitor Interface
 
-@section Custom Protocols
+@subsection Custom Protocols
 
-@section Transport Layer
+@subsection Transport Layer
 
-@section Builtin Simulator
+@subsection Builtin Simulator
 
 
 @node Native Debugging
Index: gdb-mainline/gdb/remote-sim.c
===================================================================
--- gdb-mainline.orig/gdb/remote-sim.c	2006-04-19 00:18:09.000000000 -0700
+++ gdb-mainline/gdb/remote-sim.c	2006-08-16 18:42:28.000000000 -0700
@@ -383,8 +383,8 @@ gdbsim_kill (void)
     printf_filtered ("gdbsim_kill\n");
 
   /* There is no need to `kill' running simulator - the simulator is
-     not running */
-  inferior_ptid = null_ptid;
+     not running.  Mourning it is enough.  */
+  target_mourn_inferior ();
 }
 
 /* Load an executable file into the target process.  This is expected to
@@ -410,8 +410,6 @@ gdbsim_load (char *args, int fromtty)
   if (sr_get_debug ())
     printf_filtered ("gdbsim_load: prog \"%s\"\n", prog);
 
-  inferior_ptid = null_ptid;
-
   /* FIXME: We will print two messages on error.
      Need error to either not print anything if passed NULL or need
      another routine that doesn't take any arguments.  */
@@ -469,6 +467,7 @@ gdbsim_create_inferior (char *exec_file,
   sim_create_inferior (gdbsim_desc, exec_bfd, argv, env);
 
   inferior_ptid = pid_to_ptid (42);
+  target_mark_running (&gdbsim_ops);
   insert_breakpoints ();	/* Needed to get correct instruction in cache */
 
   clear_proceed_status ();
@@ -543,8 +542,12 @@ gdbsim_open (char *args, int from_tty)
     error (_("unable to create simulator instance"));
 
   push_target (&gdbsim_ops);
-  target_fetch_registers (-1);
   printf_filtered ("Connected to the simulator.\n");
+
+  /* There's nothing running after "target sim" or "load"; not until
+     "run".  */
+  inferior_ptid = null_ptid;
+  target_mark_exited (&gdbsim_ops);
 }
 
 /* Does whatever cleanup is required for a target that we are no longer
@@ -747,6 +750,12 @@ gdbsim_xfer_inferior_memory (CORE_ADDR m
 			     int write, struct mem_attrib *attrib,
 			     struct target_ops *target)
 {
+  /* If no program is running yet, then ignore the simulator for
+     memory.  Pass the request down to the next target, hopefully
+     an exec file.  */
+  if (!target_has_execution)
+    return 0;
+
   if (!program_loaded)
     error (_("No program loaded."));
 
@@ -802,6 +811,7 @@ gdbsim_mourn_inferior (void)
     printf_filtered ("gdbsim_mourn_inferior:\n");
 
   remove_breakpoints ();
+  target_mark_exited (&gdbsim_ops);
   generic_mourn_inferior ();
 }
 
Index: gdb-mainline/gdb/target.c
===================================================================
--- gdb-mainline.orig/gdb/target.c	2006-08-16 00:22:21.000000000 -0700
+++ gdb-mainline/gdb/target.c	2006-08-16 17:58:58.000000000 -0700
@@ -645,6 +645,56 @@ update_current_target (void)
   current_target.beneath = target_stack;
 }
 
+/* Mark OPS as a running target.  This reverses the effect
+   of target_mark_exited.  */
+
+void
+target_mark_running (struct target_ops *ops)
+{
+  struct target_ops *t;
+
+  for (t = target_stack; t != NULL; t = t->beneath)
+    if (t == ops)
+      break;
+  if (t == NULL)
+    internal_error (__FILE__, __LINE__,
+		    "Attempted to mark unpushed target \"%s\" as running",
+		    ops->to_shortname);
+
+  ops->to_has_execution = 1;
+  ops->to_has_all_memory = 1;
+  ops->to_has_memory = 1;
+  ops->to_has_stack = 1;
+  ops->to_has_registers = 1;
+
+  update_current_target ();
+}
+
+/* Mark OPS as a non-running target.  This reverses the effect
+   of target_mark_running.  */
+
+void
+target_mark_exited (struct target_ops *ops)
+{
+  struct target_ops *t;
+
+  for (t = target_stack; t != NULL; t = t->beneath)
+    if (t == ops)
+      break;
+  if (t == NULL)
+    internal_error (__FILE__, __LINE__,
+		    "Attempted to mark unpushed target \"%s\" as running",
+		    ops->to_shortname);
+
+  ops->to_has_execution = 0;
+  ops->to_has_all_memory = 0;
+  ops->to_has_memory = 0;
+  ops->to_has_stack = 0;
+  ops->to_has_registers = 0;
+
+  update_current_target ();
+}
+
 /* Push a new target type into the stack of the existing target accessors,
    possibly superseding some of the existing accessors.
 
Index: gdb-mainline/gdb/target.h
===================================================================
--- gdb-mainline.orig/gdb/target.h	2006-08-16 00:22:21.000000000 -0700
+++ gdb-mainline/gdb/target.h	2006-08-16 18:26:51.000000000 -0700
@@ -873,11 +873,12 @@ int target_follow_fork (int follow_child
      (current_target.to_has_registers)
 
 /* Does the target have execution?  Can we make it jump (through
-   hoops), or pop its stack a few times?  FIXME: If this is to work that
-   way, it needs to check whether an inferior actually exists.
-   remote-udi.c and probably other targets can be the current target
-   when the inferior doesn't actually exist at the moment.  Right now
-   this just tells us whether this target is *capable* of execution.  */
+   hoops), or pop its stack a few times?  This means that the current
+   target is currently executing; for some targets, that's the same as
+   whether or not the target is capable of execution, but there are
+   also targets which can be current while not executing.  In that
+   case this will become true after target_create_inferior or
+   target_attach.  */
 
 #define	target_has_execution	\
      (current_target.to_has_execution)
@@ -1131,6 +1132,13 @@ extern void target_preopen (int);
 
 extern void pop_target (void);
 
+/* Mark a pushed target as running or exited, for targets which do not
+   automatically pop when not active.  */
+
+void target_mark_running (struct target_ops *);
+
+void target_mark_exited (struct target_ops *);
+
 /* Struct section_table maps address ranges to file sections.  It is
    mostly used with BFD files, but can be used without (e.g. for handling
    raw disks, or files not in formats handled by BFD).  */