Re: [PATCH] Variable objects in multi-threaded programs

[Vladimir Prus <vladimir at codesourcery dot com>]

On Friday 25 January 2008 01:18:46 Nick Roberts wrote:
> > ?> I'm somewhat concerned that this patch makes gdb traverse stacks of all
> ?> ?> threads -- it can take quite some time. Would a better solution be
> ?> ?> to store thread id inside varobj?
> ?> 
> ?> Yes I think it would. ?As an element of struct varobj_root?
> ?> 
> ?> Perhaps this should also included as a field in the output of -var-create,
> ?> the frontend could organise the display of watch expressions by thread. ?If
> ?> we use the GDB thread id, this would be consistent with infrun.c
> 
> Here's a revised patch along those lines. ?The diffs for gdbthread.h, thread.c
> are as before, that of varobj.c is new and there are now ones for mi-cmd-var.c
> and varobj.h.
> 
> -- 
> Nick ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? http://www.inet.net.nz/~nickrob
> 
> 2008-01-25 ?Nick Roberts ?<nickrob@snap.net.nz>
> 
> ????????* thread.c (make_cleanup_restore_current_thread)
> ????????(do_restore_current_thread_cleanup): Don't make static
> ????????(struct current_thread_cleanup): Move to gdbthread.h
> 
> ????????* gdbthread.h: Declare above functions as externs here.
> 
> ????????* varobj.c ?(struct varobj_root): New component thread_id.
> ????????(varobj_create): Set it to -2 for global variables.
> ????????(varobj_get_thread_id, check_scope): New functions.
> ????????(c_value_of_root): Use it to iterate over threads.
> 
> ????????* varobj.h (varobj_get_thread_id): New extern.
> 
> ????????* mi/mi-cmd-var.c (print_varobj): Add thread-id field.

> --- varobj.c.~1.99.~    2008-01-04 10:24:29.000000000 +1300
> +++ varobj.c    2008-01-25 10:50:06.000000000 +1300
> @@ -24,7 +24,9 @@
>  #include "language.h"
>  #include "wrapper.h"
>  #include "gdbcmd.h"
> +#include "gdbthread.h"
>  #include "block.h"
> +#include "inferior.h"
>
>  #include "gdb_assert.h"
>  #include "gdb_string.h"
> @@ -65,6 +67,9 @@
>    /* The frame for this expression */
>    struct frame_id frame;
>
> +  /* GDB thread id.  */
> +  int thread_id;

I don't think this comment explains when this
field is valid, and what does it mean when valid.
The magic -2 value is not documented, too.

>    /* If 1, "update" always recomputes the frame & valid block
>       using the currently selected frame. */
>    int use_selected_frame;
> @@ -492,6 +497,11 @@
>
>        var->format = variable_default_display (var);
>        var->root->valid_block = innermost_block;
> +      if (innermost_block)
> +      var->root->thread_id = pid_to_thread_id (inferior_ptid);
> +      else
> +      /* Give global variables a thread_id of -2.  */
> +      var->root->thread_id = -2;

Something is wrong with indentation here.

>        expr_len = strlen (expression);
>        var->name = savestring (expression, expr_len);
>        /* For a root var, the name and the expr are the same.  */
> @@ -686,6 +696,12 @@
>    return var->format;
>  }
>
> +int
> +varobj_get_thread_id (struct varobj *var)
> +{
> +  return var->root->thread_id;
> +}
> +
>  void
>  varobj_set_frozen (struct varobj *var, int frozen)
>  {
> @@ -2153,37 +2169,61 @@
>    return child->path_expr;
>  }
>
> +static void
> +check_scope (struct varobj *var, int* scope)
> +{

This function needs a comment. 'check_scope', in
itself, can do just about anything. Why does
it use out-parameter, as opposed to just returning
a value?

> +  struct frame_info *fi;
> +
> +  fi = frame_find_by_id (var->root->frame);
> +  *scope = fi != NULL;
> +
> +  /* FIXME: select_frame could fail */
> +  if (fi)
> +    {
> +      CORE_ADDR pc = get_frame_pc (fi);
> +      if (pc <  BLOCK_START (var->root->valid_block) ||
> +        pc >= BLOCK_END (var->root->valid_block))
> +      *scope = 0;
> +      else
> +      select_frame (fi);
> +    }
> +}
> +
>  static struct value *
>  c_value_of_root (struct varobj **var_handle)
>  {
>    struct value *new_val = NULL;
>    struct varobj *var = *var_handle;
> -  struct frame_info *fi;
> -  int within_scope;
> +  struct frame_id saved_frame_id;
> +  struct cleanup *old_cleanups = NULL;
> +  int within_scope, thread_id;
> +  ptid_t ptid;
>
>    /*  Only root variables can be updated... */
>    if (!is_root_p (var))
>      /* Not a root var */
>      return NULL;
>
> -
>    /* Determine whether the variable is still around. */
>    if (var->root->valid_block == NULL || var->root->use_selected_frame)
>      within_scope = 1;
>    else
>      {
> -      fi = frame_find_by_id (var->root->frame);
> -      within_scope = fi != NULL;
> -      /* FIXME: select_frame could fail */
> -      if (fi)
> +      thread_id = var->root->thread_id;
> +      ptid = thread_id_to_pid (thread_id);
> +      if (thread_id == 0)
> +      check_scope (var, &within_scope);

Something appears wrong with indentation here.

> +      else if (thread_id != -1 && target_thread_alive (ptid))
>        {

I'm confused about meaning of thread_id==0, thread_id==-1 
and thread_id==-2. Can you clarify that and add that to 
varobj_root->thread_id comment? We probably need some comments
for the branches here.

> -        CORE_ADDR pc = get_frame_pc (fi);
> -        if (pc <  BLOCK_START (var->root->valid_block) ||
> -            pc >= BLOCK_END (var->root->valid_block))
> -          within_scope = 0;
> -        else
> -          select_frame (fi);
> -      }
> +
> +        saved_frame_id = get_frame_id (get_selected_frame (NULL));
> +        old_cleanups = make_cleanup_restore_current_thread (inferior_ptid,
>				saved_frame_id); 

Presumably, we can move this to the top-level of c_value_of_root, and then
get rid of save/restore of selected frame in varobj_update?

> +        switch_to_thread (ptid); 
> +        check_scope (var, &within_scope);
> +      }
> +      else
> +      var->root->thread_id = -1;
>      }

I think that it would be important to have some automated tests
to come with this patch.

>    if (within_scope)
> @@ -2194,6 +2234,8 @@
>        return new_val;
>      }
>
> +  do_cleanups (old_cleanups);
> +
>    return NULL;
>  }
>
> --- thread.c.~1.59.~    2008-01-24 09:47:31.000000000 +1300
> +++ thread.c    2008-01-24 15:38:41.000000000 +1300
> @@ -60,8 +60,6 @@
>  static void thread_apply_command (char *, int);
>  static void restore_current_thread (ptid_t);
>  static void prune_threads (void);
> -static struct cleanup *make_cleanup_restore_current_thread (ptid_t,
> -                                                            struct
> frame_id);
>
>  void
>  delete_step_resume_breakpoint (void *arg)
> @@ -496,13 +494,7 @@
>      }
>  }
>
> -struct current_thread_cleanup
> -{
> -  ptid_t inferior_ptid;
> -  struct frame_id selected_frame_id;
> -};
> -
> -static void
> +void
>  do_restore_current_thread_cleanup (void *arg)
>  {
>    struct current_thread_cleanup *old = arg;
> @@ -511,7 +503,7 @@
>    xfree (old);
>  }
>
> -static struct cleanup *
> +struct cleanup *
>  make_cleanup_restore_current_thread (ptid_t inferior_ptid,
>                                       struct frame_id a_frame_id)
>  {
>
>
> --- gdbthread.h.~1.19.~ 2008-01-24 09:47:30.000000000 +1300
> +++ gdbthread.h 2008-01-24 15:37:19.000000000 +1300
> @@ -143,6 +143,17 @@
>  /* Switch from one thread to another.  */
>  extern void switch_to_thread (ptid_t ptid);
>
> +struct current_thread_cleanup
> +{
> +  ptid_t inferior_ptid;
> +  struct frame_id selected_frame_id;
> +};
> +
> +extern void do_restore_current_thread_cleanup (void *arg);
> +
> +extern struct cleanup * make_cleanup_restore_current_thread
> +                          (ptid_t inferior_ptid, struct frame_id
> a_frame_id); +
>  /* Commands with a prefix of `thread'.  */
>  extern struct cmd_list_element *thread_cmd_list;
>
>
> --- varobj.h.~1.14.~    2008-01-04 10:24:30.000000000 +1300
> +++ varobj.h    2008-01-24 22:09:17.000000000 +1300
> @@ -85,6 +85,8 @@
>  extern enum varobj_display_formats varobj_get_display_format (
>                                                        struct varobj *var);
>
> +extern int varobj_get_thread_id (struct varobj *var);
> +
>  extern void varobj_set_frozen (struct varobj *var, int frozen);
>
>  extern int varobj_get_frozen (struct varobj *var);
>
>
> --- mi-cmd-var.c.~1.44.~        2008-01-23 16:19:39.000000000 +1300
> +++ mi-cmd-var.c        2008-01-25 00:43:22.000000000 +1300
> @@ -50,6 +50,7 @@
>  {
>    struct type *gdb_type;
>    char *type;
> +  int thread_id;
>
>    ui_out_field_string (uiout, "name", varobj_get_objname (var));
>    if (print_expression)
> @@ -66,6 +67,10 @@
>        xfree (type);
>      }
>
> +  thread_id = varobj_get_thread_id (var);
> +  if (thread_id != -2)
> +    ui_out_field_int (uiout, "thread-id", thread_id);
> +
>    if (varobj_get_frozen (var))
>      ui_out_field_int (uiout, "frozen", 1);
>  }

- Volodya