Re: [RFC]: Improvement of TUI status line to display pid and thread instead of target name

[Pedro Alves <palves at redhat dot com>]

On 11/10/2012 05:53 PM, Stephane Carrez wrote:
> Hi!

Hello!

> While debugging a multi-thread application, the TUI status line looks
> as follows:
> 
> multi-thre Thread 0xb7175 In:
> system__tasking__rendezvous__timed_task_entry_call
> Line: ??   PC: 0xb7d49787
> 
> The target name and thread information are not useful at all.

Yeah.  It's worse on GNU/Linux since the space allocated for the
target_pid_to_str string is too short, and so the string is truncated, which
makes all threads appear as "Thread 0x7ffff":

multi-thre Thread 0x7ffff In: main

(gdb) info threads
* 2    Thread 0x7ffff7fd0700 (LWP 7616) "threads" 0x0000003de92ba6cd in nanosleep () from /lib64/libc.so.6
  1    Thread 0x7ffff7fd1740 (LWP 7605) "threads" main () at threads.c:43

> I've worked on a better status line which displays the process id, the
> current thread id
> and the number of threads.  

Thanks.

> The new status line looks as follows:
> Pid 25932 Tid 1 on 7 In:
> system__tasking__rendezvous__timed_task_entry_call
>   Line: ??   PC: 0xb7d49787
> 
> I've also used the thread observer to be notified when a thread is
> created or exited
> so the status line is refreshed while threads are created/exited.

(I'd leave the issue of _when_ to update the status line
to a separate patch/discussion, as it's logically independent
change from _what_ is displayed in the status line.)

> 
> Attached is the patch for the above changes.
> 
> Before committing it, I would like your feedback.

See below.

> +
>  /* The selected frame has changed.  This is happens after a target
>     stop or when the user explicitly changes the frame
>     (up/down/thread/...).  */
> @@ -253,6 +269,8 @@ static struct observer *tui_bp_deleted_o
>  static struct observer *tui_bp_modified_observer;
>  static struct observer *tui_inferior_exit_observer;
>  static struct observer *tui_about_to_proceed_observer;
> +static struct observer *tui_thread_exit_observer;
> +static struct observer *tui_new_thread_observer;
>  void _initialize_tui_hooks (void);
> Index: tui/tui-stack.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/tui/tui-stack.c,v
> retrieving revision 1.46
> diff -u -p -r1.46 tui-stack.c
> --- tui/tui-stack.c	25 Apr 2012 08:16:43 -0000	1.46
> +++ tui/tui-stack.c	10 Nov 2012 17:41:48 -0000
> @@ -29,6 +29,7 @@
>  #include "top.h"
>  #include "gdb-demangle.h"
>  #include "gdb_string.h"
> +#include "gdbthread.h"
>  #include "tui/tui.h"
>  #include "tui/tui-data.h"
>  #include "tui/tui-stack.h"
> @@ -57,8 +58,8 @@ static void tui_update_command (char *, 
>  
>  
>  /* Create the status line to display as much information as we can on
> -   this single line: target name, process number, current function,
> -   current line, current PC, SingleKey mode.  */
> +   this single line: target name, process number, thread id, thread count,
> +   current function, current line, current PC, SingleKey mode.  */
>  static char*
>  tui_make_status_line (struct tui_locator_element *loc)
>  {
> @@ -74,16 +75,31 @@ tui_make_status_line (struct tui_locator
>    int line_width;
>    int pc_width;
>    struct ui_file *pc_out;
> +  char proc_info[40];
> +
> +  target_width = strlen (target_shortname);
> +  if (target_width > MAX_TARGET_WIDTH)
> +    target_width = MAX_TARGET_WIDTH;

I'm confused, we still account for "target_shortname", but
the target name no longer appears in the status line?

>  
>    if (ptid_equal (inferior_ptid, null_ptid))
>      pid_name = "No process";
>    else
> -    pid_name = target_pid_to_str (inferior_ptid);
> +    {
> +      int nr_threads = thread_count ();
>  
> -  target_width = strlen (target_shortname);
> -  if (target_width > MAX_TARGET_WIDTH)
> -    target_width = MAX_TARGET_WIDTH;
> +      if (nr_threads > 1)
> +        xsnprintf (proc_info, sizeof (proc_info), "Pid %d Tid %d on %d",
> +                   ptid_get_pid (inferior_ptid),
> +                   pid_to_thread_id (inferior_ptid),
> +                   nr_threads);
> +      else
> +        xsnprintf (proc_info, sizeof (proc_info), "Pid %d",
> +                   ptid_get_pid (inferior_ptid));
>  
> +      target_width = 0;
> +      pid_name = proc_info;
> +    }
> +  

This prints the raw pid (ptid_get_pid).  That value is never supposed to
be user visible directly -- it may have been invented by gdb, e.g., on targets
that have no concept of process at all (or even core files on some targets).
There's also a concept discrepancy in that it prints the PID, which is a target-side
number, and the gdb thread id, which is a gdb-invented number, not the target's
idea of a thread id, which was what was given by target_pid_to_str.
I think that printing the (gdb invented) inferior id in addition or instead
of the process id to be more in line with printing the gdb thread id.  After all,
all commands that work with inferiors, accept or display an inferior id, not
a target-side process id.  In any case, to print the PID we should go
through target_pid_to_str too -- see inferior.c:print_inferior
(and "info inferiors"), or the quit prompt when you're debugging something live.

Not sure about printing the thread count.  Sounds like something that might
confuse users -- on some targets (including all remote targets), the thread
list is only updated when the user does "info threads".  (Due to updating on
every thread add/remove, this also adds a linear walk over the thread list
for each thread added, though that'd of course be fixable in the thread list
data structure itself.)

-- 
Pedro Alves