Re: generic_prepare_to_proceed vs. Ctrl-C

[Jonathan Larmour <jlarmour@redhat.com>]

David Smith wrote:
> 
> Now let's get to the details of your patch.  The main problem I have with
> your patch is that it leaves that comment before switching the ptid's in an
> odd state.  I'd change it to something like "Switch back to WAIT_PID
> thread.".  Also, your change before the call to breakpoint_here_p() needs a
> better comment explaining why we only return 1 if there is a breakpoint there.
> 
> One last thing.  Your patch fixes the generic implementation of
> PREPARE_TO_PROCEED, but it doesn't do anything for the 4 other
> implementations of PREPARE_TO_PROCEED.  These are located in: hppa-tdep.c,
> lin-lwp.c, linux-thread.c, and m3-nat.c.  At bare minimum you might add a
> FIXME comment to each of the 4 other implementations describing what needs
> doing.  Of course if you have the time and are braver than I was, you could
> try switching the Linux port to just use the generic version.

Call me a cowardly custard in that case :). Is this better?

2001-05-23  Jonathan Larmour  <jlarmour@redhat.com>

	* arch-utils.c (generic_prepare_to_proceed): Allow for having
	stopped due to a Ctrl-C as well as breakpoints.

	* hppa-tdep.c (hppa_prepare_to_proceed): Add FIXME as this may not
	support thread switches after Ctrl-C.
	* lin-lwp.c (lin_lwp_prepare_to_proceed): Ditto.
	* linux-thread.c (linuxthreads_prepare_to_proceed): Ditto.
	* m3-nat.c (mach3_prepare_to_proceed): Ditto.


Jifl
-- 
Red Hat, Rustat House, Clifton Road, Cambridge, UK. Tel: +44 (1223) 271062
Maybe this world is another planet's Hell -Aldous Huxley || Opinions==mine
Index: arch-utils.c
===================================================================
RCS file: /cvs/src/src/gdb/arch-utils.c,v
retrieving revision 1.27
diff -u -5 -p -r1.27 arch-utils.c
--- arch-utils.c	2001/05/10 18:36:26	1.27
+++ arch-utils.c	2001/05/23 17:25:07
@@ -256,42 +256,46 @@ generic_prepare_to_proceed (int select_i
   struct target_waitstatus wait_status;
 
   /* Get the last target status returned by target_wait().  */
   get_last_target_status (&wait_ptid, &wait_status);
 
-  /* Make sure we were stopped at a breakpoint.  */
+  /* Make sure we were stopped either at a breakpoint, or because
+     of a Ctrl-C.  */
   if (wait_status.kind != TARGET_WAITKIND_STOPPED
-      || wait_status.value.sig != TARGET_SIGNAL_TRAP)
+      || (wait_status.value.sig != TARGET_SIGNAL_TRAP &&
+          wait_status.value.sig != TARGET_SIGNAL_INT))
     {
       return 0;
     }
 
   if (!ptid_equal (wait_ptid, minus_one_ptid)
       && !ptid_equal (inferior_ptid, wait_ptid))
     {
       /* Switched over from WAIT_PID.  */
       CORE_ADDR wait_pc = read_pc_pid (wait_ptid);
 
-      /* Avoid switching where it wouldn't do any good, i.e. if both
-         threads are at the same breakpoint.  */
-      if (wait_pc != read_pc () && breakpoint_here_p (wait_pc))
+      if (wait_pc != read_pc ())
 	{
 	  if (select_it)
 	    {
-	      /* User hasn't deleted the breakpoint.  Switch back to
-		 WAIT_PID and return non-zero.  */
+	      /* Switch back to WAIT_PID thread.  */
 	      inferior_ptid = wait_ptid;
 
 	      /* FIXME: This stuff came from switch_to_thread() in
 		 thread.c (which should probably be a public function).  */
 	      flush_cached_frames ();
 	      registers_changed ();
 	      stop_pc = wait_pc;
 	      select_frame (get_current_frame (), 0);
 	    }
-
-	  return 1;
+          /* We return 1 to indicate that there is a breakpoint here,
+             so we need to step over it before continuing to avoid
+             hitting it straight away. */
+          if (breakpoint_here_p (wait_pc))
+            {
+	      return 1;
+            }
 	}
     }
   return 0;
   
 }
Index: hppa-tdep.c
===================================================================
RCS file: /cvs/src/src/gdb/hppa-tdep.c,v
retrieving revision 1.14
diff -u -5 -p -r1.14 hppa-tdep.c
--- hppa-tdep.c	2001/05/12 03:18:33	1.14
+++ hppa-tdep.c	2001/05/23 17:25:11
@@ -4592,11 +4592,14 @@ unwind_command (char *exp, int from_tty)
    call "breakpoint_here_p".  If core gdb thinks there is a bpt
    here, that's what counts, as core gdb is the one which is
    putting the BPT instruction in and taking it out.
 
    Note that this implementation is potentially redundant now that
-   default_prepare_to_proceed() has been added.  */
+   default_prepare_to_proceed() has been added.
+
+   FIXME This may not support switching threads after Ctrl-C
+   correctly. The default implementation does support this. */
 int
 hppa_prepare_to_proceed (void)
 {
   pid_t old_thread;
   pid_t current_thread;
Index: lin-lwp.c
===================================================================
RCS file: /cvs/src/src/gdb/lin-lwp.c,v
retrieving revision 1.20
diff -u -5 -p -r1.20 lin-lwp.c
--- lin-lwp.c	2001/05/15 00:13:47	1.20
+++ lin-lwp.c	2001/05/23 17:25:11
@@ -261,11 +261,14 @@ iterate_over_lwps (int (*callback) (stru
 
 /* Implementation of the PREPARE_TO_PROCEED hook for the Linux LWP
    layer.
 
    Note that this implementation is potentially redundant now that
-   default_prepare_to_proceed() has been added.  */
+   default_prepare_to_proceed() has been added.
+
+   FIXME This may not support switching threads after Ctrl-C
+   correctly. The default implementation does support this. */
 
 int
 lin_lwp_prepare_to_proceed (void)
 {
   if (! ptid_equal (trap_ptid, null_ptid)
Index: linux-thread.c
===================================================================
RCS file: /cvs/src/src/gdb/linux-thread.c,v
retrieving revision 1.14
diff -u -5 -p -r1.14 linux-thread.c
--- linux-thread.c	2001/05/06 22:22:03	1.14
+++ linux-thread.c	2001/05/23 17:25:11
@@ -1038,11 +1038,14 @@ quit:
 
 /* If we have switched threads from a one that stopped at breakpoint,
    return 1 otherwise 0.
 
    Note that this implementation is potentially redundant now that
-   default_prepare_to_proceed() has been added.  */
+   default_prepare_to_proceed() has been added.
+
+   FIXME This may not support switching threads after Ctrl-C
+   correctly. The default implementation does support this. */
 
 int
 linuxthreads_prepare_to_proceed (int step)
 {
   if (!linuxthreads_max
Index: m3-nat.c
===================================================================
RCS file: /cvs/src/src/gdb/m3-nat.c,v
retrieving revision 1.13
diff -u -5 -p -r1.13 m3-nat.c
--- m3-nat.c	2001/05/04 04:15:25	1.13
+++ m3-nat.c	2001/05/23 17:25:14
@@ -1573,10 +1573,13 @@ mach_thread_output_id (int mid)
  *  if SELECT_IT is nonzero, reselect the thread that was active when
  *  we stopped at a breakpoint.
  *
  * Note that this implementation is potentially redundant now that
  * default_prepare_to_proceed() has been added.  
+ *
+ * FIXME This may not support switching threads after Ctrl-C
+ * correctly. The default implementation does support this.
  */
 
 mach3_prepare_to_proceed (int select_it)
 {
   if (stop_thread &&
From chastain@cygnus.com Wed May 23 11:11:00 2001
From: Michael Elizabeth Chastain <chastain@cygnus.com>
To: fnasser@redhat.com, msnyder@cygnus.com
Cc: chastain@cygnus.com, gdb-patches@sources.redhat.com
Subject: Re: [RFA] gdb.base/callfuncs.exp: make all test names unique
Date: Wed, 23 May 2001 11:11:00 -0000
Message-id: <200105231816.LAA13966@bosch.cygnus.com>
X-SW-Source: 2001-05/msg00439.html
Content-length: 661

chastain> ! gdb_test "next" "t_double_values\\(double_val1, double_val2\\);.*" "next 1"
chastain> ! gdb_test "next" "t_structs_c\\(struct_val1\\);.*" "next 2"

snyder> I wonder if it would not be preferable to use "next over t_double_values"
snyder> and "next over t_structs_c".

nasser> Michael (Chastain), please check it in your patch with these changes. 
nasser> Thank you very much to help eliminating these ambiguities.

Oops, actually that should be "next to t_double_values" rather than
"next over t_double_values" and so on.  It still looks better than
"next" or "next 1".

Works for me, I will use "next to t_double_values" and check it in.

Michael