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

[Joel Brobecker <brobecker at adacore dot com>]

> 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