per-thread variable objects

per-thread variable objects

[Vladimir Prus]

[-- Attachment #1: Type: text/plain, Size: 649 bytes --]


I've checked it the following revision of Nick's previous
patch (http://permalink.gmane.org/gmane.comp.gdb.devel/22707) that
makes variable object remember the thread they belong to.
I've tested this manually, proper thread and docs will be following
shortly.

Nick,
the changes I've made are as follows:

	- I use the valid_block instead of special -2 value
	to check if a varobj is global.
	- I did not use -1 value to indicate the thread is gone. Presently,
	we don't specifically mark variables object whose frame is gone,
	and I don't see why we should be do differently for threads.

If something seems wrong, please say.

Thansk,
Volodya




[-- Attachment #2: commit.diff --]
[-- Type: text/x-diff, Size: 5916 bytes --]

Index: gdb/ChangeLog
===================================================================
RCS file: /cvs/src/src/gdb/ChangeLog,v
retrieving revision 1.9247
diff -u -p -r1.9247 ChangeLog
--- gdb/ChangeLog	23 Mar 2008 17:29:32 -0000	1.9247
+++ gdb/ChangeLog	24 Mar 2008 17:31:09 -0000
@@ -1,3 +1,15 @@
+2008-03-24  Nick Roberts  <nickrob@snap.net.nz>
+	    Vladimir Prus  <vladimir@codesourcery.com>
+
+        * varobj.c  (struct varobj_root): New component thread_id.
+        (varobj_get_thread_id, check_scope): New functions.
+        (c_value_of_root): Use check_scope.  Switch to the
+	proper thread if necessary.
+
+        * varobj.h (varobj_get_thread_id): New extern.
+
+        * mi/mi-cmd-var.c (print_varobj): Add thread-id field.
+
 2008-03-23  Daniel Jacobowitz  <dan@codesourcery.com>
 
 	PR gdb/544
Index: gdb/varobj.c
===================================================================
RCS file: /cvs/src/src/gdb/varobj.c,v
retrieving revision 1.105
diff -u -p -r1.105 varobj.c
--- gdb/varobj.c	23 Mar 2008 09:53:52 -0000	1.105
+++ gdb/varobj.c	24 Mar 2008 17:31:11 -0000
@@ -68,6 +68,13 @@ struct varobj_root
      not NULL.  */
   struct frame_id frame;
 
+  /* The thread ID that this varobj_root belong to.  This field
+     is only valid if valid_block is not NULL.  
+     When not 0, indicates which thread 'frame' belongs to.
+     When 0, indicates that the thread list was empty when the varobj_root
+     was created.  */
+  int thread_id;
+
   /* If 1, "update" always recomputes the frame & valid block
      using the currently selected frame. */
   int use_selected_frame;
@@ -503,8 +510,9 @@ varobj_create (char *objname,
       if (innermost_block && fi != NULL)
 	{
 	  var->root->frame = get_frame_id (fi);
+	  var->root->thread_id = pid_to_thread_id (inferior_ptid);
 	  old_fi = get_selected_frame (NULL);
-	  select_frame (fi);
+	  select_frame (fi);	 
 	}
 
       /* We definitely need to catch errors here.
@@ -692,6 +700,19 @@ varobj_get_display_format (struct varobj
   return var->format;
 }
 
+/* If the variable object is bound to a specific thread, that
+   is its evaluation can always be done in context of a frame
+   inside that thread, returns GDB id of the thread -- which
+   is always positive.  Otherwise, returns -1. */
+int
+varobj_get_thread_id (struct varobj *var)
+{
+  if (var->root->valid_block && var->root->thread_id > 0)
+    return var->root->thread_id;
+  else
+    return -1;
+}
+
 void
 varobj_set_frozen (struct varobj *var, int frozen)
 {
@@ -2138,13 +2159,36 @@ c_path_expr_of_child (struct varobj *chi
   return child->path_expr;
 }
 
+/* If frame associated with VAR can be found, switch
+   to it and return 1.  Otherwise, return 0.  */
+static int
+check_scope (struct varobj *var)
+{
+  struct frame_info *fi;
+  int scope;
+
+  fi = frame_find_by_id (var->root->frame);
+  scope = fi != NULL;
+
+  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;
+  int within_scope = 0;
   struct cleanup *back_to;
 								 
   /*  Only root variables can be updated... */
@@ -2158,20 +2202,22 @@ c_value_of_root (struct varobj **var_han
   /* Determine whether the variable is still around. */
   if (var->root->valid_block == NULL || var->root->use_selected_frame)
     within_scope = 1;
+  else if (var->root->thread_id == 0)
+    {
+      /* The program was single-threaded when the variable object was
+	 created.  Technically, it's possible that the program became
+	 multi-threaded since then, but we don't support such
+	 scenario yet.  */
+      within_scope = check_scope (var);	  
+    }
   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);
-	}	  
+      ptid_t ptid = thread_id_to_pid (var->root->thread_id);
+      if (in_thread_list (ptid))
+	{
+	  switch_to_thread (ptid);
+	  within_scope = check_scope (var);
+	}
     }
 
   if (within_scope)
Index: gdb/varobj.h
===================================================================
RCS file: /cvs/src/src/gdb/varobj.h,v
retrieving revision 1.15
diff -u -p -r1.15 varobj.h
--- gdb/varobj.h	30 Jan 2008 07:17:31 -0000	1.15
+++ gdb/varobj.h	24 Mar 2008 17:31:11 -0000
@@ -89,6 +89,8 @@ extern enum varobj_display_formats varob
 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);
Index: gdb/mi/mi-cmd-var.c
===================================================================
RCS file: /cvs/src/src/gdb/mi/mi-cmd-var.c,v
retrieving revision 1.45
diff -u -p -r1.45 mi-cmd-var.c
--- gdb/mi/mi-cmd-var.c	30 Jan 2008 07:17:31 -0000	1.45
+++ gdb/mi/mi-cmd-var.c	24 Mar 2008 17:31:13 -0000
@@ -50,6 +50,7 @@ print_varobj (struct varobj *var, enum p
 {
   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 @@ print_varobj (struct varobj *var, enum p
       xfree (type);
     }
 
+  thread_id = varobj_get_thread_id (var);
+  if (thread_id > 0)
+    ui_out_field_int (uiout, "thread-id", thread_id);
+
   if (varobj_get_frozen (var))
     ui_out_field_int (uiout, "frozen", 1);
 }

Re: per-thread variable objects

[Nick Roberts]

 > I've checked it the following revision of Nick's previous
 > patch (http://permalink.gmane.org/gmane.comp.gdb.devel/22707) that
 > makes variable object remember the thread they belong to.
 > I've tested this manually, proper thread and docs will be following
 > shortly.
 > 
 > Nick,
 > the changes I've made are as follows:
 > 
 > 	- I use the valid_block instead of special -2 value
 > 	to check if a varobj is global.
 > 	- I did not use -1 value to indicate the thread is gone. Presently,
 > 	we don't specifically mark variables object whose frame is gone,
 > 	and I don't see why we should be do differently for threads.
 > 
 > If something seems wrong, please say.

It would be nice to reach an agreement before the changes are commmitted.
Clearly as maintainer, you have final say if we can't reach such an
agreement.

The final patch looks cleaner than my original one and appears to do the same
thing.  Thanks.

 > Index: gdb/ChangeLog
 > ===================================================================
 > RCS file: /cvs/src/src/gdb/ChangeLog,v
 > retrieving revision 1.9247
 > diff -u -p -r1.9247 ChangeLog
 > --- gdb/ChangeLog	23 Mar 2008 17:29:32 -0000	1.9247
 > +++ gdb/ChangeLog	24 Mar 2008 17:31:09 -0000
 > @@ -1,3 +1,15 @@
 >...

I think ChangeLog entries shouldn't be presented as diffs.

 >...
 > +/* If frame associated with VAR can be found, switch
 > +   to it and return 1.  Otherwise, return 0.  */

It's not enough for a frame to be found, the pc also has to have an address
value within the valid block.  This is one reason why, for such small
functions, I would rather the code spoke for itself.

 > +static int
 > +check_scope (struct varobj *var)
 > +{
 > +  struct frame_info *fi;
 > +  int scope;
 > +
 > +  fi = frame_find_by_id (var->root->frame);
 > +  scope = fi != NULL;
 > +
 > +  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;
 > +}

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

Re: per-thread variable objects

[Vladimir Prus]

On Monday 24 March 2008 23:59:20 Nick Roberts wrote:
>  > I've checked it the following revision of Nick's previous
>  > patch (http://permalink.gmane.org/gmane.comp.gdb.devel/22707) that
>  > makes variable object remember the thread they belong to.
>  > I've tested this manually, proper thread and docs will be following
>  > shortly.
>  > 
>  > Nick,
>  > the changes I've made are as follows:
>  > 
>  > 	- I use the valid_block instead of special -2 value
>  > 	to check if a varobj is global.
>  > 	- I did not use -1 value to indicate the thread is gone. Presently,
>  > 	we don't specifically mark variables object whose frame is gone,
>  > 	and I don't see why we should be do differently for threads.
>  > 
>  > If something seems wrong, please say.
> 
> It would be nice to reach an agreement before the changes are commmitted.

You are right; I apologise for rushing to cross an item from my todo list.

>  > Index: gdb/ChangeLog
>  > ===================================================================
>  > RCS file: /cvs/src/src/gdb/ChangeLog,v
>  > retrieving revision 1.9247
>  > diff -u -p -r1.9247 ChangeLog
>  > --- gdb/ChangeLog	23 Mar 2008 17:29:32 -0000	1.9247
>  > +++ gdb/ChangeLog	24 Mar 2008 17:31:09 -0000
>  > @@ -1,3 +1,15 @@
>  >...
> 
> I think ChangeLog entries shouldn't be presented as diffs.

Does that cause any practical issue to anybody?

>  >...
>  > +/* If frame associated with VAR can be found, switch
>  > +   to it and return 1.  Otherwise, return 0.  */
> 
> It's not enough for a frame to be found, the pc also has to have an address
> value within the valid block.  This is one reason why, for such small
> functions, I would rather the code spoke for itself.

To be honest, the checking for block boundaries cries for comments too.
It's not really appparent why we check for it, and importantly -- should
variables not in the current block be reported as "out of scope", or 
reported in some other way.

- Volodya

Re: per-thread variable objects

[Eli Zaretskii]

> From: Vladimir Prus <vladimir@codesourcery.com>
> Date: Tue, 25 Mar 2008 08:43:24 +0300
> Cc: gdb-patches@sources.redhat.com
> 
> >  > --- gdb/ChangeLog	23 Mar 2008 17:29:32 -0000	1.9247
> >  > +++ gdb/ChangeLog	24 Mar 2008 17:31:09 -0000
> >  > @@ -1,3 +1,15 @@
> >  >...
> > 
> > I think ChangeLog entries shouldn't be presented as diffs.
> 
> Does that cause any practical issue to anybody?

Not a big deal, but it is indeed customary to send ChangeLog entries
themselves, not as diffs.

Re: per-thread variable objects

[Daniel Jacobowitz]

On Wed, Mar 26, 2008 at 06:31:27AM +0200, Eli Zaretskii wrote:
> Not a big deal, but it is indeed customary to send ChangeLog entries
> themselves, not as diffs.

For instance, if I need to grab the patch from the mailing list
archives to backport it to an older version of GDB - I do that often.

-- 
Daniel Jacobowitz
CodeSourcery

Re: per-thread variable objects

[Daniel Jacobowitz]

On Mon, Mar 24, 2008 at 08:46:06PM +0300, Vladimir Prus wrote:
> @@ -66,6 +67,10 @@ print_varobj (struct varobj *var, enum p
>        xfree (type);
>      }
>  
> +  thread_id = varobj_get_thread_id (var);
> +  if (thread_id > 0)
> +    ui_out_field_int (uiout, "thread-id", thread_id);
> +
>    if (varobj_get_frozen (var))
>      ui_out_field_int (uiout, "frozen", 1);
>  }

Please add this field to the manual.

-- 
Daniel Jacobowitz
CodeSourcery