Re: [RFA] Implement -thread-info.

[Daniel Jacobowitz <drow at false dot org>]

On Sat, Mar 08, 2008 at 06:54:45PM +0300, Vladimir Prus wrote:
> Vladimir Prus wrote:
> 
> > I attach the revised version of the patch, including docs. Eli, is the
> > doc part OK? I intend to commit the code part in a week unless there are
> > objections.
> 
> I have realized that while the patch is mostly about MI, it also
> touches generic code -- thread.c. Is that part of patch (attached again
> for convenience) OK?
> 
> - Volodya
> 
>         * gdb.h (mi_info_threads): Declare.

I think gdbthread.h is the appropriate header for this; gdb.h is for
things using GDB_RC_OK and so forth.

>  @smallexample
> - -thread-info
> + -thread-info [ @var{thread-id} ]
>  @end smallexample
>  
> +Reports the information about either a specific thread, if 

Just "reports information", no "the".

> @@ -1055,6 +1073,7 @@ mi_cmd_list_features (char *command, char **argv, int argc)
>  
>        ui_out_field_string (uiout, NULL, "frozen-varobjs");
>        ui_out_field_string (uiout, NULL, "pending-breakpoints");
> +      ui_out_field_string (uiout, NULL, "thread-info");
>        
>        do_cleanups (cleanup);
>  

Shouldn't this be documented too?

> +void
> +mi_info_threads (struct ui_out *uiout, int requested_thread)

Nick's right about the name of this function.  print_thread_info would
be fine.  info_threads_1 would be fine too - the _1 is a weird
convention that just means "the real body of info_threads", not
"info_threads for one thread".  print_thread_info sounds better to me.

>    /* Backup current thread and selected frame.  */
>    saved_frame_id = get_frame_id (get_selected_frame (NULL));
>    old_chain = make_cleanup_restore_current_thread (inferior_ptid, saved_frame_id);
>  
> +  old_chain = make_cleanup_ui_out_list_begin_end (uiout, "threads");
> +

By overwriting old_chain, you've stopped do_cleanups from restoring
the previously selected thread.

>        extra_info = target_extra_thread_info (tp);
>        if (extra_info)
> -       printf_filtered (" (%s)", extra_info);
> -      puts_filtered ("  ");
> +       ui_out_field_fmt (uiout, "details", " (%s)", extra_info);
> +      ui_out_text (uiout, "  ");

Do you really want to give the front end " (some extra text)"?  It'll
want to display this in a different column anyway.  It should probably
be

  ui_out_text (uiout, " (");
  ui_out_field_string (uiout, extra_info);
  ui_out_text (")  ");

>        /* That switch put us at the top of the stack (leaf frame).  */
>        switch_to_thread (tp->ptid);
>        print_stack_frame (get_selected_frame (NULL), 0, LOCATION);

I think Nick was right about printing the frame level.  It's not
necessarily going to be zero in the future.  For instance, GDB might
automatically select the nearest user frame if you stop when a C++
exception is thrown, like we already do for Ada; and then we could
teach GDB not to lose the selected frame whenever it switches threads.
That wants doing anyway.

-- 
Daniel Jacobowitz
CodeSourcery