Re: [patch] info threads sort by name and name regex matching

[Aaron Gamble <agamble@google.com>]

On Fri, Aug 24, 2012 at 10:57 AM, Tom Tromey <tromey@redhat.com> wrote:
>
>>>>>> "Aaron" == Aaron Gamble <agamble@google.com> writes:
>
> Aaron> -a enables sorting by thread name
>
> Is '-a' mnemonic for something?
-a is for alphabetical. Would -s be more appropriate?


> Aaron> +static int
> Aaron> +print_thread_sort_cmp (const void *p1, const void *p2)
> Aaron> +{
> Aaron> +  const char *name1 = (*(struct thread_info **) p1)->cached_name;
> Aaron> +  const char *name2 = (*(struct thread_info **) p2)->cached_name;
> Aaron> +  if (name1 && name2)
> Aaron> +    return strcmp (name1, name2);
>
> The gdb style is to have a blank line between declarations and code.
Ack.


> Aaron> +static void
> Aaron> +thread_cache_name (struct thread_info *tp)
> Aaron> +{
> Aaron> +  /* Does not need to be freed, is only transient.  */
> Aaron> +  tp->cached_name = tp->name ? tp->name : target_thread_name (tp);
>
> I realized while re-reading the patch that this isn't safe.
> target_thread_name has a funny contract, as you note, where it returns
> static data.  But this means that the result can't be cached --
> otherwise all threads with non-NULL names will end up with the same
> cached name.
>
> Either you have to copy the name here or change target_thread_name to do so;
> and then make sure to free at the right spots, etc.
Would it be safe to use target_thread_name in print_thread_sort_cmp
each time the name is used in the sort function? I'm not entirely sure
why it's unsafe to store the NULL pointers when the name could not be
determined.