[PATCH] Variable objects in multi-threaded programs

[PATCH] Variable objects in multi-threaded programs

[Nick Roberts]

Currently GDB thinks variable objects have gone out of scope if the thread
changes.  This patch corrects that.

---
Nick                                           http://www.inet.net.nz/~nickrob


2008-01-24  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 (look_for_frame): New function.
	(c_value_of_root): Use it to iterate over threads.



*** varobj.c.~1.99.~	2008-01-04 10:24:29.000000000 +1300
--- varobj.c	2008-01-24 16:42:41.000000000 +1300
***************
*** 24,30 ****
--- 24,32 ----
  #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"
*************** c_path_expr_of_child (struct varobj *chi
*** 2153,2164 ****
--- 2155,2181 ----
    return child->path_expr;
  }
  
+ static int
+ look_for_frame (struct thread_info *thr, void* arg)
+ {
+   struct frame_id *id = arg;
+   struct frame_info *fi;
+   switch_to_thread (thr->ptid);
+   fi = frame_find_by_id (*id);
+   if (fi)
+     return 1;
+   else
+     return 0;
+ }
+ 
  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;
+   struct frame_id saved_frame_id;
+   struct cleanup *old_cleanups = NULL;
    int within_scope;
  
    /*  Only root variables can be updated... */
*************** c_value_of_root (struct varobj **var_han
*** 2172,2179 ****
      within_scope = 1;
    else
      {
        fi = frame_find_by_id (var->root->frame);
!       within_scope = fi != NULL;
        /* FIXME: select_frame could fail */
        if (fi)
  	{
--- 2189,2203 ----
      within_scope = 1;
    else
      {
+       saved_frame_id = get_frame_id (get_selected_frame (NULL));
+       old_cleanups
+ 	= make_cleanup_restore_current_thread (inferior_ptid, saved_frame_id);
+ 
+       if (iterate_over_threads (look_for_frame, &var->root->frame))
+ 	within_scope = 1;
+ 
        fi = frame_find_by_id (var->root->frame);
! 
        /* FIXME: select_frame could fail */
        if (fi)
  	{
*************** c_value_of_root (struct varobj **var_han
*** 2194,2199 ****
--- 2218,2225 ----
        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
*************** static void info_threads_command (char *
*** 60,67 ****
  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)
--- 60,65 ----
*************** restore_selected_frame (struct frame_id 
*** 496,508 ****
      }
  }
  
! struct current_thread_cleanup
! {
!   ptid_t inferior_ptid;
!   struct frame_id selected_frame_id;
! };
! 
! static void
  do_restore_current_thread_cleanup (void *arg)
  {
    struct current_thread_cleanup *old = arg;
--- 494,500 ----
      }
  }
  
! void
  do_restore_current_thread_cleanup (void *arg)
  {
    struct current_thread_cleanup *old = arg;
*************** do_restore_current_thread_cleanup (void 
*** 511,517 ****
    xfree (old);
  }
  
! static struct cleanup *
  make_cleanup_restore_current_thread (ptid_t inferior_ptid, 
                                       struct frame_id a_frame_id)
  {
--- 503,509 ----
    xfree (old);
  }
  
! 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
*************** extern void load_infrun_state (ptid_t pt
*** 143,148 ****
--- 143,159 ----
  /* 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;
  

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

[Vladimir Prus]

Nick Roberts wrote:

> 
> Currently GDB thinks variable objects have gone out of scope if the thread
> changes.  

This is indeed a major problem.

> This patch corrects that. 

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?

- Volodya

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

[Nick Roberts]

 > 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

-- 
Nick                                           http://www.inet.net.nz/~nickrob

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

[Nick Roberts]

 >  > 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,30 ****
--- 24,32 ----
  #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"
*************** struct varobj_root
*** 65,70 ****
--- 67,75 ----
    /* The frame for this expression */
    struct frame_id frame;
  
+   /* GDB thread id.  */
+   int thread_id;
+ 
    /* If 1, "update" always recomputes the frame & valid block
       using the currently selected frame. */
    int use_selected_frame;
*************** varobj_create (char *objname,
*** 492,497 ****
--- 497,507 ----
  
        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;
        expr_len = strlen (expression);
        var->name = savestring (expression, expr_len);
        /* For a root var, the name and the expr are the same.  */
*************** varobj_get_display_format (struct varobj
*** 686,691 ****
--- 696,707 ----
    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)
  {
*************** c_path_expr_of_child (struct varobj *chi
*** 2153,2189 ****
    return child->path_expr;
  }
  
  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;
  
    /*  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)
  	{
! 	  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);
! 	}	  
      }
  
    if (within_scope)
--- 2169,2229 ----
    return child->path_expr;
  }
  
+ static void
+ check_scope (struct varobj *var, int* scope)
+ {
+   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_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
      {
!       thread_id = var->root->thread_id;
!       ptid = thread_id_to_pid (thread_id);
!       if (thread_id == 0)
! 	check_scope (var, &within_scope);
!       else if (thread_id != -1 && target_thread_alive (ptid))
  	{
! 
! 	  saved_frame_id = get_frame_id (get_selected_frame (NULL));
! 	  old_cleanups = make_cleanup_restore_current_thread (inferior_ptid,
! 							      saved_frame_id);
! 	  switch_to_thread (ptid);
! 	  check_scope (var, &within_scope);
! 	}
!       else
! 	var->root->thread_id = -1;
      }
  
    if (within_scope)
*************** c_value_of_root (struct varobj **var_han
*** 2194,2199 ****
--- 2234,2241 ----
        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
*************** static void info_threads_command (char *
*** 60,67 ****
  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)
--- 60,65 ----
*************** restore_selected_frame (struct frame_id 
*** 496,508 ****
      }
  }
  
! struct current_thread_cleanup
! {
!   ptid_t inferior_ptid;
!   struct frame_id selected_frame_id;
! };
! 
! static void
  do_restore_current_thread_cleanup (void *arg)
  {
    struct current_thread_cleanup *old = arg;
--- 494,500 ----
      }
  }
  
! void
  do_restore_current_thread_cleanup (void *arg)
  {
    struct current_thread_cleanup *old = arg;
*************** do_restore_current_thread_cleanup (void 
*** 511,517 ****
    xfree (old);
  }
  
! static struct cleanup *
  make_cleanup_restore_current_thread (ptid_t inferior_ptid, 
                                       struct frame_id a_frame_id)
  {
--- 503,509 ----
    xfree (old);
  }
  
! 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
*************** extern void load_infrun_state (ptid_t pt
*** 143,148 ****
--- 143,159 ----
  /* 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
*************** extern enum varobj_display_formats varob
*** 85,90 ****
--- 85,92 ----
  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
*************** print_varobj (struct varobj *var, enum p
*** 50,55 ****
--- 50,56 ----
  {
    struct type *gdb_type;
    char *type;
+   int thread_id;
  
    ui_out_field_string (uiout, "name", varobj_get_objname (var));
    if (print_expression)
*************** print_varobj (struct varobj *var, enum p
*** 66,71 ****
--- 67,76 ----
        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);
  }

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

[Vladimir Prus]

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

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

[Nick Roberts]

 > > --- 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.
 
I don't think I need the value -2 now.  I've explained
0 and -1.

 > >    /* 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.

I think it's just to dow with the way a TAB appears when prefixed with +.

 >...
 > > +static void
 > > +check_scope (struct varobj *var, int* scope)
 > > +{
 > 
 > This function needs a comment. 'check_scope', in
 > itself, can do just about anything.

This is a small function and I think it speaks for itself.  I don't
know if GNU Coding standards dictate how much commenting there should be but
I think that in varobj.c there is too much and it makes the code harder to
read.

 >                                      Why does
 > it use out-parameter, as opposed to just returning
 > a value?

Yes, this is silly.  I've changed it to an int.

 > >    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.

Just TABs again, I think.

 > > +      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.

I've got rid of -2 and explained 0 and -1.

 > > -        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?

No.  This is only necessary in the multi-threaded case.

 > > +        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.

Sure.  I'm just testing acceptance to the idea first.

Thanks.

-- 
Nick                                           http://www.inet.net.nz/~nickrob


2008-01-30  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_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.

	* Makefile.in (varobj_h): Update dependencies.


*** varobj.c.~1.100~	2008-01-30 14:16:52.000000000 +1300
--- varobj.c	2008-01-30 13:57:24.000000000 +1300
***************
*** 24,30 ****
--- 24,32 ----
  #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"
*************** struct varobj_root
*** 65,70 ****
--- 67,77 ----
    /* The frame for this expression */
    struct frame_id frame;
  
+   /* GDB thread id.
+      0 means that the application is single-threaded.
+     -1 means that the thread is dead.  */
+   int thread_id;
+ 
    /* If 1, "update" always recomputes the frame & valid block
       using the currently selected frame. */
    int use_selected_frame;
*************** varobj_get_display_format (struct varobj
*** 686,691 ****
--- 693,704 ----
    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)
  {
*************** c_path_expr_of_child (struct varobj *chi
*** 2161,2197 ****
    return child->path_expr;
  }
  
  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;
  
    /*  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)
  	{
! 	  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);
! 	}	  
      }
  
    if (within_scope)
--- 2174,2238 ----
    return child->path_expr;
  }
  
+ static int
+ check_scope (struct varobj *var)
+ {
+   struct frame_info *fi;
+   int scope;
+ 
+   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);
+     }
+   return scope;
+ }
+ 
  static struct value *
  c_value_of_root (struct varobj **var_handle)
  {
    struct value *new_val = NULL;
    struct varobj *var = *var_handle;
!   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
      {
!       thread_id = var->root->thread_id;
!       ptid = thread_id_to_pid (thread_id);
!       if (thread_id == 0)
! 	/* Single-threaded application.  */
! 	within_scope = check_scope (var);
!       else if (thread_id != -1 && target_thread_alive (ptid))
  	{
! 
! 	  saved_frame_id = get_frame_id (get_selected_frame (NULL));
! 	  old_cleanups = make_cleanup_restore_current_thread (inferior_ptid,
! 							      saved_frame_id);
! 	  switch_to_thread (ptid);
! 	  within_scope = check_scope (var);
! 	}
!       else
! 	/* Mark it as dead.  */
! 	var->root->thread_id = -1;
      }
  
    if (within_scope)
*************** c_value_of_root (struct varobj **var_han
*** 2202,2207 ****
--- 2243,2250 ----
        return new_val;
      }
  
+   do_cleanups (old_cleanups);
+ 
    return NULL;
  }

  
*** Makefile.in.~1.977.~	2008-01-30 12:04:18.000000000 +1300
--- Makefile.in	2008-01-30 14:02:06.000000000 +1300
*************** user_regs_h = user-regs.h
*** 901,907 ****
  valprint_h = valprint.h
  value_h = value.h $(doublest_h) $(frame_h) $(symtab_h) $(gdbtypes_h) \
  	$(expression_h)
! varobj_h = varobj.h $(symtab_h) $(gdbtypes_h)
  vax_tdep_h = vax-tdep.h
  vec_h = vec.h $(gdb_assert_h) $(gdb_string_h)
  version_h = version.h
--- 901,907 ----
  valprint_h = valprint.h
  value_h = value.h $(doublest_h) $(frame_h) $(symtab_h) $(gdbtypes_h) \
  	$(expression_h)
! varobj_h = varobj.h $(symtab_h) $(gdbtypes_h) $(inferior_h) $(gdbthread_h)
  vax_tdep_h = vax-tdep.h
  vec_h = vec.h $(gdb_assert_h) $(gdb_string_h)
  version_h = version.h


*** thread.c.~1.59.~	2008-01-24 09:47:31.000000000 +1300
--- thread.c	2008-01-24 15:38:41.000000000 +1300
*************** static void info_threads_command (char *
*** 60,67 ****
  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)
--- 60,65 ----
*************** restore_selected_frame (struct frame_id 
*** 496,508 ****
      }
  }
  
! struct current_thread_cleanup
! {
!   ptid_t inferior_ptid;
!   struct frame_id selected_frame_id;
! };
! 
! static void
  do_restore_current_thread_cleanup (void *arg)
  {
    struct current_thread_cleanup *old = arg;
--- 494,500 ----
      }
  }
  
! void
  do_restore_current_thread_cleanup (void *arg)
  {
    struct current_thread_cleanup *old = arg;
*************** do_restore_current_thread_cleanup (void 
*** 511,517 ****
    xfree (old);
  }
  
! static struct cleanup *
  make_cleanup_restore_current_thread (ptid_t inferior_ptid, 
                                       struct frame_id a_frame_id)
  {
--- 503,509 ----
    xfree (old);
  }
  
! 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
*************** extern void load_infrun_state (ptid_t pt
*** 143,148 ****
--- 143,159 ----
  /* 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
*************** extern enum varobj_display_formats varob
*** 85,90 ****
--- 85,92 ----
  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
*************** print_varobj (struct varobj *var, enum p
*** 50,55 ****
--- 50,56 ----
  {
    struct type *gdb_type;
    char *type;
+   int thread_id;
  
    ui_out_field_string (uiout, "name", varobj_get_objname (var));
    if (print_expression)
*************** print_varobj (struct varobj *var, enum p
*** 66,71 ****
--- 67,76 ----
        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);
  }

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

[Vladimir Prus]

On Wednesday 30 January 2008 04:22:56 Nick Roberts wrote:

>  > > +  /* 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.
>  
> I don't think I need the value -2 now.  I've explained
> 0 and -1.

Ok, thanks.
 
>  > >    /* 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.
> 
> I think it's just to dow with the way a TAB appears when prefixed with +.

I don't see such effect in other gdb-patches postings, FWIW.

> 
>  >...
>  > > +static void
>  > > +check_scope (struct varobj *var, int* scope)
>  > > +{
>  > 
>  > This function needs a comment. 'check_scope', in
>  > itself, can do just about anything.
> 
> This is a small function and I think it speaks for itself.

Not in my opinion. 'check_scope' can mean anything.

> I don't 
> know if GNU Coding standards dictate how much commenting there should be but
> I think that in varobj.c there is too much and it makes the code harder to
> read.

I disagree -- practice shows that the exact semantics of varobj
logic is still not 100% clear.

>  > > -        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?
> 
> No.  This is only necessary in the multi-threaded case.

make_cleanup_restore_current_thread arranges for two things to
be restored:
   - current thread
   - selected frame

We now always save/restore selected frame in varobj_update. And
changing/saving/restoring current thread is extremely fast. So,
why not just save both thread and frame in all cases, and simplify
the logic.

> 
>  > > +        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.
> 
> Sure.  I'm just testing acceptance to the idea first.


> 2008-01-30  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_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.
>
>         * Makefile.in (varobj_h): Update dependencies.
>
>
> --- varobj.c.~1.100~    2008-01-30 14:16:52.000000000 +1300
> +++ varobj.c    2008-01-30 13:57:24.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,11 @@
>    /* The frame for this expression */
>    struct frame_id frame;
>
> +  /* GDB thread id.
> +     0 means that the application is single-threaded.
> +    -1 means that the thread is dead.  */

I'd expand this as follows:

	/* If frame is not null_frame_id, the id of the thread
	   that the frame belongs to.
           0 means that the application is single-threaded.
          -1 means that the thread is dead.  */

> +  int thread_id;
> +
>    /* If 1, "update" always recomputes the frame & valid block
>       using the currently selected frame. */
>    int use_selected_frame;
> @@ -686,6 +693,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)
>  {
> @@ -2161,37 +2174,65 @@
>    return child->path_expr;
>  }
>
> +static int
> +check_scope (struct varobj *var)
> +{
> +  struct frame_info *fi;
> +  int scope;
> +
> +  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);
> +    }
> +  return scope;
> +}
> +
>  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)
> +      /* Single-threaded application.  */
> +      within_scope = check_scope (var);
> +      else if (thread_id != -1 && target_thread_alive (ptid))
>        {
> -        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); +        switch_to_thread (ptid);
> +        within_scope = check_scope (var);
> +      }
> +      else
> +      /* Mark it as dead.  */
> +      var->root->thread_id = -1;
>      }
>
>    if (within_scope)
> @@ -2202,6 +2243,8 @@
>        return new_val;
>      }
>
> +  do_cleanups (old_cleanups);
> +
>    return NULL;
>  }
>
>
> --- Makefile.in.~1.977.~        2008-01-30 12:04:18.000000000 +1300
> +++ Makefile.in 2008-01-30 14:02:06.000000000 +1300
> @@ -901,7 +901,7 @@
>  valprint_h = valprint.h
>  value_h = value.h $(doublest_h) $(frame_h) $(symtab_h) $(gdbtypes_h) \
>        $(expression_h)
> -varobj_h = varobj.h $(symtab_h) $(gdbtypes_h)
> +varobj_h = varobj.h $(symtab_h) $(gdbtypes_h) $(inferior_h) $(gdbthread_h)
>  vax_tdep_h = vax-tdep.h
>  vec_h = vec.h $(gdb_assert_h) $(gdb_string_h)
>  version_h = version.h
>
>
> --- 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);
>  }

I don't have any more objections to the code, except for the
minor ones above in this email. Of course, I still would like
automated test to be checked in together with the patch.

- Volodya

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

[Vladimir Prus]

On Wednesday 30 January 2008 10:57:29 Nick Roberts wrote:
>  > >  > > +static void
>  > >  > > +check_scope (struct varobj *var, int* scope)
>  > >  > > +{
>  > >  > 
>  > >  > This function needs a comment. 'check_scope', in
>  > >  > itself, can do just about anything.
>  > > 
>  > > This is a small function and I think it speaks for itself.
>  > 
>  > Not in my opinion. 'check_scope' can mean anything.
> 
> The function name doesn't speak for itself, but being only about twelve lines
> of code, it is easy to see what it does..

It might be easy to see what the function body does. But I don't know
what's the intended usage of the function is, and when the preconditions are
(for example, calling this function when varobj has no associated frame is
probably not going to work).

For example, here's what I'd write as comment:

	/* If the frame associated with this varobj exists,
           and the address in that frame is inside varobj's block,
	   switch to the frame and return 1.  Otherwise, return 0.
	   This function should be called only for variable object
	   that are associated with frame and block, and only in
           the thread associated with the frame.  */

These are 6 lines of english text, and one can understand everything
he needs to know about check_scope, without looking at 12 lines
of body, and without knowing anything about functions called in
that body.

> 
>  > > I don't 
>  > > know if GNU Coding standards dictate how much commenting there should be but
>  > > I think that in varobj.c there is too much and it makes the code harder to
>  > > read.
>  > 
>  > I disagree -- practice shows that the exact semantics of varobj
>  > logic is still not 100% clear.
> 
> But it looks like the coding standards allow us to choose our own style.

Yes, the GNU coding standards mostly have to do with brace placement, that's
why I don't understand why you use it as reference in this case.

I can only repeat that in practice, varobj.c proves hard to understand, which suggest
it needs more comments, not less.

>  > >  > > +        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?
>  > > 
>  > > No.  This is only necessary in the multi-threaded case.
>  > 
>  > make_cleanup_restore_current_thread arranges for two things to
>  > be restored:
>  >    - current thread
>  >    - selected frame
>  > 
>  > We now always save/restore selected frame in varobj_update. And
>  > changing/saving/restoring current thread is extremely fast. So,
>  > why not just save both thread and frame in all cases, and simplify
>  > the logic.
> 
> Then we presumably could remove the calls after value_of_root:
> 
>       /* Restore selected frame.  */
>       fi = frame_find_by_id (old_fid);
>       if (fi)
> 	select_frame (fi);
> 
> but I have no idea how fast do_restore_current_thread_cleanup is.  Indirectly,
> it calls functions like reinit_frame_cache.

Well:

	void
	switch_to_thread (ptid_t ptid)
	{
	  if (ptid_equal (ptid, inferior_ptid))
	    return;

	  inferior_ptid = ptid;
	  reinit_frame_cache ();
	  registers_changed ();
	  stop_pc = read_pc ();
	}

So in the case of non-threaded program, we exit early and pay no price.

- Volodya