RFC: threads PREPARE_TO_PROCEED patch

RFC: threads PREPARE_TO_PROCEED patch

[Daniel Jacobowitz]

This patch aims to avoid problems when using a native GNU/Linux debugger
(which supports threads) with a remote protocol stub that supports threads. 
lin_lwp_prepare_to_proceed gets called anyway, but it doesn't function,
because the trap_ptid is (unsurprisingly) not set; since it's internal
accounting for lin-lwp.  This patch makes us use generic_prepare_to_proceed
instead of the old default_prepare_to_proceed or lin_lwp_prepare_to_proceed;
it can get everything it needs from infrun.  If I recall correctly, it also,
as per the FIXME, supports switching threads better than the lin_lwp version
did.  But I haven't tested that in a long time.


This is a patch from last year, just before 5.3 was branched.  At the time
it was decided to be generally right, but too risky for 5.3.  Well, now I've
gone and sat on it for too long, and we're coming up on 5.4.  But I've been
using this patch since August without any problems.  Depending on consensus
I'd like to check it in either before or after we branch for release.

Thoughts, thread maintainers?

-- 
Daniel Jacobowitz
MontaVista Software                         Debian GNU/Linux Developer

2003-03-13  Daniel Jacobowitz  <drow@mvista.com>

	* arch-utils.c (default_prepare_to_proceed): Remove.
	* arch-utils.h (default_prepare_to_proceed): Remove prototype.
	* gdbarch.sh (PREPARE_TO_PROCEED): Change default to
	generic_prepare_to_proceed.
	* gdbarch.c: Regenerated.
	* gdbarch.h: Regenerated.
	* lin-lwp.c (lin_lwp_prepare_to_proceed): Remove.
	* config/nm-linux.h (PREPARE_TO_PROCEED): Don't define.
	(lin_lwp_prepare_to_proceed): Remove prototype.

Index: arch-utils.c
===================================================================
RCS file: /cvs/src/src/gdb/arch-utils.c,v
retrieving revision 1.76
diff -u -p -r1.76 arch-utils.c
--- arch-utils.c	2 Mar 2003 10:23:09 -0000	1.76
+++ arch-utils.c	11 Mar 2003 14:34:22 -0000
@@ -313,13 +313,6 @@ no_op_reg_to_regnum (int reg)
   return reg;
 }
 
-/* Default prepare_to_procced().  */
-int
-default_prepare_to_proceed (int select_it)
-{
-  return 0;
-}
-
 /* Generic prepare_to_proceed().  This one should be suitable for most
    targets that support threads. */
 int
Index: arch-utils.h
===================================================================
RCS file: /cvs/src/src/gdb/arch-utils.h,v
retrieving revision 1.43
diff -u -p -r1.43 arch-utils.h
--- arch-utils.h	9 Dec 2002 02:04:16 -0000	1.43
+++ arch-utils.h	11 Mar 2003 14:34:22 -0000
@@ -111,8 +111,6 @@ extern int no_op_reg_to_regnum (int reg)
 
 /* Default prepare_to_procced. */
 
-extern int default_prepare_to_proceed (int select_it);
-
 extern int generic_prepare_to_proceed (int select_it);
 
 /* Versions of init_frame_pc().  Do nothing; do the default. */
Index: gdbarch.c
===================================================================
RCS file: /cvs/src/src/gdb/gdbarch.c,v
retrieving revision 1.188
diff -u -p -r1.188 gdbarch.c
--- gdbarch.c	10 Mar 2003 15:28:40 -0000	1.188
+++ gdbarch.c	11 Mar 2003 14:34:23 -0000
@@ -555,7 +555,7 @@ gdbarch_alloc (const struct gdbarch_info
   current_gdbarch->memory_insert_breakpoint = default_memory_insert_breakpoint;
   current_gdbarch->memory_remove_breakpoint = default_memory_remove_breakpoint;
   current_gdbarch->decr_pc_after_break = -1;
-  current_gdbarch->prepare_to_proceed = default_prepare_to_proceed;
+  current_gdbarch->prepare_to_proceed = generic_prepare_to_proceed;
   current_gdbarch->function_start_offset = -1;
   current_gdbarch->remote_translate_xfer_address = generic_remote_translate_xfer_address;
   current_gdbarch->frame_args_skip = -1;
Index: gdbarch.h
===================================================================
RCS file: /cvs/src/src/gdb/gdbarch.h,v
retrieving revision 1.151
diff -u -p -r1.151 gdbarch.h
--- gdbarch.h	10 Mar 2003 15:28:41 -0000	1.151
+++ gdbarch.h	11 Mar 2003 14:34:23 -0000
@@ -2083,7 +2083,7 @@ extern void set_gdbarch_decr_pc_after_br
 
 /* Default (function) for non- multi-arch platforms. */
 #if (!GDB_MULTI_ARCH) && !defined (PREPARE_TO_PROCEED)
-#define PREPARE_TO_PROCEED(select_it) (default_prepare_to_proceed (select_it))
+#define PREPARE_TO_PROCEED(select_it) (generic_prepare_to_proceed (select_it))
 #endif
 
 typedef int (gdbarch_prepare_to_proceed_ftype) (int select_it);
Index: gdbarch.sh
===================================================================
RCS file: /cvs/src/src/gdb/gdbarch.sh,v
retrieving revision 1.203
diff -u -p -r1.203 gdbarch.sh
--- gdbarch.sh	10 Mar 2003 15:28:41 -0000	1.203
+++ gdbarch.sh	11 Mar 2003 14:34:24 -0000
@@ -582,7 +582,7 @@ f:2:BREAKPOINT_FROM_PC:const unsigned ch
 f:2:MEMORY_INSERT_BREAKPOINT:int:memory_insert_breakpoint:CORE_ADDR addr, char *contents_cache:addr, contents_cache::0:default_memory_insert_breakpoint::0
 f:2:MEMORY_REMOVE_BREAKPOINT:int:memory_remove_breakpoint:CORE_ADDR addr, char *contents_cache:addr, contents_cache::0:default_memory_remove_breakpoint::0
 v:2:DECR_PC_AFTER_BREAK:CORE_ADDR:decr_pc_after_break::::0:-1
-f:2:PREPARE_TO_PROCEED:int:prepare_to_proceed:int select_it:select_it::0:default_prepare_to_proceed::0
+f:2:PREPARE_TO_PROCEED:int:prepare_to_proceed:int select_it:select_it::0:generic_prepare_to_proceed::0
 v:2:FUNCTION_START_OFFSET:CORE_ADDR:function_start_offset::::0:-1
 #
 f:2:REMOTE_TRANSLATE_XFER_ADDRESS:void:remote_translate_xfer_address:CORE_ADDR gdb_addr, int gdb_len, CORE_ADDR *rem_addr, int *rem_len:gdb_addr, gdb_len, rem_addr, rem_len:::generic_remote_translate_xfer_address::0
Index: lin-lwp.c
===================================================================
RCS file: /cvs/src/src/gdb/lin-lwp.c,v
retrieving revision 1.42
diff -u -p -r1.42 lin-lwp.c
--- lin-lwp.c	13 Jan 2003 21:47:35 -0000	1.42
+++ lin-lwp.c	11 Mar 2003 14:34:24 -0000
@@ -294,46 +294,6 @@ iterate_over_lwps (int (*callback) (stru
 }
 \f
 
-/* Implementation of the PREPARE_TO_PROCEED hook for the GNU/Linux LWP
-   layer.
-
-   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. */
-
-int
-lin_lwp_prepare_to_proceed (void)
-{
-  if (! ptid_equal (trap_ptid, null_ptid)
-      && ! ptid_equal (inferior_ptid, trap_ptid))
-    {
-      /* Switched over from TRAP_PID.  */
-      CORE_ADDR stop_pc = read_pc ();
-      CORE_ADDR trap_pc;
-
-      /* Avoid switching where it wouldn't do any good, i.e. if both
-         threads are at the same breakpoint.  */
-      trap_pc = read_pc_pid (trap_ptid);
-      if (trap_pc != stop_pc && breakpoint_here_p (trap_pc))
-	{
-	  /* User hasn't deleted the breakpoint.  Return non-zero, and
-             switch back to TRAP_PID.  */
-	  inferior_ptid = trap_ptid;
-
-	  /* FIXME: Is this stuff really necessary?  */
-	  flush_cached_frames ();
-	  registers_changed ();
-
-	  return 1;
-	}
-    }
-
-  return 0;
-}
-\f
-
 #if 0
 static void
 lin_lwp_open (char *args, int from_tty)
Index: config/nm-linux.h
===================================================================
RCS file: /cvs/src/src/gdb/config/nm-linux.h,v
retrieving revision 1.13
diff -u -p -r1.13 nm-linux.h
--- config/nm-linux.h	26 Nov 2002 01:23:46 -0000	1.13
+++ config/nm-linux.h	11 Mar 2003 14:34:24 -0000
@@ -49,9 +49,6 @@ struct target_waitstatus;
 extern ptid_t child_wait (ptid_t ptid, struct target_waitstatus *ourstatus);
 #define CHILD_WAIT
 
-extern int lin_lwp_prepare_to_proceed (void);
-#define PREPARE_TO_PROCEED(select_it) lin_lwp_prepare_to_proceed ()
-
 extern void lin_lwp_attach_lwp (ptid_t ptid, int verbose);
 #define ATTACH_LWP(ptid, verbose) lin_lwp_attach_lwp ((ptid), (verbose))
 

Re: RFC: threads PREPARE_TO_PROCEED patch

[Mark Kettenis]

Daniel Jacobowitz <drow@mvista.com> writes:

> This patch aims to avoid problems when using a native GNU/Linux debugger
> (which supports threads) with a remote protocol stub that supports threads. 
> lin_lwp_prepare_to_proceed gets called anyway, but it doesn't function,
> because the trap_ptid is (unsurprisingly) not set; since it's internal
> accounting for lin-lwp.  This patch makes us use generic_prepare_to_proceed
> instead of the old default_prepare_to_proceed or lin_lwp_prepare_to_proceed;
> it can get everything it needs from infrun.  If I recall correctly, it also,
> as per the FIXME, supports switching threads better than the lin_lwp version
> did.  But I haven't tested that in a long time.
> 
> This is a patch from last year, just before 5.3 was branched.  At the time
> it was decided to be generally right, but too risky for 5.3.  Well, now I've
> gone and sat on it for too long, and we're coming up on 5.4.  But I've been
> using this patch since August without any problems.  Depending on consensus
> I'd like to check it in either before or after we branch for release.
> 
> Thoughts, thread maintainers?

I'm not quite sure whether changing the gdbarch default is a good
idea, but replacing lin_lwp_prepare_to_proceed with
generic_prepare_to_proceed has been the intention all along.

Mark

Re: RFC: threads PREPARE_TO_PROCEED patch

[Daniel Jacobowitz]

On Fri, Mar 14, 2003 at 06:20:45PM +0100, Mark Kettenis wrote:
> Daniel Jacobowitz <drow@mvista.com> writes:
> 
> > This patch aims to avoid problems when using a native GNU/Linux debugger
> > (which supports threads) with a remote protocol stub that supports threads. 
> > lin_lwp_prepare_to_proceed gets called anyway, but it doesn't function,
> > because the trap_ptid is (unsurprisingly) not set; since it's internal
> > accounting for lin-lwp.  This patch makes us use generic_prepare_to_proceed
> > instead of the old default_prepare_to_proceed or lin_lwp_prepare_to_proceed;
> > it can get everything it needs from infrun.  If I recall correctly, it also,
> > as per the FIXME, supports switching threads better than the lin_lwp version
> > did.  But I haven't tested that in a long time.
> > 
> > This is a patch from last year, just before 5.3 was branched.  At the time
> > it was decided to be generally right, but too risky for 5.3.  Well, now I've
> > gone and sat on it for too long, and we're coming up on 5.4.  But I've been
> > using this patch since August without any problems.  Depending on consensus
> > I'd like to check it in either before or after we branch for release.
> > 
> > Thoughts, thread maintainers?
> 
> I'm not quite sure whether changing the gdbarch default is a good
> idea, but replacing lin_lwp_prepare_to_proceed with
> generic_prepare_to_proceed has been the intention all along.

Well, let me describe the problem I'm trying to solve; I'd like your
opinion on how to approach it.  When using gdbserver, we need to have
generic_prepare_to_proceed.  Not the lin_lwp version, and not the
"default" one from arch-utils.  The former won't work and the latter
doesn't do enough.  So cross debuggers need to pick this up.

Note that this is a property of the target.  Not of the architecture. 
I'm not sure PREPARE_TO_PROCEED belongs in gdbarch at all.  It's only
defined by Mach3, HP/UX, and Linux; it's undefined for x86-64-linux
(why???).  I could set it in all the Linux gdbarch init functions that
I care about, but that doesn't seem like much of a solution.

-- 
Daniel Jacobowitz
MontaVista Software                         Debian GNU/Linux Developer

Re: RFC: threads PREPARE_TO_PROCEED patch

[Mark Kettenis]

   Date: Fri, 14 Mar 2003 13:35:29 -0500
   From: Daniel Jacobowitz <drow@mvista.com>

   > I'm not quite sure whether changing the gdbarch default is a good
   > idea, but replacing lin_lwp_prepare_to_proceed with
   > generic_prepare_to_proceed has been the intention all along.

   Well, let me describe the problem I'm trying to solve; I'd like your
   opinion on how to approach it.  When using gdbserver, we need to have
   generic_prepare_to_proceed.  Not the lin_lwp version, and not the
   "default" one from arch-utils.  The former won't work and the latter
   doesn't do enough.  So cross debuggers need to pick this up.

OK, but generic_prepare_to_proceed() is perfectly usable on a native
GNU/Linux GDB too, isn't it?

   Note that this is a property of the target.  Not of the architecture. 
   I'm not sure PREPARE_TO_PROCEED belongs in gdbarch at all.  It's only
   defined by Mach3, HP/UX, and Linux; it's undefined for x86-64-linux
   (why???).  I could set it in all the Linux gdbarch init functions that
   I care about, but that doesn't seem like much of a solution.

It seems to be a property of the OS to me.  In its current
incarnation, gdbarch does includes details of both the architecture
(ISA) and the OS (OS/ABI).  So gdbarch seems to be the correct place
for PREPARE_TO_PROCEED to me.  So yes, I think you should add it to
all relevant Linux gdbarch init functions.

Mark

Re: RFC: threads PREPARE_TO_PROCEED patch

[Daniel Jacobowitz]

On Sat, Mar 15, 2003 at 09:39:14PM +0100, Mark Kettenis wrote:
>    Date: Fri, 14 Mar 2003 13:35:29 -0500
>    From: Daniel Jacobowitz <drow@mvista.com>
> 
>    > I'm not quite sure whether changing the gdbarch default is a good
>    > idea, but replacing lin_lwp_prepare_to_proceed with
>    > generic_prepare_to_proceed has been the intention all along.
> 
>    Well, let me describe the problem I'm trying to solve; I'd like your
>    opinion on how to approach it.  When using gdbserver, we need to have
>    generic_prepare_to_proceed.  Not the lin_lwp version, and not the
>    "default" one from arch-utils.  The former won't work and the latter
>    doesn't do enough.  So cross debuggers need to pick this up.
> 
> OK, but generic_prepare_to_proceed() is perfectly usable on a native
> GNU/Linux GDB too, isn't it?

Yes, exactly.

>    Note that this is a property of the target.  Not of the architecture. 
>    I'm not sure PREPARE_TO_PROCEED belongs in gdbarch at all.  It's only
>    defined by Mach3, HP/UX, and Linux; it's undefined for x86-64-linux
>    (why???).  I could set it in all the Linux gdbarch init functions that
>    I care about, but that doesn't seem like much of a solution.
> 
> It seems to be a property of the OS to me.  In its current
> incarnation, gdbarch does includes details of both the architecture
> (ISA) and the OS (OS/ABI).  So gdbarch seems to be the correct place
> for PREPARE_TO_PROCEED to me.  So yes, I think you should add it to
> all relevant Linux gdbarch init functions.

I can do that; I'll put a patch together.

But I must admit that I don't really agree.  It seems to be a property
of the threads implementation for the target instead. Consider this
case: if someone wanted to write a remote protocol stub for HP/UX. 
They wouldn't want the HP/UX version of PREPARE_TO_PROCEED naturally,
since that's native-only.  They'd want most likely
generic_prepare_to_proceed.  The default function isn't useful; it
doesn't support switching threads correctly.

(Incidentally, from reading the HP/UX implementation, I believe that
using generic_prepare_to_proceed would work there too.  It wouldn't
work for the Mach 3.0 implementation as-is but I think it could be made
to work.  I'm not volunteering; are either hppa*-*-osf* or
mips*-*-mach3* still living?  Perhaps we should deprecate them next
release.)

-- 
Daniel Jacobowitz
MontaVista Software                         Debian GNU/Linux Developer

Re: RFC: threads PREPARE_TO_PROCEED patch

[Andrew Cagney]

> On Sat, Mar 15, 2003 at 09:39:14PM +0100, Mark Kettenis wrote:
> 
>>    Date: Fri, 14 Mar 2003 13:35:29 -0500
>>    From: Daniel Jacobowitz <drow@mvista.com>
>> 
>>    > I'm not quite sure whether changing the gdbarch default is a good
>>    > idea, but replacing lin_lwp_prepare_to_proceed with
>>    > generic_prepare_to_proceed has been the intention all along.
>> 
>>    Well, let me describe the problem I'm trying to solve; I'd like your
>>    opinion on how to approach it.  When using gdbserver, we need to have
>>    generic_prepare_to_proceed.  Not the lin_lwp version, and not the
>>    "default" one from arch-utils.  The former won't work and the latter
>>    doesn't do enough.  So cross debuggers need to pick this up.
>> 
>> OK, but generic_prepare_to_proceed() is perfectly usable on a native
>> GNU/Linux GDB too, isn't it?
> 
> 
> Yes, exactly.

Needing to change one of GDB's architecture methods dependant on a 
target attribute (local or remote) suggests that the method should be in 
the target vector.

>>    Note that this is a property of the target.  Not of the architecture. 
>>    I'm not sure PREPARE_TO_PROCEED belongs in gdbarch at all.  It's only
>>    defined by Mach3, HP/UX, and Linux; it's undefined for x86-64-linux
>>    (why???).  I could set it in all the Linux gdbarch init functions that
>>    I care about, but that doesn't seem like much of a solution.
>> 
>> It seems to be a property of the OS to me.  In its current
>> incarnation, gdbarch does includes details of both the architecture
>> (ISA) and the OS (OS/ABI).  So gdbarch seems to be the correct place
>> for PREPARE_TO_PROCEED to me.  So yes, I think you should add it to
>> all relevant Linux gdbarch init functions.
> 
> 
> I can do that; I'll put a patch together.
> 
> But I must admit that I don't really agree.  It seems to be a property
> of the threads implementation for the target instead. Consider this
> case: if someone wanted to write a remote protocol stub for HP/UX. 
> They wouldn't want the HP/UX version of PREPARE_TO_PROCEED naturally,
> since that's native-only.  They'd want most likely
> generic_prepare_to_proceed.  The default function isn't useful; it
> doesn't support switching threads correctly.
> 
> (Incidentally, from reading the HP/UX implementation, I believe that
> using generic_prepare_to_proceed would work there too.  It wouldn't
> work for the Mach 3.0 implementation as-is but I think it could be made
> to work.  I'm not volunteering; are either hppa*-*-osf* or
> mips*-*-mach3* still living?  Perhaps we should deprecate them next
> release.)

Obsoleting hppa*-*-osf* was sent to announce list (no objection so far). 
  Can you formally propose removing mips*mach3 here (and create a bug 
report) that can quickly happen.

If you both really think that PREPARE_TO_PROCEED is wrong, deprecate it 
(details left as an exercise for the reader).

Andrew

Re: RFC: threads PREPARE_TO_PROCEED patch

[Andrew Cagney]

[picking up old thread sitting in my in box, added AndreasS]

Can I suggest:

- moving default_prepare_to_proceed() to infrun.c, make it static, 
rename it to prepare_to_proceed().
- removing PREPARE_TO_PROCEED from the architecture vector
- renaming HP's (and Mach) PREPARE_TO_PROCEED to 
DEPRECATED_PREPARE_TO_PROCEED
- tweaking the PREPARE_TO_PROCEED code to use #ifdef'd 
DEPRECATED_PREPARE_TO_PROCEED if available.

Andrew

Re: RFC: threads PREPARE_TO_PROCEED patch

[Daniel Jacobowitz]

On Mon, May 26, 2003 at 03:54:19PM -0400, Andrew Cagney wrote:
> [picking up old thread sitting in my in box, added AndreasS]
> 
> Can I suggest:
> 
> - moving default_prepare_to_proceed() to infrun.c, make it static, 
> rename it to prepare_to_proceed().
> - removing PREPARE_TO_PROCEED from the architecture vector
> - renaming HP's (and Mach) PREPARE_TO_PROCEED to 
> DEPRECATED_PREPARE_TO_PROCEED
> - tweaking the PREPARE_TO_PROCEED code to use #ifdef'd 
> DEPRECATED_PREPARE_TO_PROCEED if available.

That sounds reasonable to me, and saves me having to go and test my
theories on HP/UX :)

-- 
Daniel Jacobowitz
MontaVista Software                         Debian GNU/Linux Developer