This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: Updated patch for Bug 13217 - thread apply all detach throws a SEGFAULT
- From: Joel Brobecker <brobecker at adacore dot com>
- To: ali_anwar <ali_anwar at codesourcery dot com>
- Cc: Pedro Alves <palves at redhat dot com>, Tom Tromey <tromey at redhat dot com>, gdb-patches at sourceware dot org
- Date: Wed, 10 Jul 2013 05:57:38 -0700
- Subject: Re: Updated patch for Bug 13217 - thread apply all detach throws a SEGFAULT
- References: <5062EA9B dot 1060505 at codesourcery dot com> <871uhnwkf3 dot fsf at fleche dot redhat dot com> <50C62BBB dot 6010404 at codesourcery dot com> <87hantek1p dot fsf at fleche dot redhat dot com> <50C75319 dot 9080903 at codesourcery dot com> <50C7628F dot 5080004 at redhat dot com> <51DD37FA dot 8010306 at codesourcery 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