[rfa/threads] Convert thread event descriptors to code addrs

[rfa/threads] Convert thread event descriptors to code addrs

[Andrew Cagney]

[-- Attachment #1: Type: text/plain, Size: 602 bytes --]

Hello,

For PPC64 every function has two minimal symbols vis:

.__nptl_create_event: the start address
__nptl_create_event: the descriptor

This patch modifies ps_pglobal_lookup so that it always returns the 
function's start address.  Doing this ensures that libthread_db and 
GDB's thread code are "on the same page" when it comes to the true 
address of the thread-create and thread-death breakpoints.

The alternative would be to modify libthread_db so that it knew that 
PPC64 symbol were special but I suspect that it doesn't want to know 
about such underlying details.

ok for mainline?
Andrew

[-- Attachment #2: diffs --]
[-- Type: text/plain, Size: 1012 bytes --]

2003-11-25  Andrew Cagney  <cagney@redhat.com>

	* proc-service.c (ps_pglobal_lookup): Convert function descriptors
	into code addresses using gdbarch_convert_from_func_ptr_addr.

Index: ./gdb/proc-service.c
===================================================================
RCS file: /cvs/src/src/gdb/proc-service.c,v
retrieving revision 1.7
diff -u -r1.7 proc-service.c
--- ./gdb/proc-service.c	24 Feb 2002 22:31:19 -0000	1.7
+++ ./gdb/proc-service.c	25 Nov 2003 20:59:49 -0000
@@ -181,7 +181,13 @@
   if (ms == NULL)
     return PS_NOSYM;
 
-  *sym_addr = SYMBOL_VALUE_ADDRESS (ms);
+  /* Get the addres, make certain that any descriptors are converted
+     into corresponding code addresses.  (For PPC64, the symbol
+     "__nptl_create_event" points at a function descriptor while this
+     code needs the corresponding function's start address.)  */
+  *sym_addr = gdbarch_convert_from_func_ptr_addr (current_gdbarch,
+						  SYMBOL_VALUE_ADDRESS (ms),
+						  &current_target);
   return PS_OK;
 }
 

Re: [rfa/threads] Convert thread event descriptors to code addrs

[Michael Snyder]

Andrew Cagney wrote:
> Hello,
> 
> For PPC64 every function has two minimal symbols vis:
> 
> .__nptl_create_event: the start address
> __nptl_create_event: the descriptor
> 
> This patch modifies ps_pglobal_lookup so that it always returns the 
> function's start address.  Doing this ensures that libthread_db and 
> GDB's thread code are "on the same page" when it comes to the true 
> address of the thread-create and thread-death breakpoints.
> 
> The alternative would be to modify libthread_db so that it knew that 
> PPC64 symbol were special but I suspect that it doesn't want to know 
> about such underlying details.
> 
> ok for mainline?

Yes, with typo correction noted below.

> ------------------------------------------------------------------------
> 
> 2003-11-25  Andrew Cagney  <cagney@redhat.com>
> 
> 	* proc-service.c (ps_pglobal_lookup): Convert function descriptors
> 	into code addresses using gdbarch_convert_from_func_ptr_addr.
> 
> Index: ./gdb/proc-service.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/proc-service.c,v
> retrieving revision 1.7
> diff -u -r1.7 proc-service.c
> --- ./gdb/proc-service.c	24 Feb 2002 22:31:19 -0000	1.7
> +++ ./gdb/proc-service.c	25 Nov 2003 20:59:49 -0000
> @@ -181,7 +181,13 @@
>    if (ms == NULL)
>      return PS_NOSYM;
>  
> -  *sym_addr = SYMBOL_VALUE_ADDRESS (ms);
> +  /* Get the addres, make certain that any descriptors are converted

"address".

> +     into corresponding code addresses.  (For PPC64, the symbol
> +     "__nptl_create_event" points at a function descriptor while this
> +     code needs the corresponding function's start address.)  */
> +  *sym_addr = gdbarch_convert_from_func_ptr_addr (current_gdbarch,
> +						  SYMBOL_VALUE_ADDRESS (ms),
> +						  &current_target);
>    return PS_OK;
>  }
>  

Re: [rfa/threads] Convert thread event descriptors to code addrs

[Roland McGrath]

> The alternative would be to modify libthread_db so that it knew that 
> PPC64 symbol were special but I suspect that it doesn't want to know 
> about such underlying details.

It seems to me that this is really what ought to change.  ps_pglobal_lookup
and similar symbols have always had an architecture-neutral interface (with
the exception of our invention, ps_get_thread_area).  There is no reason
the implementation of this function by a user of libthread_db should have
to do anything machine-specific.  gdb already has tons of machine-specific
knowledge, so it's not really an issue there; but if there are ever simpler
users of libthread_db, they shouldn't have to worry about this.  It is easy
enough to change libthread_db to request the symbols it really wants for
the addresses it needs.  

Depending how convert_from_func_ptr_addr works, it may well be reasonable
for gdb to use it as your patch does, to work around the existing buggy
PPC64 libthread_db implementations.  That is, if that transformation will
always be a no-op for a real code symbol (i.e. a ".foo" on PPC64), then you
might as well make gdb seamlessly handle either case.  I'll leave that up
to you, but do let me know whether you decide to take or leave it, so I
know how urgent the glibc fix is for gdb users.


Thanks,
Roland

Re: [rfa/threads] Convert thread event descriptors to code addrs

[Andrew Cagney]

Does NPTL do anything with that address internally?  Or just pass it 
through?

> The alternative would be to modify libthread_db so that it knew that 
>> PPC64 symbol were special but I suspect that it doesn't want to know 
>> about such underlying details.
> 
> 
> It seems to me that this is really what ought to change.  ps_pglobal_lookup
> and similar symbols have always had an architecture-neutral interface (with
> the exception of our invention, ps_get_thread_area).  There is no reason
> the implementation of this function by a user of libthread_db should have
> to do anything machine-specific.  gdb already has tons of machine-specific
> knowledge, so it's not really an issue there; but if there are ever simpler
> users of libthread_db, they shouldn't have to worry about this.  It is easy
> enough to change libthread_db to request the symbols it really wants for
> the addresses it needs.  
> 
> Depending how convert_from_func_ptr_addr works, it may well be reasonable
> for gdb to use it as your patch does, to work around the existing buggy
> PPC64 libthread_db implementations.  That is, if that transformation will
> always be a no-op for a real code symbol (i.e. a ".foo" on PPC64), then you
> might as well make gdb seamlessly handle either case.  I'll leave that up
> to you, but do let me know whether you decide to take or leave it, so I
> know how urgent the glibc fix is for gdb users.
> 
> 

Re: [rfa/threads] Convert thread event descriptors to code addrs

[Roland McGrath]

> Does NPTL do anything with that address internally?  Or just pass it 
> through?

It just returns it for td_ta_event_addr.  The interface of that function is
that NOTIFY_BPT specifies an address at which to place a breakpoint, so it
seems appropriate that it pass back the actual code address.
Will that cause any confusion?


Thanks,
Roland

Re: [rfa/threads] Convert thread event descriptors to code addrs

[Andrew Cagney]

[-- Attachment #1: Type: text/plain, Size: 599 bytes --]

In the light of roland's comments, I've checked in the attached 
variation on the original patch.

It still does the conversion but in GDB's libthread_db caller 
(enable_thread_event_reporting) and not in libthread_db's symbol lookup 
callee (ps_pglobal_lookup).

This way, libthread_db is free to search for either:
.__nptl_create_event: the start address
__nptl_create_event: the descriptor
(the original change would have restricted searches to just the start 
address - not a problem now but we never know) and at the same time 
ensure that GDB sets breakpoints at the address it needs.

Andrew

[-- Attachment #2: diffs --]
[-- Type: text/plain, Size: 2358 bytes --]

2003-11-25  Andrew Cagney  <cagney@redhat.com>

	* thread-db.c (enable_thread_event): New function.  Ensure that BP
	is a code address.
	(enable_thread_event_reporting): Use enable_thread_event.
	
Index: ./gdb/thread-db.c
===================================================================
RCS file: /cvs/src/src/gdb/thread-db.c,v
retrieving revision 1.34
diff -u -r1.34 thread-db.c
--- ./gdb/thread-db.c	4 Sep 2003 21:03:37 -0000	1.34
+++ ./gdb/thread-db.c	25 Nov 2003 23:28:49 -0000
@@ -465,6 +465,26 @@
   return 1;
 }
 
+static int
+enable_thread_event (td_thragent_t *thread_agent, int event, CORE_ADDR *bp)
+{
+  td_notify_t notify;
+  int err;
+
+  /* Get the breakpoint address for thread EVENT.  */
+  err = td_ta_event_addr_p (thread_agent, event, &notify);
+  if (err != TD_OK)
+    return 0;
+
+  /* Set up the breakpoint.  */
+  (*bp) = gdbarch_convert_from_func_ptr_addr (current_gdbarch,
+					      (CORE_ADDR) notify.u.bptaddr,
+					      &current_target);
+  create_thread_event_breakpoint ((*bp));
+
+  return 1;
+}
+
 static void
 enable_thread_event_reporting (void)
 {
@@ -498,32 +518,24 @@
 
   /* Delete previous thread event breakpoints, if any.  */
   remove_thread_event_breakpoints ();
+  td_create_bp_addr = 0;
+  td_death_bp_addr = 0;
 
-  /* Get address for thread creation breakpoint.  */
-  err = td_ta_event_addr_p (thread_agent, TD_CREATE, &notify);
-  if (err != TD_OK)
+  /* Set up the thread creation event.  */
+  if (!enable_thread_event (thread_agent, TD_CREATE, &td_create_bp_addr))
     {
       warning ("Unable to get location for thread creation breakpoint: %s",
 	       thread_db_err_str (err));
       return;
     }
 
-  /* Set up the breakpoint.  */
-  td_create_bp_addr = (CORE_ADDR) notify.u.bptaddr;
-  create_thread_event_breakpoint (td_create_bp_addr);
-
-  /* Get address for thread death breakpoint.  */
-  err = td_ta_event_addr_p (thread_agent, TD_DEATH, &notify);
-  if (err != TD_OK)
+  /* Set up the thread death event.  */
+  if (!enable_thread_event (thread_agent, TD_DEATH, &td_death_bp_addr))
     {
       warning ("Unable to get location for thread death breakpoint: %s",
 	       thread_db_err_str (err));
       return;
     }
-
-  /* Set up the breakpoint.  */
-  td_death_bp_addr = (CORE_ADDR) notify.u.bptaddr;
-  create_thread_event_breakpoint (td_death_bp_addr);
 }
 
 static void

Re: [rfa/threads] Convert thread event descriptors to code addrs

[Daniel Jacobowitz]

On Tue, Nov 25, 2003 at 06:39:42PM -0500, Andrew Cagney wrote:
> In the light of roland's comments, I've checked in the attached 
> variation on the original patch.
> 
> It still does the conversion but in GDB's libthread_db caller 
> (enable_thread_event_reporting) and not in libthread_db's symbol lookup 
> callee (ps_pglobal_lookup).
> 
> This way, libthread_db is free to search for either:
> .__nptl_create_event: the start address
> __nptl_create_event: the descriptor
> (the original change would have restricted searches to just the start 
> address - not a problem now but we never know) and at the same time 
> ensure that GDB sets breakpoints at the address it needs.
> 
> Andrew

> 2003-11-25  Andrew Cagney  <cagney@redhat.com>
> 
> 	* thread-db.c (enable_thread_event): New function.  Ensure that BP
> 	is a code address.
> 	(enable_thread_event_reporting): Use enable_thread_event.

Hmm.  Does this mean remote_lookup_symbol (spelling?) should do the
same thing?

-- 
Daniel Jacobowitz
MontaVista Software                         Debian GNU/Linux Developer

Re: [rfa/threads] Convert thread event descriptors to code addrs

[Andrew Cagney]

> On Tue, Nov 25, 2003 at 06:39:42PM -0500, Andrew Cagney wrote:
> 
>> In the light of roland's comments, I've checked in the attached 
>> variation on the original patch.
>> 
>> It still does the conversion but in GDB's libthread_db caller 
>> (enable_thread_event_reporting) and not in libthread_db's symbol lookup 
>> callee (ps_pglobal_lookup).
>> 
>> This way, libthread_db is free to search for either:
>> .__nptl_create_event: the start address
>> __nptl_create_event: the descriptor
>> (the original change would have restricted searches to just the start 
>> address - not a problem now but we never know) and at the same time 
>> ensure that GDB sets breakpoints at the address it needs.
>> 
>> Andrew
> 
> 
>> 2003-11-25  Andrew Cagney  <cagney@redhat.com>
>> 
>> 	* thread-db.c (enable_thread_event): New function.  Ensure that BP
>> 	is a code address.
>> 	(enable_thread_event_reporting): Use enable_thread_event.
> 
> 
> Hmm.  Does this mean remote_lookup_symbol (spelling?) should do the
> same thing?

(remote_check_symbols?)  Wouldn't that be equivalent to my original 
patch (wrong side of libthread_db)?  I suspect you want to add something 
to gdbserver (or just wait for Roland's updated libthread_db).

Andrew

Re: [rfa/threads] Convert thread event descriptors to code addrs

[Daniel Jacobowitz]

On Wed, Nov 26, 2003 at 12:28:38PM -0500, Andrew Cagney wrote:
> >On Tue, Nov 25, 2003 at 06:39:42PM -0500, Andrew Cagney wrote:
> >
> >>In the light of roland's comments, I've checked in the attached 
> >>variation on the original patch.
> >>
> >>It still does the conversion but in GDB's libthread_db caller 
> >>(enable_thread_event_reporting) and not in libthread_db's symbol lookup 
> >>callee (ps_pglobal_lookup).
> >>
> >>This way, libthread_db is free to search for either:
> >>.__nptl_create_event: the start address
> >>__nptl_create_event: the descriptor
> >>(the original change would have restricted searches to just the start 
> >>address - not a problem now but we never know) and at the same time 
> >>ensure that GDB sets breakpoints at the address it needs.
> >>
> >>Andrew
> >
> >
> >>2003-11-25  Andrew Cagney  <cagney@redhat.com>
> >>
> >>	* thread-db.c (enable_thread_event): New function.  Ensure that BP
> >>	is a code address.
> >>	(enable_thread_event_reporting): Use enable_thread_event.
> >
> >
> >Hmm.  Does this mean remote_lookup_symbol (spelling?) should do the
> >same thing?
> 
> (remote_check_symbols?)  Wouldn't that be equivalent to my original 
> patch (wrong side of libthread_db)?  I suspect you want to add something 
> to gdbserver (or just wait for Roland's updated libthread_db).

Any remote client asking for a function address is going to want to put
a breakpoint there, I would have guessed.  Thus we should return the
breakpointable address.

Hum, maybe there is some use in having the descriptor... let me think
about it.  This should go into the remote protocol doco one way or the
other.

-- 
Daniel Jacobowitz
MontaVista Software                         Debian GNU/Linux Developer

Re: [rfa/threads] Convert thread event descriptors to code addrs

[Andrew Cagney]

>> (remote_check_symbols?)  Wouldn't that be equivalent to my original 
>> patch (wrong side of libthread_db)?  I suspect you want to add something 
>> to gdbserver (or just wait for Roland's updated libthread_db).
> 
> 
> Any remote client asking for a function address is going to want to put
> a breakpoint there, I would have guessed.  Thus we should return the
> breakpointable address.

Suggest re-reading roland's comments.  The contract is to convert a 
symbol "foo" to its value.  Not convert a symbol foo to a code address 
by doing a descriptor re-direction.

> Hum, maybe there is some use in having the descriptor... let me think
> about it.  This should go into the remote protocol doco one way or the
> other.

I think the protocol is pretty clear here.

Andrew

Re: [rfa/threads] Convert thread event descriptors to code addrs

[Roland McGrath]

> Does NPTL do anything with that address internally?  Or just pass it 
> through?

Note, btw, that LinuxThreads and NPTL both have the same issue here,
and the answers I've given apply to both.