[patchv2] Sort threads for thread apply all (bt)
Commit Message
On Thu, 15 Jan 2015 20:29:07 +0100, Doug Evans wrote:
> On Thu, Jan 15, 2015 at 10:33 AM, Jan Kratochvil <jan.kratochvil@redhat.com> wrote:
> > I find maybe as good enough and with no risk of UI change flamewar to just
> > sort the threads by their number. Currently they are printed as they happen
> > in the internal GDB list which has no advantage. Printing thread #1 as the
> > first one with assumed 'thread apply all bt' (after the core file is loaded)
> > should make the complaint resolved I guess.
> >
> > No regressions on {x86_64,x86_64-m32,i686}-fedora22pre-linux-gnu.
>
> No objection to sorting the list, but if thread #1 is the important one,
> then a concern could be it'll have scrolled off the screen (such a
> concern has been voiced in another thread in another context),
> and if not lost (say it's in an emacs buffer) one would still have
> to scroll back to see it.
> So one *could* still want #1 to be last.
> Do we want an option to choose the sort direction?
> [I wouldn't make it a global parameter, just an option to
> thread apply.]
Done.
Thanks,
Jan
gdb/ChangeLog
2015-01-16 Jan Kratochvil <jan.kratochvil@redhat.com>
* thread.c (tp_array_compar_asc, tp_array_compar): New.
(thread_apply_all_command): Parse CMD for tp_array_compar_asc. Sort
tp_array using tp_array_compar.
(_initialize_thread): Extend thread_apply_all_command help.
gdb/doc/ChangeLog
2015-01-16 Jan Kratochvil <jan.kratochvil@redhat.com>
* gdb.texinfo (Threads): Describe -asc for thread apply all.
Comments
Jan Kratochvil writes:
> On Thu, 15 Jan 2015 20:29:07 +0100, Doug Evans wrote:
> > On Thu, Jan 15, 2015 at 10:33 AM, Jan Kratochvil <jan.kratochvil@redhat.com> wrote:
> > > I find maybe as good enough and with no risk of UI change flamewar to just
> > > sort the threads by their number. Currently they are printed as they happen
> > > in the internal GDB list which has no advantage. Printing thread #1 as the
> > > first one with assumed 'thread apply all bt' (after the core file is loaded)
> > > should make the complaint resolved I guess.
> > >
> > > No regressions on {x86_64,x86_64-m32,i686}-fedora22pre-linux-gnu.
> >
> > No objection to sorting the list, but if thread #1 is the important one,
> > then a concern could be it'll have scrolled off the screen (such a
> > concern has been voiced in another thread in another context),
> > and if not lost (say it's in an emacs buffer) one would still have
> > to scroll back to see it.
> > So one *could* still want #1 to be last.
> > Do we want an option to choose the sort direction?
> > [I wouldn't make it a global parameter, just an option to
> > thread apply.]
>
> Done.
>
>
> Thanks,
> Jan
> gdb/ChangeLog
> 2015-01-16 Jan Kratochvil <jan.kratochvil@redhat.com>
>
> * thread.c (tp_array_compar_asc, tp_array_compar): New.
> (thread_apply_all_command): Parse CMD for tp_array_compar_asc. Sort
> tp_array using tp_array_compar.
> (_initialize_thread): Extend thread_apply_all_command help.
>
> gdb/doc/ChangeLog
> 2015-01-16 Jan Kratochvil <jan.kratochvil@redhat.com>
>
> * gdb.texinfo (Threads): Describe -asc for thread apply all.
Hi.
Just some nits, no need to resubmit for review.
>
> diff --git a/gdb/thread.c b/gdb/thread.c
> index ed20fbe..9685351 100644
> --- a/gdb/thread.c
> +++ b/gdb/thread.c
> @@ -1382,6 +1382,20 @@ make_cleanup_restore_current_thread (void)
> restore_current_thread_cleanup_dtor);
> }
>
> +static int tp_array_compar_asc;
This should probably have a comment.
> +
> +/* Sort an array for struct thread_info pointers by their ascending NUM. */
> +
> +static int
> +tp_array_compar (const void *ap_voidp, const void *bp_voidp)
> +{
> + const struct thread_info *const *ap = ap_voidp;
> + const struct thread_info *const *bp = bp_voidp;
> +
> + return ((((*ap)->num > (*bp)->num) - ((*ap)->num < (*bp)->num))
> + * (tp_array_compar_asc ? +1 : -1));
> +}
This triggers my "passing parameters as global variables" alarm,
and while one could instead have two different functions,
this is ok, at least for now.
> +
> /* Apply a GDB command to a list of threads. List syntax is a whitespace
> seperated list of numbers, or ranges, or the keyword `all'. Ranges consist
> of two numbers seperated by a hyphen. Examples:
> @@ -1398,6 +1412,13 @@ thread_apply_all_command (char *cmd, int from_tty)
> int tc;
> struct thread_array_cleanup ta_cleanup;
>
> + tp_array_compar_asc = 0;
> + if (cmd && (check_for_argument (&cmd, "-asc", strlen ("-asc"))))
cmd != NULL
> + {
> + cmd = skip_spaces (cmd);
> + tp_array_compar_asc = 1;
> + }
> +
> if (cmd == NULL || *cmd == '\000')
> error (_("Please specify a command following the thread ID list"));
>
> @@ -1431,6 +1452,8 @@ thread_apply_all_command (char *cmd, int from_tty)
> i++;
> }
>
> + qsort (tp_array, i, sizeof (*tp_array), tp_array_compar);
> +
> make_cleanup (set_thread_refcount, &ta_cleanup);
>
> for (k = 0; k != i; k++)
> @@ -1739,7 +1762,14 @@ The new thread ID must be currently known."),
> &thread_apply_list, "thread apply ", 1, &thread_cmd_list);
>
> add_cmd ("all", class_run, thread_apply_all_command,
> - _("Apply a command to all threads."), &thread_apply_list);
> + _("\
> +Apply a command to all threads.\n\
> +\n\
> +Usage: thread apply all [-asc] <command>\n\
> +-asc: Call <command> for all threads in ascending order.\n\
> + The default is descending order.\n\
No final trailing newline.
> +"),
> + &thread_apply_list);
>
> add_cmd ("name", class_run, thread_name_command,
> _("Set the current thread's name.\n\
> diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
> index f413e23..2207ce4 100644
> --- a/gdb/doc/gdb.texinfo
> +++ b/gdb/doc/gdb.texinfo
> @@ -2959,14 +2959,17 @@ information on convenience variables.
>
> @kindex thread apply
> @cindex apply command to several threads
> -@item thread apply [@var{threadno} | all] @var{command}
> +@item thread apply [@var{threadno} | all [-asc]] @var{command}
> The @code{thread apply} command allows you to apply the named
> @var{command} to one or more threads. Specify the numbers of the
> threads that you want affected with the command argument
> @var{threadno}. It can be a single thread number, one of the numbers
> shown in the first field of the @samp{info threads} display; or it
> -could be a range of thread numbers, as in @code{2-4}. To apply a
> -command to all threads, type @kbd{thread apply all @var{command}}.
> +could be a range of thread numbers, as in @code{2-4}. To apply
> +a command to all threads in descending order, type @kbd{thread apply all
> +@var{command}}. To apply a command to all threads in ascending order,
> +type @kbd{thread apply all -asc @var{command}}.
> +
>
> @kindex thread name
> @cindex name a thread
With http://sourceware.org/bugzilla/show_bug.cgi?id=16445 in
mind, should we add a counterpart option to explicitly force
descending order at the same time, so we're a bit more free
to change the default order at some point?
Thanks,
Pedro Alves
On Thu, 22 Jan 2015 12:18:27 +0100, Pedro Alves wrote:
> With http://sourceware.org/bugzilla/show_bug.cgi?id=16445 in
> mind, should we add a counterpart option to explicitly force
> descending order at the same time, so we're a bit more free
> to change the default order at some point?
Isn't the PR # a typo? I do not see how 16445 could be related.
Jan
On 01/22/2015 04:43 PM, Jan Kratochvil wrote:
> On Thu, 22 Jan 2015 12:18:27 +0100, Pedro Alves wrote:
>> With http://sourceware.org/bugzilla/show_bug.cgi?id=16445 in
>> mind, should we add a counterpart option to explicitly force
>> descending order at the same time, so we're a bit more free
>> to change the default order at some point?
>
> Isn't the PR # a typo? I do not see how 16445 could be related.
It is, sorry. Here's the right one:
https://sourceware.org/bugzilla/show_bug.cgi?id=17539
My Firefox has this annoying bug where sometimes the url
line edit contents get stale... I follow links, paste a new
url, or open new tabs, and the visible url doesn't
change... sigh.
Thanks,
Pedro Alves
On Thu, 22 Jan 2015 18:07:33 +0100, Pedro Alves wrote:
> https://sourceware.org/bugzilla/show_bug.cgi?id=17539
While we're getting to the classical bikeshed before coding in the '-desc'
option I would like to reassure we should not go the global setting way
instead which I would find more appropriate myself in such situation, contrary
to what Doug said:
On Thu, 15 Jan 2015 20:29:07 +0100, Doug Evans wrote:
# Do we want an option to choose the sort direction?
# [I wouldn't make it a global parameter, just an option to
# thread apply.]
> My Firefox has this annoying bug where sometimes the url
> line edit contents get stale... I follow links, paste a new
> url, or open new tabs, and the visible url doesn't
> change... sigh.
BTW I always type 'escape ctrl-l ctrl-c' instead of just 'ctrl-l ctrl-c'.
Jan
On Thu, Jan 22, 2015 at 9:17 AM, Jan Kratochvil
<jan.kratochvil@redhat.com> wrote:
> On Thu, 22 Jan 2015 18:07:33 +0100, Pedro Alves wrote:
>> https://sourceware.org/bugzilla/show_bug.cgi?id=17539
>
> While we're getting to the classical bikeshed before coding in the '-desc'
> option I would like to reassure we should not go the global setting way
> instead which I would find more appropriate myself in such situation, contrary
> to what Doug said:
>
> On Thu, 15 Jan 2015 20:29:07 +0100, Doug Evans wrote:
> # Do we want an option to choose the sort direction?
> # [I wouldn't make it a global parameter, just an option to
> # thread apply.]
I'd rather not add to the global state until there's a justified reason for it.
We're not there yet IMO.
And we can add -desc later.
[Though -asc vs -desc is a bit awkward.
-a and -d?
-ascending and -descending ?]
So I'm happy with the patch as is.
[Which isn't to say I wouldn't be happy with
something different though.]
On Thu, 22 Jan 2015 19:07:37 +0100, Doug Evans wrote:
> I'd rather not add to the global state until there's a justified reason for it.
> We're not there yet IMO.
OK.
> And we can add -desc later.
I also think so.
> [Though -asc vs -desc is a bit awkward.
> -a and -d?
> -ascending and -descending ?]
I can put there "-ascending".
> So I'm happy with the patch as is.
Jan
@@ -1382,6 +1382,20 @@ make_cleanup_restore_current_thread (void)
restore_current_thread_cleanup_dtor);
}
+static int tp_array_compar_asc;
+
+/* Sort an array for struct thread_info pointers by their ascending NUM. */
+
+static int
+tp_array_compar (const void *ap_voidp, const void *bp_voidp)
+{
+ const struct thread_info *const *ap = ap_voidp;
+ const struct thread_info *const *bp = bp_voidp;
+
+ return ((((*ap)->num > (*bp)->num) - ((*ap)->num < (*bp)->num))
+ * (tp_array_compar_asc ? +1 : -1));
+}
+
/* Apply a GDB command to a list of threads. List syntax is a whitespace
seperated list of numbers, or ranges, or the keyword `all'. Ranges consist
of two numbers seperated by a hyphen. Examples:
@@ -1398,6 +1412,13 @@ thread_apply_all_command (char *cmd, int from_tty)
int tc;
struct thread_array_cleanup ta_cleanup;
+ tp_array_compar_asc = 0;
+ if (cmd && (check_for_argument (&cmd, "-asc", strlen ("-asc"))))
+ {
+ cmd = skip_spaces (cmd);
+ tp_array_compar_asc = 1;
+ }
+
if (cmd == NULL || *cmd == '\000')
error (_("Please specify a command following the thread ID list"));
@@ -1431,6 +1452,8 @@ thread_apply_all_command (char *cmd, int from_tty)
i++;
}
+ qsort (tp_array, i, sizeof (*tp_array), tp_array_compar);
+
make_cleanup (set_thread_refcount, &ta_cleanup);
for (k = 0; k != i; k++)
@@ -1739,7 +1762,14 @@ The new thread ID must be currently known."),
&thread_apply_list, "thread apply ", 1, &thread_cmd_list);
add_cmd ("all", class_run, thread_apply_all_command,
- _("Apply a command to all threads."), &thread_apply_list);
+ _("\
+Apply a command to all threads.\n\
+\n\
+Usage: thread apply all [-asc] <command>\n\
+-asc: Call <command> for all threads in ascending order.\n\
+ The default is descending order.\n\
+"),
+ &thread_apply_list);
add_cmd ("name", class_run, thread_name_command,
_("Set the current thread's name.\n\
@@ -2959,14 +2959,17 @@ information on convenience variables.
@kindex thread apply
@cindex apply command to several threads
-@item thread apply [@var{threadno} | all] @var{command}
+@item thread apply [@var{threadno} | all [-asc]] @var{command}
The @code{thread apply} command allows you to apply the named
@var{command} to one or more threads. Specify the numbers of the
threads that you want affected with the command argument
@var{threadno}. It can be a single thread number, one of the numbers
shown in the first field of the @samp{info threads} display; or it
-could be a range of thread numbers, as in @code{2-4}. To apply a
-command to all threads, type @kbd{thread apply all @var{command}}.
+could be a range of thread numbers, as in @code{2-4}. To apply
+a command to all threads in descending order, type @kbd{thread apply all
+@var{command}}. To apply a command to all threads in ascending order,
+type @kbd{thread apply all -asc @var{command}}.
+
@kindex thread name
@cindex name a thread