This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH] Variable objects in multi-threaded programs
> > --- 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);
}