Updated patch for Bug 13217 - thread apply all detach throws a SEGFAULT

Updated patch for Bug 13217 - thread apply all detach throws a SEGFAULT

[ali_anwar]

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

Fixed a formating issue...

Attached patch is for Bug 13217 - "thread apply all detach throws a 
SEGFAULT".

Issue:
Segmentation fault was coming from invalid access to tp->* i.e "tp->ptid".

Fix:
Check if the thread is valid before we apply a command
Check for the thread count after executing a command on every thread

Testing:
* Tested on powerpc-linux as well as i686-pc-linux
* Also checked with valgrind

Test case:
Added a test case to test "thread apply all detach" command

Regards,
-Ali



[-- Attachment #2: Bug13217.patch --]
[-- Type: text/x-patch, Size: 2839 bytes --]

Index: ChangeLog
===================================================================
RCS file: /cvs/src/src/ChangeLog,v
retrieving revision 1.1036
diff -u -r1.1036 ChangeLog
--- ChangeLog	21 Sep 2012 15:16:59 -0000	1.1036
+++ ChangeLog	26 Sep 2012 09:51:12 -0000
@@ -1,3 +1,9 @@
+2012-09-21  Ali Anwar  <ali_anwar@codesourcery.com>
+
+	PR threads/13217
+	* thread.c (thread_valid): New function.
+	(thread_apply_all_command): Check for valid thread and thread count.
+
 2012-09-21  Steve Ellcey  <sellcey@mips.com>
 
 	* configure.ac: Add mips*-mti-elf* target.
Index: gdb/thread.c
===================================================================
RCS file: /cvs/src/src/gdb/thread.c,v
retrieving revision 1.149
diff -u -r1.149 thread.c
--- gdb/thread.c	27 Jul 2012 00:52:36 -0000	1.149
+++ gdb/thread.c	26 Sep 2012 09:51:13 -0000
@@ -57,6 +57,7 @@
 struct thread_info *thread_list = NULL;
 static int highest_thread_num;
 
+static int thread_valid (struct thread_info *);
 static void thread_command (char *tidstr, int from_tty);
 static void thread_apply_all_command (char *, int);
 static int thread_alive (struct thread_info *);
@@ -73,6 +74,17 @@
   return tp;
 }
 
+/* Return true if TP is valid thread. */
+static int
+thread_valid (struct thread_info *tp)
+{
+  struct thread_info  *utp;
+  for (utp = thread_list; utp; utp = utp->next)
+     if (tp == utp)
+       return 1;
+  return 0;
+}
+
 void
 delete_step_resume_breakpoint (struct thread_info *tp)
 {
@@ -1193,7 +1205,7 @@
      execute_command.  */
   saved_cmd = xstrdup (cmd);
   make_cleanup (xfree, saved_cmd);
-  for (tp = thread_list; tp; tp = tp->next)
+  for (tp = thread_list; thread_valid(tp); tp = tp->next)
     if (thread_alive (tp))
       {
 	switch_to_thread (tp->ptid);
@@ -1203,6 +1215,8 @@
 	execute_command (cmd, from_tty);
 	strcpy (cmd, saved_cmd);	/* Restore exact command used
 					   previously.  */
+	if (thread_count() == 0)
+	  break;
       }
 
   do_cleanups (old_chain);
Index: gdb/testsuite/gdb.threads/threadapply.exp
===================================================================
RCS file: /cvs/src/src/gdb/testsuite/gdb.threads/threadapply.exp,v
retrieving revision 1.15
diff -u -r1.15 threadapply.exp
--- gdb/testsuite/gdb.threads/threadapply.exp	26 Jun 2012 19:23:20 -0000	1.15
+++ gdb/testsuite/gdb.threads/threadapply.exp	26 Sep 2012 09:51:14 -0000
@@ -63,3 +63,4 @@
 gdb_test "up" ".*in main.*" "go up in the stack frame" 
 gdb_test "thread apply all print 1"  "Thread ..*\\\$\[0-9]+ = 1.*Thread ..*\\\$\[0-9]+ = 1.*Thread ..*\\\$\[0-9]+ = 1.*Thread ..*\\\$\[0-9]+ = 1.*Thread ..*\\\$\[0-9]+ = 1.*Thread ..*\\\$\[0-9]+ = 1" "run a simple print command on all threads"
 gdb_test "down" "#0.*thread_function.*" "go down and check selected frame"
+gdb_test "thread apply all detach" "Detaching from.*" "detach all threads"

Re: Updated patch for Bug 13217 - thread apply all detach throws a SEGFAULT

[ali_anwar]

On 09/26/2012 04:44 PM, ali_anwar wrote:
> Fixed a formating issue...
>
> Attached patch is for Bug 13217 - "thread apply all detach throws a 
> SEGFAULT".
>
> Issue:
> Segmentation fault was coming from invalid access to tp->* i.e 
> "tp->ptid".
>
> Fix:
> Check if the thread is valid before we apply a command
> Check for the thread count after executing a command on every thread
>
> Testing:
> * Tested on powerpc-linux as well as i686-pc-linux
> * Also checked with valgrind
>
> Test case:
> Added a test case to test "thread apply all detach" command
>
> Regards,
> -Ali
>
>
My last patch was not recieved so please ignore the line "Fixed the 
formating issue...".

-Ali

Re: Updated patch for Bug 13217 - thread apply all detach throws a SEGFAULT

[Jan Kratochvil]

On Wed, 26 Sep 2012 13:44:27 +0200, ali_anwar wrote:
[...]
> --- gdb/thread.c	27 Jul 2012 00:52:36 -0000	1.149
> +++ gdb/thread.c	26 Sep 2012 09:51:13 -0000
> @@ -57,6 +57,7 @@
>  struct thread_info *thread_list = NULL;
>  static int highest_thread_num;
>  
> +static int thread_valid (struct thread_info *);

Forward declaration not needed when all callers are after the function
definition like in this case.


>  static void thread_command (char *tidstr, int from_tty);
>  static void thread_apply_all_command (char *, int);
>  static int thread_alive (struct thread_info *);
> @@ -73,6 +74,17 @@
>    return tp;
>  }
>  
> +/* Return true if TP is valid thread. */

Empty line between a function comment tnd the function implementation.
Also the comment should end with two spaces:
	/* Return true if TP is valid thread.  */

> +static int
> +thread_valid (struct thread_info *tp)
> +{
> +  struct thread_info  *utp;

Two spaces, use one.  Use empty line between declarations and code statements.

> +  for (utp = thread_list; utp; utp = utp->next)
> +     if (tp == utp)
> +       return 1;
> +  return 0;
> +}
> +
>  void
>  delete_step_resume_breakpoint (struct thread_info *tp)
>  {
> @@ -1193,7 +1205,7 @@
>       execute_command.  */
>    saved_cmd = xstrdup (cmd);
>    make_cleanup (xfree, saved_cmd);
> -  for (tp = thread_list; tp; tp = tp->next)
> +  for (tp = thread_list; thread_valid(tp); tp = tp->next)

Space before function parameters:
	thread_valid (tp)


>      if (thread_alive (tp))
>        {
>  	switch_to_thread (tp->ptid);
> @@ -1203,6 +1215,8 @@
>  	execute_command (cmd, from_tty);
>  	strcpy (cmd, saved_cmd);	/* Restore exact command used
>  					   previously.  */
> +	if (thread_count() == 0)

Space before function parameters:
	thread_count ()


> +	  break;

This will not work universally.  I did not try it but one can do something
like:
	thread apply all call func()

Where func() will pthread_cancel some threads but not all of them.  In such
case thread_count() will not be 0 but still tp->next may be invalid crashing
GDB.

You can remember 'num' and always try to find 'num + 1' thread or any higher
than 'num'.


Thanks,
Jan

Re: Updated patch for Bug 13217 - thread apply all detach throws a SEGFAULT

[Tom Tromey]

>>>>> "Ali" == ali anwar <ali_anwar@codesourcery.com> writes:

Ali> +static int
Ali> +thread_valid (struct thread_info *tp)
Ali> +{
Ali> +  struct thread_info  *utp;
Ali> +  for (utp = thread_list; utp; utp = utp->next)
Ali> +     if (tp == utp)
Ali> +       return 1;
Ali> +  return 0;
Ali> +}

Ali> +  for (tp = thread_list; thread_valid(tp); tp = tp->next)

It seems like this introduces quadratic behavior here.
I think it would be better to avoid this.

Ali> +	if (thread_count() == 0)
Ali> +	  break;

Here too.

Tom

Re: Updated patch for Bug 13217 - thread apply all detach throws a SEGFAULT

[ali_anwar]

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

On 09/27/2012 08:32 PM, Tom Tromey wrote:
>>>>>> "Ali" == ali anwar <ali_anwar@codesourcery.com> writes:
>
> Ali> +static int
> Ali> +thread_valid (struct thread_info *tp)
> Ali> +{
> Ali> +  struct thread_info  *utp;
> Ali> +  for (utp = thread_list; utp; utp = utp->next)
> Ali> +     if (tp == utp)
> Ali> +       return 1;
> Ali> +  return 0;
> Ali> +}
>
> Ali> +  for (tp = thread_list; thread_valid(tp); tp = tp->next)
>
> It seems like this introduces quadratic behavior here.
> I think it would be better to avoid this.
>
> Ali> +	if (thread_count() == 0)
> Ali> +	  break;
>
> Here too.
>
> Tom
>

Please find attached the modified patch. This time the complexity is 
linear. The thread list is saved first and then it is verified that each 
thread still exists or not.

Testing:
* Tested on i686-pc-linux
* Also checked with valgrind
* Executed the testuite

Thanks,
-Ali



[-- Attachment #2: isssue_13217.patch --]
[-- Type: text/x-patch, Size: 3406 bytes --]

Index: gdb/ChangeLog
===================================================================
RCS file: /cvs/src/src/gdb/ChangeLog,v
retrieving revision 1.14894
diff -u -r1.14894 ChangeLog
--- gdb/ChangeLog	9 Dec 2012 18:39:57 -0000	1.14894
+++ gdb/ChangeLog	10 Dec 2012 18:13:27 -0000
@@ -1,3 +1,9 @@
+2012-12-10  Ali Anwar  <ali_anwar@codesourcery.com>
+
+	PR threads/13217
+	* thread.c (thread_apply_all_command): Check for valid threads 
+	and thread count.
+
 2012-12-09  Jan Kratochvil  <jan.kratochvil@redhat.com>
 
 	* configure.ac (CC_HAS_LONG_LONG): Replace by AC_MSG_ERROR.
Index: gdb/thread.c
===================================================================
RCS file: /cvs/src/src/gdb/thread.c,v
retrieving revision 1.149
diff -u -r1.149 thread.c
--- gdb/thread.c	27 Jul 2012 00:52:36 -0000	1.149
+++ gdb/thread.c	10 Dec 2012 18:13:27 -0000
@@ -1174,11 +1174,9 @@
    thread apply 1 2 7 4 backtrace       Apply backtrace cmd to threads 1,2,7,4
    thread apply 2-7 9 p foo(1)  Apply p foo(1) cmd to threads 2->7 & 9
    thread apply all p x/i $pc   Apply x/i $pc cmd to all threads.  */
-
 static void
 thread_apply_all_command (char *cmd, int from_tty)
 {
-  struct thread_info *tp;
   struct cleanup *old_chain;
   char *saved_cmd;
 
@@ -1193,18 +1191,30 @@
      execute_command.  */
   saved_cmd = xstrdup (cmd);
   make_cleanup (xfree, saved_cmd);
-  for (tp = thread_list; tp; tp = tp->next)
-    if (thread_alive (tp))
-      {
-	switch_to_thread (tp->ptid);
-
-	printf_filtered (_("\nThread %d (%s):\n"),
-			 tp->num, target_pid_to_str (inferior_ptid));
-	execute_command (cmd, from_tty);
-	strcpy (cmd, saved_cmd);	/* Restore exact command used
-					   previously.  */
-      }
 
+  if (thread_count ())
+    {
+      struct thread_info tp_array [thread_count ()];
+      struct thread_info *tp;
+      int i, k;
+
+      /* Save a copy of the thread_list in case we execute detach
+         command.  */
+      for (i = 0, tp = thread_list; tp; i++, tp = tp->next)
+        tp_array [i] = *tp;
+
+      for(k = 0; k != i; k++)
+        if (thread_alive (&tp_array [k]))
+          {
+            switch_to_thread ((&tp_array [k])->ptid);
+
+            printf_filtered (_("\nThread %d (%s):\n"),
+                             (&tp_array [k])->num, target_pid_to_str (inferior_ptid));
+            execute_command (cmd, from_tty);
+            strcpy (cmd, saved_cmd);        /* Restore exact command used
+                                               previously.  */
+           }
+    }
   do_cleanups (old_chain);
 }
Index: gdb/testsuite/gdb.threads/threadapply.exp
===================================================================
RCS file: /cvs/src/src/gdb/testsuite/gdb.threads/threadapply.exp,v
retrieving revision 1.15
diff -u -r1.15 threadapply.exp
--- gdb/testsuite/gdb.threads/threadapply.exp	26 Jun 2012 19:23:20 -0000	1.15
+++ gdb/testsuite/gdb.threads/threadapply.exp	10 Dec 2012 18:13:33 -0000
@@ -63,3 +63,4 @@
 gdb_test "up" ".*in main.*" "go up in the stack frame" 
 gdb_test "thread apply all print 1"  "Thread ..*\\\$\[0-9]+ = 1.*Thread ..*\\\$\[0-9]+ = 1.*Thread ..*\\\$\[0-9]+ = 1.*Thread ..*\\\$\[0-9]+ = 1.*Thread ..*\\\$\[0-9]+ = 1.*Thread ..*\\\$\[0-9]+ = 1" "run a simple print command on all threads"
 gdb_test "down" "#0.*thread_function.*" "go down and check selected frame"
+gdb_test "thread apply all detach" "Detaching from.*" "detach all threads"

Re: Updated patch for Bug 13217 - thread apply all detach throws a SEGFAULT

[Tom Tromey]

>>>>> "Ali" == ali anwar <ali_anwar@codesourcery.com> writes:

Ali> +  if (thread_count ())
Ali> +    {
Ali> +      struct thread_info tp_array [thread_count ()];

You can't use variable-length arrays in gdb.
Probably xmalloc and a cleanup is your best option here.

Ali> +      for(k = 0; k != i; k++)

Space between 'for' and '('.

Ali> +            switch_to_thread ((&tp_array [k])->ptid);

No space before '['.  This occurs in a couple places.

Tom

Re: Updated patch for Bug 13217 - thread apply all detach throws a SEGFAULT

[ali_anwar]

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

On 12/11/2012 01:20 AM, Tom Tromey wrote:
>>>>>> "Ali" == ali anwar <ali_anwar@codesourcery.com> writes:
>
> Ali> +  if (thread_count ())
> Ali> +    {
> Ali> +      struct thread_info tp_array [thread_count ()];
>
> You can't use variable-length arrays in gdb.
> Probably xmalloc and a cleanup is your best option here.
>

Removed the array, now using xmalloc and xfree.

> Ali> +      for(k = 0; k != i; k++)
>
> Space between 'for' and '('.
>
Added a space.

> Ali> +            switch_to_thread ((&tp_array [k])->ptid);
>
> No space before '['.  This occurs in a couple places.
>

Removed a space.
> Tom
>

Thanks for the review. PFA the modified patch.

Regards,
-Ali

[-- Attachment #2: issue_13369.patch --]
[-- Type: text/x-patch, Size: 3227 bytes --]

Index: ChangeLog
===================================================================
RCS file: /cvs/src/src/gdb/ChangeLog,v
retrieving revision 1.14894
diff -u -r1.14894 ChangeLog
--- ChangeLog	9 Dec 2012 18:39:57 -0000	1.14894
+++ ChangeLog	11 Dec 2012 15:26:11 -0000
@@ -1,3 +1,9 @@
+2012-12-10  Ali Anwar  <ali_anwar@codesourcery.com>
+
+	PR threads/13217
+	* thread.c (thread_apply_all_command): Check for valid threads 
+	and thread count.
+
 2012-12-09  Jan Kratochvil  <jan.kratochvil@redhat.com>
 
 	* configure.ac (CC_HAS_LONG_LONG): Replace by AC_MSG_ERROR.
Index: thread.c
===================================================================
RCS file: /cvs/src/src/gdb/thread.c,v
retrieving revision 1.149
diff -u -r1.149 thread.c
--- thread.c	27 Jul 2012 00:52:36 -0000	1.149
+++ thread.c	11 Dec 2012 15:31:52 -0000
@@ -1178,7 +1178,6 @@
 static void
 thread_apply_all_command (char *cmd, int from_tty)
 {
-  struct thread_info *tp;
   struct cleanup *old_chain;
   char *saved_cmd;
 
@@ -1193,18 +1192,32 @@
      execute_command.  */
   saved_cmd = xstrdup (cmd);
   make_cleanup (xfree, saved_cmd);
-  for (tp = thread_list; tp; tp = tp->next)
-    if (thread_alive (tp))
-      {
-	switch_to_thread (tp->ptid);
-
-	printf_filtered (_("\nThread %d (%s):\n"),
-			 tp->num, target_pid_to_str (inferior_ptid));
-	execute_command (cmd, from_tty);
-	strcpy (cmd, saved_cmd);	/* Restore exact command used
-					   previously.  */
-      }
 
+  if (thread_count ())
+    {
+      struct thread_info *tp_array;
+      struct thread_info *tp;
+      int i, k;
+
+      /* Save a copy of the thread_list in case we execute detach
+         command.  */
+      tp_array =  xmalloc (sizeof (struct thread_info) * thread_count ());
+      for (i = 0, tp = thread_list; tp; i++, tp = tp->next)
+        tp_array[i] = *tp;
+
+      for (k = 0; k != i; k++)
+        if (thread_alive (&tp_array[k]))
+          {
+            switch_to_thread ((&tp_array[k])->ptid);
+
+            printf_filtered (_("\nThread %d (%s):\n"),
+                             (&tp_array[k])->num, target_pid_to_str (inferior_ptid));
+            execute_command (cmd, from_tty);
+            strcpy (cmd, saved_cmd);        /* Restore exact command used
+                                               previously.  */
+           }
+       xfree (tp_array);
+    }
   do_cleanups (old_chain);
 }
Index: testsuite/gdb.threads/threadapply.exp
===================================================================
RCS file: /cvs/src/src/gdb/testsuite/gdb.threads/threadapply.exp,v
retrieving revision 1.15
diff -u -r1.15 threadapply.exp
--- testsuite/gdb.threads/threadapply.exp	26 Jun 2012 19:23:20 -0000	1.15
+++ testsuite/gdb.threads/threadapply.exp	11 Dec 2012 15:26:20 -0000
@@ -63,3 +63,4 @@
 gdb_test "up" ".*in main.*" "go up in the stack frame" 
 gdb_test "thread apply all print 1"  "Thread ..*\\\$\[0-9]+ = 1.*Thread ..*\\\$\[0-9]+ = 1.*Thread ..*\\\$\[0-9]+ = 1.*Thread ..*\\\$\[0-9]+ = 1.*Thread ..*\\\$\[0-9]+ = 1.*Thread ..*\\\$\[0-9]+ = 1" "run a simple print command on all threads"
 gdb_test "down" "#0.*thread_function.*" "go down and check selected frame"
+gdb_test "thread apply all detach" "Detaching from.*" "detach all threads"

Re: Updated patch for Bug 13217 - thread apply all detach throws a SEGFAULT

[Pedro Alves]

On 12/11/2012 03:36 PM, ali_anwar wrote:
> +  if (thread_count ())
> +    {
> +      struct thread_info *tp_array;
> +      struct thread_info *tp;
> +      int i, k;
> +
> +      /* Save a copy of the thread_list in case we execute detach
> +         command.  */
> +      tp_array =  xmalloc (sizeof (struct thread_info) * thread_count ());

No need to compute the thread count twice, you can cache it.  No need to
copy the whole thread structure.  Make this an array of a thread
pointers, and then,

> +      for (i = 0, tp = thread_list; tp; i++, tp = tp->next)
> +        tp_array[i] = *tp;

      ALL_THREADS (tp)
        {
          tp_array[i] = tp;
          tp->refcount++;
        }

This increments the refcount of each current thread, so that attempts to
delete it just mark it as deleted (so the C object remains valid).

> +
> +      for (k = 0; k != i; k++)
> +        if (thread_alive (&tp_array[k]))

and then write:

      for (k = 0; k != i; k++)
        {
          if (thread_alive (tp_array[k]))
            {
              switch_to_thread (tp_array[k]->ptid);

              printf_filtered (_("\nThread %d (%s):\n"),
                               (tp_array->num, target_pid_to_str (inferior_ptid));
              execute_command (cmd, from_tty);
              strcpy (cmd, saved_cmd);        /* Restore exact command used
                                                 previously.  */
            }
        }

And put this in a cleanup:

      for (k = 0; k != i; k++)
        tp_array[k]->refcount--;

So that if the command throws an error, we still leave with the correct
refcounts.

The advantages are:

 - less memory necessary for the array.
 - handles the corner case of the target reusing a ptid (see
   add_thread_silent).  IOW, this way, even if the command happens to
   make the target reuse a ptid, "thread apply all" won't run the command
   on that new threads my mistake.



> +          {
> +            switch_to_thread ((&tp_array[k])->ptid);
> +
> +            printf_filtered (_("\nThread %d (%s):\n"),
> +                             (&tp_array[k])->num, target_pid_to_str (inferior_ptid));
> +            execute_command (cmd, from_tty);
> +            strcpy (cmd, saved_cmd);        /* Restore exact command used
> +                                               previously.  */
> +           }
> +       xfree (tp_array);

This xfree should be on the cleanup as well.

> +    }
>    do_cleanups (old_chain);

-- 
Pedro Alves

Re: Updated patch for Bug 13217 - thread apply all detach throws a SEGFAULT

[ali_anwar]

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

On 12/11/2012 09:42 PM, Pedro Alves wrote:
> On 12/11/2012 03:36 PM, ali_anwar wrote:
>> +  if (thread_count ())
>> +    {
>> +      struct thread_info *tp_array;
>> +      struct thread_info *tp;
>> +      int i, k;
>> +
>> +      /* Save a copy of the thread_list in case we execute detach
>> +         command.  */
>> +      tp_array =  xmalloc (sizeof (struct thread_info) * thread_count ());
>
> No need to compute the thread count twice, you can cache it.  No need to
> copy the whole thread structure.  Make this an array of a thread
> pointers, and then,
>
>> +      for (i = 0, tp = thread_list; tp; i++, tp = tp->next)
>> +        tp_array[i] = *tp;
>
>        ALL_THREADS (tp)
>          {
>            tp_array[i] = tp;
>            tp->refcount++;
>          }
>
> This increments the refcount of each current thread, so that attempts to
> delete it just mark it as deleted (so the C object remains valid).
>
>> +
>> +      for (k = 0; k != i; k++)
>> +        if (thread_alive (&tp_array[k]))
>
> and then write:
>
>        for (k = 0; k != i; k++)
>          {
>            if (thread_alive (tp_array[k]))
>              {
>                switch_to_thread (tp_array[k]->ptid);
>
>                printf_filtered (_("\nThread %d (%s):\n"),
>                                 (tp_array->num, target_pid_to_str (inferior_ptid));
>                execute_command (cmd, from_tty);
>                strcpy (cmd, saved_cmd);        /* Restore exact command used
>                                                   previously.  */
>              }
>          }
>
> And put this in a cleanup:
>
>        for (k = 0; k != i; k++)
>          tp_array[k]->refcount--;
>
> So that if the command throws an error, we still leave with the correct
> refcounts.
>
> The advantages are:
>
>   - less memory necessary for the array.
>   - handles the corner case of the target reusing a ptid (see
>     add_thread_silent).  IOW, this way, even if the command happens to
>     make the target reuse a ptid, "thread apply all" won't run the command
>     on that new threads my mistake.
>
>

I have tried to implement what you suggested in the attach patch. Does 
it look reasonable?

Thanks,
-Ali

[-- Attachment #2: Thread_apply_all.patch --]
[-- Type: text/x-patch, Size: 3787 bytes --]

Index: gdb/thread.c
===================================================================
RCS file: /cvs/src/src/gdb/thread.c,v
retrieving revision 1.153
diff -u -r1.153 thread.c
--- gdb/thread.c	11 Mar 2013 08:17:08 -0000	1.153
+++ gdb/thread.c	10 Jul 2013 09:51:08 -0000
@@ -65,6 +65,12 @@
 static void restore_current_thread (ptid_t);
 static void prune_threads (void);
 
+struct thread_array_cleanup {
+  struct thread_info **tp_array;
+  int count;
+};
+
+
 struct thread_info*
 inferior_thread (void)
 {
@@ -1125,6 +1131,15 @@
   xfree (old);
 }
 
+static void
+make_cleanup_thread_refcount (void *data)
+{
+  int k;
+  struct thread_array_cleanup *ta_cleanup = data;
+  for (k = 0; k != ta_cleanup->count; k++)
+    ta_cleanup->tp_array[k]->refcount--;
+}
+
 struct cleanup *
 make_cleanup_restore_current_thread (void)
 {
@@ -1176,13 +1191,13 @@
    thread apply 1 2 7 4 backtrace       Apply backtrace cmd to threads 1,2,7,4
    thread apply 2-7 9 p foo(1)  Apply p foo(1) cmd to threads 2->7 & 9
    thread apply all p x/i $pc   Apply x/i $pc cmd to all threads.  */
-
 static void
 thread_apply_all_command (char *cmd, int from_tty)
 {
-  struct thread_info *tp;
   struct cleanup *old_chain;
   char *saved_cmd;
+  int tc;
+  struct thread_array_cleanup ta_cleanup;
 
   if (cmd == NULL || *cmd == '\000')
     error (_("Please specify a command following the thread ID list"));
@@ -1195,17 +1210,41 @@
      execute_command.  */
   saved_cmd = xstrdup (cmd);
   make_cleanup (xfree, saved_cmd);
-  for (tp = thread_list; tp; tp = tp->next)
-    if (thread_alive (tp))
-      {
-	switch_to_thread (tp->ptid);
+  tc = thread_count ();
 
-	printf_filtered (_("\nThread %d (%s):\n"),
-			 tp->num, target_pid_to_str (inferior_ptid));
-	execute_command (cmd, from_tty);
-	strcpy (cmd, saved_cmd);	/* Restore exact command used
-					   previously.  */
-      }
+  if (tc)
+    {
+      struct thread_info **tp_array;
+      struct thread_info *tp;
+      int i, k;
+      i = 0;
+
+      /* Save a copy of the thread_list in case we execute detach
+         command.  */
+      tp_array =  xmalloc (sizeof (struct thread_info*) * tc);
+      ta_cleanup.tp_array = tp_array;
+      ta_cleanup.count = tc;
+
+      ALL_THREADS (tp)
+        {
+          tp_array[i] = tp;
+          tp->refcount++;
+          i++;
+        }
+      for (k = 0; k != i; k++)
+        if (thread_alive (tp_array[k]))
+          {
+            switch_to_thread (tp_array[k]->ptid);
+            printf_filtered (_("\nThread %d (%s):\n"),
+                             tp_array[k]->num, target_pid_to_str (inferior_ptid));
+            execute_command (cmd, from_tty);
+            strcpy (cmd, saved_cmd);        /* Restore exact command used
+                                               previously.  */
+           }
+
+      make_cleanup (xfree, tp_array);
+      make_cleanup (make_cleanup_thread_refcount, &ta_cleanup);
+    }
 
   do_cleanups (old_chain);
 }
Index: gdb/testsuite/gdb.threads/threadapply.exp
===================================================================
RCS file: /cvs/src/src/gdb/testsuite/gdb.threads/threadapply.exp,v
retrieving revision 1.16
diff -u -r1.16 threadapply.exp
--- gdb/testsuite/gdb.threads/threadapply.exp	1 Jan 2013 06:41:27 -0000	1.16
+++ gdb/testsuite/gdb.threads/threadapply.exp	10 Jul 2013 09:51:08 -0000
@@ -63,3 +63,4 @@
 gdb_test "up" ".*in main.*" "go up in the stack frame" 
 gdb_test "thread apply all print 1"  "Thread ..*\\\$\[0-9]+ = 1.*Thread ..*\\\$\[0-9]+ = 1.*Thread ..*\\\$\[0-9]+ = 1.*Thread ..*\\\$\[0-9]+ = 1.*Thread ..*\\\$\[0-9]+ = 1.*Thread ..*\\\$\[0-9]+ = 1" "run a simple print command on all threads"
 gdb_test "down" "#0.*thread_function.*" "go down and check selected frame"
+gdb_test "thread apply all detach" "Thread .*"

Re: Updated patch for Bug 13217 - thread apply all detach throws a SEGFAULT

[Joel Brobecker]

> I have tried to implement what you suggested in the attach patch.
> Does it look reasonable?

Putting my patch-champion hat on (reviewing the style without commenting
on the actual code), I noticed:

Missing ChangeLog entry.

> +struct thread_array_cleanup {
> +  struct thread_info **tp_array;
> +  int count;
> +};

Formatting of the opening curly brace:

    struct thread_array_cleanup
    {
      struct thread_info **tp_array;
      int count;
    };

There should be a comment documenting this struct and its fields.
Typically, we have a global comment before the struct, and then
one command before each field explaining the semantics of that
field. A global comment is enough if it trivially documents the
fields themselves. Here is an example:

    /* Our private data in struct so_list.  */

    struct lm_info
    {
      /* The name of the file mapped by the loader.  Apart from the entry
         for the main executable, this is usually a shared library (which,
         on AIX, is an archive library file, created using the "ar"
         command).  */
      char *filename;

      /* The name of the shared object file with the actual dynamic
         loading dependency.  This may be NULL (Eg. main executable).  */
      char *member_name;

> +static void
> +make_cleanup_thread_refcount (void *data)
> +{
> +  int k;
> +  struct thread_array_cleanup *ta_cleanup = data;
> +  for (k = 0; k != ta_cleanup->count; k++)
> +    ta_cleanup->tp_array[k]->refcount--;
> +}

Every new function should now be documented with a comment above
the function. The GDB project also asks that an empty line between
the documentation/comment and the start of the function definition
be added. Example:

    /* Free the memory allocated for the given lm_info.  */

    static void
    solib_aix_xfree_lm_info (struct lm_info *info)
    {

Another small nit: Empty line after local variable declarations.
Thus:

     int k;
     struct thread_array_cleanup *ta_cleanup = data;

     for (k = 0; k != ta_cleanup->count; k++)

> @@ -1176,13 +1191,13 @@
>     thread apply 1 2 7 4 backtrace       Apply backtrace cmd to threads 1,2,7,4
>     thread apply 2-7 9 p foo(1)  Apply p foo(1) cmd to threads 2->7 & 9
>     thread apply all p x/i $pc   Apply x/i $pc cmd to all threads.  */
> -
>  static void

Please do not delete that line.

>  thread_apply_all_command (char *cmd, int from_tty)
>  {
> -  struct thread_info *tp;
>    struct cleanup *old_chain;
>    char *saved_cmd;
> +  int tc;
> +  struct thread_array_cleanup ta_cleanup;
>  
>    if (cmd == NULL || *cmd == '\000')
>      error (_("Please specify a command following the thread ID list"));
> @@ -1195,17 +1210,41 @@
>       execute_command.  */
>    saved_cmd = xstrdup (cmd);
>    make_cleanup (xfree, saved_cmd);
> -  for (tp = thread_list; tp; tp = tp->next)
> -    if (thread_alive (tp))
> -      {
> -	switch_to_thread (tp->ptid);
> +  tc = thread_count ();
>  
> -	printf_filtered (_("\nThread %d (%s):\n"),
> -			 tp->num, target_pid_to_str (inferior_ptid));
> -	execute_command (cmd, from_tty);
> -	strcpy (cmd, saved_cmd);	/* Restore exact command used
> -					   previously.  */
> -      }
> +  if (tc)
> +    {
> +      struct thread_info **tp_array;
> +      struct thread_info *tp;
> +      int i, k;
> +      i = 0;
> +
> +      /* Save a copy of the thread_list in case we execute detach
> +         command.  */
> +      tp_array =  xmalloc (sizeof (struct thread_info*) * tc);
> +      ta_cleanup.tp_array = tp_array;
> +      ta_cleanup.count = tc;
> +
> +      ALL_THREADS (tp)
> +        {
> +          tp_array[i] = tp;
> +          tp->refcount++;
> +          i++;
> +        }
> +      for (k = 0; k != i; k++)
> +        if (thread_alive (tp_array[k]))
> +          {
> +            switch_to_thread (tp_array[k]->ptid);
> +            printf_filtered (_("\nThread %d (%s):\n"),
> +                             tp_array[k]->num, target_pid_to_str (inferior_ptid));

This line is too long (max 70-74 characters, hard-limit at 80 characters).

> +            execute_command (cmd, from_tty);
> +            strcpy (cmd, saved_cmd);        /* Restore exact command used
> +                                               previously.  */

Please move the comment ahead of the strcpy statement.  Comments
on the side like this are ok when short enough to fit on the same
line.

Thank you,
-- 
Joel

Re: Updated patch for Bug 13217 - thread apply all detach throws a SEGFAULT

[Tom Tromey]

>>>>> "Ali" == ali anwar <ali_anwar@codesourcery.com> writes:

Ali> +      /* Save a copy of the thread_list in case we execute detach
Ali> +         command.  */
Ali> +      tp_array =  xmalloc (sizeof (struct thread_info*) * tc);

Extra space after the "=".  Missing space before the "*" in the type
name.

Ali> +      ta_cleanup.tp_array = tp_array;
Ali> +      ta_cleanup.count = tc;
Ali> +
Ali> +      ALL_THREADS (tp)
Ali> +        {
Ali> +          tp_array[i] = tp;
Ali> +          tp->refcount++;
Ali> +          i++;
Ali> +        }
Ali> +      for (k = 0; k != i; k++)
Ali> +        if (thread_alive (tp_array[k]))
Ali> +          {
Ali> +            switch_to_thread (tp_array[k]->ptid);
Ali> +            printf_filtered (_("\nThread %d (%s):\n"),
Ali> +                             tp_array[k]->num, target_pid_to_str (inferior_ptid));
Ali> +            execute_command (cmd, from_tty);
Ali> +            strcpy (cmd, saved_cmd);        /* Restore exact command used
Ali> +                                               previously.  */
Ali> +           }
Ali> +
Ali> +      make_cleanup (xfree, tp_array);
Ali> +      make_cleanup (make_cleanup_thread_refcount, &ta_cleanup);

This installs the cleanups at the wrong point.
They must be installed before any potential exception is thrown.

That is, the xfree cleanup ought to be created just after initialization;
and the refcount cleanup ought to be created after the refcounts are
incremented.

Also, make_cleanup_thread_refcount is misnamed.
By convention, a "make_cleanup_" function creates a new cleanup.
The function that does the work of cleaning up is given a different name.

Tom

Re: Updated patch for Bug 13217 - thread apply all detach throws a SEGFAULT

[ali_anwar]

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

On 07/10/2013 10:55 PM, Tom Tromey wrote:
>>>>>> "Ali" == ali anwar <ali_anwar@codesourcery.com> writes:
>
>
> This installs the cleanups at the wrong point.
> They must be installed before any potential exception is thrown.
>
> That is, the xfree cleanup ought to be created just after initialization;
> and the refcount cleanup ought to be created after the refcounts are
> incremented.
>
> Also, make_cleanup_thread_refcount is misnamed.
> By convention, a "make_cleanup_" function creates a new cleanup.
> The function that does the work of cleaning up is given a different name.
>

Thanks for the Review.

Please find attached updated patch.

-Ali


[-- Attachment #2: PR13217.patch --]
[-- Type: text/x-patch, Size: 4334 bytes --]

Index: gdb/ChangeLog
===================================================================
RCS file: /cvs/src/src/gdb/ChangeLog,v
retrieving revision 1.15797
diff -u -r1.15797 ChangeLog
--- gdb/ChangeLog	11 Jul 2013 09:07:41 -0000	1.15797
+++ gdb/ChangeLog	11 Jul 2013 12:11:08 -0000
@@ -1,3 +1,11 @@
+2013-07-11  Ali Anwar  <ali_anwar@codesourcery.com>
+
+	PR threads/13217
+	* thread.c (thread_apply_all_command): Check for valid threads
+	and thread count.
+	(thread_array_cleanup): New struct.
+	(set_thread_refcount): New function.
+
 2013-07-11  Andrew Burgess  <aburgess@broadcom.com>
 
 	* infcmd.c (default_print_one_register_info): Reuse function
Index: gdb/thread.c
===================================================================
RCS file: /cvs/src/src/gdb/thread.c,v
retrieving revision 1.154
diff -u -r1.154 thread.c
--- gdb/thread.c	23 May 2013 17:12:51 -0000	1.154
+++ gdb/thread.c	11 Jul 2013 12:18:00 -0000
@@ -65,6 +65,19 @@
 static void restore_current_thread (ptid_t);
 static void prune_threads (void);
 
+/* Data to cleanup thread array.  */
+
+struct thread_array_cleanup
+{
+  /* Array of thread pointers used to set
+     reference count.  */
+  struct thread_info **tp_array;
+
+  /* Thread count in the array.  */
+  int count;
+};
+
+
 struct thread_info*
 inferior_thread (void)
 {
@@ -1132,6 +1145,18 @@
   xfree (old);
 }
 
+/* Set the thread reference count.  */
+
+static void
+set_thread_refcount (void *data)
+{
+  int k;
+  struct thread_array_cleanup *ta_cleanup = data;
+
+  for (k = 0; k != ta_cleanup->count; k++)
+    ta_cleanup->tp_array[k]->refcount--;
+}
+
 struct cleanup *
 make_cleanup_restore_current_thread (void)
 {
@@ -1187,9 +1212,10 @@
 static void
 thread_apply_all_command (char *cmd, int from_tty)
 {
-  struct thread_info *tp;
   struct cleanup *old_chain;
   char *saved_cmd;
+  int tc;
+  struct thread_array_cleanup ta_cleanup;
 
   if (cmd == NULL || *cmd == '\000')
     error (_("Please specify a command following the thread ID list"));
@@ -1202,17 +1228,44 @@
      execute_command.  */
   saved_cmd = xstrdup (cmd);
   make_cleanup (xfree, saved_cmd);
-  for (tp = thread_list; tp; tp = tp->next)
-    if (thread_alive (tp))
-      {
-	switch_to_thread (tp->ptid);
+  tc = thread_count ();
 
-	printf_filtered (_("\nThread %d (%s):\n"),
-			 tp->num, target_pid_to_str (inferior_ptid));
-	execute_command (cmd, from_tty);
-	strcpy (cmd, saved_cmd);	/* Restore exact command used
-					   previously.  */
-      }
+  if (tc)
+    {
+      struct thread_info **tp_array;
+      struct thread_info *tp;
+      int i, k;
+      i = 0;
+
+      /* Save a copy of the thread_list in case we execute detach
+         command.  */
+      tp_array = xmalloc (sizeof (struct thread_info *) * tc);
+      make_cleanup (xfree, tp_array);
+      ta_cleanup.tp_array = tp_array;
+      ta_cleanup.count = tc;
+
+      ALL_THREADS (tp)
+        {
+          tp_array[i] = tp;
+          tp->refcount++;
+          i++;
+        }
+
+      make_cleanup (set_thread_refcount, &ta_cleanup);
+
+      for (k = 0; k != i; k++)
+        if (thread_alive (tp_array[k]))
+          {
+            switch_to_thread (tp_array[k]->ptid);
+            printf_filtered (_("\nThread %d (%s):\n"), 
+			     tp_array[k]->num,
+			     target_pid_to_str (inferior_ptid));
+            execute_command (cmd, from_tty);
+
+            /* Restore exact command used previously.  */
+            strcpy (cmd, saved_cmd);
+	  }
+    }
 
   do_cleanups (old_chain);
 }
Index: gdb/testsuite/gdb.threads/threadapply.exp
===================================================================
RCS file: /cvs/src/src/gdb/testsuite/gdb.threads/threadapply.exp,v
retrieving revision 1.16
diff -u -r1.16 threadapply.exp
--- gdb/testsuite/gdb.threads/threadapply.exp	1 Jan 2013 06:41:27 -0000	1.16
+++ gdb/testsuite/gdb.threads/threadapply.exp	10 Jul 2013 09:51:08 -0000
@@ -63,3 +63,4 @@
 gdb_test "up" ".*in main.*" "go up in the stack frame" 
 gdb_test "thread apply all print 1"  "Thread ..*\\\$\[0-9]+ = 1.*Thread ..*\\\$\[0-9]+ = 1.*Thread ..*\\\$\[0-9]+ = 1.*Thread ..*\\\$\[0-9]+ = 1.*Thread ..*\\\$\[0-9]+ = 1.*Thread ..*\\\$\[0-9]+ = 1" "run a simple print command on all threads"
 gdb_test "down" "#0.*thread_function.*" "go down and check selected frame"
+gdb_test "thread apply all detach" "Thread .*"

Re: Updated patch for Bug 13217 - thread apply all detach throws a SEGFAULT

[Tom Tromey]

>>>>> "Ali" == ali anwar <ali_anwar@codesourcery.com> writes:

Ali> Please find attached updated patch.

Thanks.

Ali> +      struct thread_info **tp_array;
Ali> +      struct thread_info *tp;
Ali> +      int i, k;
Ali> +      i = 0;

Either there should be a blank line between the declarations and the
assignment here, or the declaration should read "int i = 0, k;".

This patch is ok with this fixed.

I found out while reviewing this that thread refcounting is weird.
It is only checked in delete_thread_1, not at all what I was expecting :-)

Tom

Re: Updated patch for Bug 13217 - thread apply all detach throws a SEGFAULT

[ali_anwar]

On 07/14/2013 02:12 AM, Tom Tromey wrote:
>>>>>> "Ali" == ali anwar <ali_anwar@codesourcery.com> writes:
>
> Ali> Please find attached updated patch.
>
> Thanks.
>
> Ali> +      struct thread_info **tp_array;
> Ali> +      struct thread_info *tp;
> Ali> +      int i, k;
> Ali> +      i = 0;
>
> Either there should be a blank line between the declarations and the
> assignment here, or the declaration should read "int i = 0, k;".
>
> This patch is ok with this fixed.
>

Thanks, committed the patch with the suggested change.

-Ali