Cell multi-arch broken (Re: [PATCH 2/2] GNU/Linux: Stop using libthread_db/td_ta_thr_iter)

Message ID m34mjkexxd.fsf@sspiff.org
State New, archived
Headers

Commit Message

Doug Evans Aug. 28, 2015, 6:20 a.m. UTC
  "Ulrich Weigand" <uweigand@de.ibm.com> writes:
> Ah, indeed that works for me.  The attached patch also fixes the
> problem for me.
>
> Bye,
> Ulrich
>
> Index: binutils-gdb/gdb/linux-thread-db.c
> ===================================================================
> --- binutils-gdb.orig/gdb/linux-thread-db.c
> +++ binutils-gdb/gdb/linux-thread-db.c
> @@ -1851,13 +1851,16 @@ thread_db_get_thread_local_address (stru
>    struct thread_info *thread_info;
>    struct target_ops *beneath;
>  
> -  /* If we have not discovered any threads yet, check now.  */
> -  if (!have_threads (ptid))
> -    thread_db_find_new_threads_1 (ptid);
> -
>    /* Find the matching thread.  */
>    thread_info = find_thread_ptid (ptid);
>  
> +  /* We may not have discovered the thread yet.  */
> +  if (thread_info != NULL && thread_info->priv == NULL)
> +    {
> +      thread_from_lwp (ptid);
> +      thread_info = find_thread_ptid (ptid);
> +    }
> +
>    if (thread_info != NULL && thread_info->priv != NULL)
>      {
>        td_err_e err;

Hi.

Just a thought.

It's kinda clumsy that thread_from_lwp ends with this:

  /* Fill the cache.  */
  tp = find_thread_ptid (ptid);
  record_thread (info, tp, ptid, &th, &ti);

and then we just call find_thread_ptid again after it returns:

> +      thread_from_lwp (ptid);
> +      thread_info = find_thread_ptid (ptid);

One might ask "Why doesn't thread_from_lwp just return thread_info?"

From record_thread things seem to be a be more subtle.
Even if we pass in a non-NULL TP we may still create a new one.

  /* Add the thread to GDB's thread list.  If we already know about a
     thread with this PTID, but it's marked exited, then the kernel
     reused the tid of an old thread.  */
  if (tp == NULL || tp->state == THREAD_EXITED)
    tp = add_thread_with_info (ptid, priv);
  else
    tp->priv = priv;

So it'd be helpful if record_thread also returned tp.
Sound about right?

2015-08-27  Doug Evans  <xdje42@gmail.com>

	* linux-thread-db.c (record_thread): Return the created thread.
	(thread_from_lwp): Likewise.
	(thread_db_get_thread_local_address): Update.
  

Comments

Pedro Alves Sept. 8, 2015, 10:31 a.m. UTC | #1
On 08/28/2015 07:20 AM, Doug Evans wrote:

> So it'd be helpful if record_thread also returned tp.
> Sound about right?

Sounds fine to me too.

Thanks,
Pedro Alves
  
Doug Evans Sept. 19, 2015, 6:20 a.m. UTC | #2
Doug Evans <xdje42@gmail.com> writes:
> "Ulrich Weigand" <uweigand@de.ibm.com> writes:
>> Ah, indeed that works for me.  The attached patch also fixes the
>> problem for me.
>>
>> Bye,
>> Ulrich
>>
>> Index: binutils-gdb/gdb/linux-thread-db.c
>> ===================================================================
>> --- binutils-gdb.orig/gdb/linux-thread-db.c
>> +++ binutils-gdb/gdb/linux-thread-db.c
>> @@ -1851,13 +1851,16 @@ thread_db_get_thread_local_address (stru
>>    struct thread_info *thread_info;
>>    struct target_ops *beneath;
>>  
>> -  /* If we have not discovered any threads yet, check now.  */
>> -  if (!have_threads (ptid))
>> -    thread_db_find_new_threads_1 (ptid);
>> -
>>    /* Find the matching thread.  */
>>    thread_info = find_thread_ptid (ptid);
>>  
>> +  /* We may not have discovered the thread yet.  */
>> +  if (thread_info != NULL && thread_info->priv == NULL)
>> +    {
>> +      thread_from_lwp (ptid);
>> +      thread_info = find_thread_ptid (ptid);
>> +    }
>> +
>>    if (thread_info != NULL && thread_info->priv != NULL)
>>      {
>>        td_err_e err;
>
> Hi.
>
> Just a thought.
>
> It's kinda clumsy that thread_from_lwp ends with this:
>
>   /* Fill the cache.  */
>   tp = find_thread_ptid (ptid);
>   record_thread (info, tp, ptid, &th, &ti);
>
> and then we just call find_thread_ptid again after it returns:
>
>> +      thread_from_lwp (ptid);
>> +      thread_info = find_thread_ptid (ptid);
>
> One might ask "Why doesn't thread_from_lwp just return thread_info?"
>
> From record_thread things seem to be a be more subtle.
> Even if we pass in a non-NULL TP we may still create a new one.
>
>   /* Add the thread to GDB's thread list.  If we already know about a
>      thread with this PTID, but it's marked exited, then the kernel
>      reused the tid of an old thread.  */
>   if (tp == NULL || tp->state == THREAD_EXITED)
>     tp = add_thread_with_info (ptid, priv);
>   else
>     tp->priv = priv;
>
> So it'd be helpful if record_thread also returned tp.
> Sound about right?
>
> 2015-08-27  Doug Evans  <xdje42@gmail.com>
>
> 	* linux-thread-db.c (record_thread): Return the created thread.
> 	(thread_from_lwp): Likewise.
> 	(thread_db_get_thread_local_address): Update.

Committed.
  

Patch

diff --git a/gdb/linux-thread-db.c b/gdb/linux-thread-db.c
index 855629b..b5719eb 100644
--- a/gdb/linux-thread-db.c
+++ b/gdb/linux-thread-db.c
@@ -198,10 +198,9 @@  static void thread_db_find_new_threads_2 (ptid_t ptid, int until_no_new);
 
 static void check_thread_signals (void);
 
-static void record_thread (struct thread_db_info *info,
-			   struct thread_info *tp,
-			   ptid_t ptid, const td_thrhandle_t *th_p,
-			   const td_thrinfo_t *ti_p);
+static struct thread_info *record_thread
+  (struct thread_db_info *info, struct thread_info *tp,
+   ptid_t ptid, const td_thrhandle_t *th_p, const td_thrinfo_t *ti_p);
 
 /* Add the current inferior to the list of processes using libpthread.
    Return a pointer to the newly allocated object that was added to
@@ -386,7 +385,7 @@  have_threads (ptid_t ptid)
 
 /* Fetch the user-level thread id of PTID.  */
 
-static void
+static struct thread_info *
 thread_from_lwp (ptid_t ptid)
 {
   td_thrhandle_t th;
@@ -419,7 +418,7 @@  thread_from_lwp (ptid_t ptid)
 
   /* Fill the cache.  */
   tp = find_thread_ptid (ptid);
-  record_thread (info, tp, ptid, &th, &ti);
+  return record_thread (info, tp, ptid, &th, &ti);
 }
 
 
@@ -1287,10 +1286,10 @@  attach_thread (ptid_t ptid, const td_thrhandle_t *th_p,
 }
 
 /* Record a new thread in GDB's thread list.  Creates the thread's
-   private info.  If TP is NULL, creates a new thread.  Otherwise,
-   uses TP.  */
+   private info.  If TP is NULL or TP is marked as having exited,
+   creates a new thread.  Otherwise, uses TP.  */
 
-static void
+static struct thread_info *
 record_thread (struct thread_db_info *info,
 	       struct thread_info *tp,
 	       ptid_t ptid, const td_thrhandle_t *th_p,
@@ -1304,7 +1303,7 @@  record_thread (struct thread_db_info *info,
      initialized yet.  Leave private == NULL until the thread library
      has initialized.  */
   if (ti_p->ti_tid == 0)
-    return;
+    return tp;
 
   /* Construct the thread's private data.  */
   priv = XCNEW (struct private_thread_info);
@@ -1333,6 +1332,8 @@  record_thread (struct thread_db_info *info,
 
   if (target_has_execution)
     check_thread_signals ();
+
+  return tp;
 }
 
 static void
@@ -1854,10 +1855,7 @@  thread_db_get_thread_local_address (struct target_ops *ops,
 
   /* We may not have discovered the thread yet.  */
   if (thread_info != NULL && thread_info->priv == NULL)
-    {
-      thread_from_lwp (ptid);
-      thread_info = find_thread_ptid (ptid);
-    }
+    thread_info = thread_from_lwp (ptid);
 
   if (thread_info != NULL && thread_info->priv != NULL)
     {