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