From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 6009 invoked by alias); 29 Jan 2008 17:40:32 -0000 Received: (qmail 5997 invoked by uid 22791); 29 Jan 2008 17:40:31 -0000 X-Spam-Check-By: sourceware.org Received: from mail.codesourcery.com (HELO mail.codesourcery.com) (65.74.133.4) by sourceware.org (qpsmtpd/0.31) with ESMTP; Tue, 29 Jan 2008 17:40:03 +0000 Received: (qmail 15581 invoked from network); 29 Jan 2008 17:40:01 -0000 Received: from unknown (HELO 172.16.unknown.plus.ru) (vladimir@127.0.0.2) by mail.codesourcery.com with ESMTPA; 29 Jan 2008 17:40:01 -0000 From: Vladimir Prus To: Nick Roberts Subject: Re: [PATCH] Variable objects in multi-threaded programs Date: Tue, 29 Jan 2008 17:43:00 -0000 User-Agent: KMail/1.9.6 (enterprise 0.20070907.709405) Cc: gdb-patches@sources.redhat.com References: <18328.5277.85018.374650@kahikatea.snap.net.nz> <18328.16293.286454.235065@kahikatea.snap.net.nz> <18329.3782.282262.175798@kahikatea.snap.net.nz> In-Reply-To: <18329.3782.282262.175798@kahikatea.snap.net.nz> MIME-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Content-Disposition: inline Message-Id: <200801292040.13986.vladimir@codesourcery.com> Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org X-SW-Source: 2008-01/txt/msg00698.txt.bz2 On Friday 25 January 2008 01:18:46 Nick Roberts wrote: > > =A0> I'm somewhat concerned that this patch makes gdb traverse stacks o= f all > =A0> =A0> threads -- it can take quite some time. Would a better solution= be > =A0> =A0> to store thread id inside varobj? > =A0>=20 > =A0> Yes I think it would. =A0As an element of struct varobj_root? > =A0>=20 > =A0> Perhaps this should also included as a field in the output of -var-c= reate, > =A0> the frontend could organise the display of watch expressions by thre= ad. =A0If > =A0> we use the GDB thread id, this would be consistent with infrun.c >=20 > Here's a revised patch along those lines. =A0The diffs for gdbthread.h, t= hread.c > are as before, that of varobj.c is new and there are now ones for mi-cmd-= var.c > and varobj.h. >=20 > --=20 > Nick =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 = =A0 =A0 =A0 =A0 http://www.inet.net.nz/~nickrob >=20 > 2008-01-25 =A0Nick Roberts =A0 >=20 > =A0=A0=A0=A0=A0=A0=A0=A0* thread.c (make_cleanup_restore_current_thread) > =A0=A0=A0=A0=A0=A0=A0=A0(do_restore_current_thread_cleanup): Don't make s= tatic > =A0=A0=A0=A0=A0=A0=A0=A0(struct current_thread_cleanup): Move to gdbthrea= d.h >=20 > =A0=A0=A0=A0=A0=A0=A0=A0* gdbthread.h: Declare above functions as externs= here. >=20 > =A0=A0=A0=A0=A0=A0=A0=A0* varobj.c =A0(struct varobj_root): New component= thread_id. > =A0=A0=A0=A0=A0=A0=A0=A0(varobj_create): Set it to -2 for global variable= s. > =A0=A0=A0=A0=A0=A0=A0=A0(varobj_get_thread_id, check_scope): New function= s. > =A0=A0=A0=A0=A0=A0=A0=A0(c_value_of_root): Use it to iterate over threads. >=20 > =A0=A0=A0=A0=A0=A0=A0=A0* varobj.h (varobj_get_thread_id): New extern. >=20 > =A0=A0=A0=A0=A0=A0=A0=A0* mi/mi-cmd-var.c (print_varobj): Add thread-id f= ield. > --- 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 =3D variable_default_display (var); > var->root->valid_block =3D innermost_block; > + if (innermost_block) > + var->root->thread_id =3D pid_to_thread_id (inferior_ptid); > + else > + /* Give global variables a thread_id of -2. */ > + var->root->thread_id =3D -2; Something is wrong with indentation here. > expr_len =3D strlen (expression); > var->name =3D 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 =3D frame_find_by_id (var->root->frame); > + *scope =3D fi !=3D NULL; > + > + /* FIXME: select_frame could fail */ > + if (fi) > + { > + CORE_ADDR pc =3D get_frame_pc (fi); > + if (pc < BLOCK_START (var->root->valid_block) || > + pc >=3D BLOCK_END (var->root->valid_block)) > + *scope =3D 0; > + else > + select_frame (fi); > + } > +} > + > static struct value * > c_value_of_root (struct varobj **var_handle) > { > struct value *new_val =3D NULL; > struct varobj *var =3D *var_handle; > - struct frame_info *fi; > - int within_scope; > + struct frame_id saved_frame_id; > + struct cleanup *old_cleanups =3D 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 =3D=3D NULL || var->root->use_selected_fram= e) > within_scope =3D 1; > else > { > - fi =3D frame_find_by_id (var->root->frame); > - within_scope =3D fi !=3D NULL; > - /* FIXME: select_frame could fail */ > - if (fi) > + thread_id =3D var->root->thread_id; > + ptid =3D thread_id_to_pid (thread_id); > + if (thread_id =3D=3D 0) > + check_scope (var, &within_scope); Something appears wrong with indentation here. > + else if (thread_id !=3D -1 && target_thread_alive (ptid)) > { I'm confused about meaning of thread_id=3D=3D0, thread_id=3D=3D-1=20 and thread_id=3D=3D-2. Can you clarify that and add that to=20 varobj_root->thread_id comment? We probably need some comments for the branches here. > - CORE_ADDR pc =3D get_frame_pc (fi); > - if (pc < BLOCK_START (var->root->valid_block) || > - pc >=3D BLOCK_END (var->root->valid_block)) > - within_scope =3D 0; > - else > - select_frame (fi); > - } > + > + saved_frame_id =3D get_frame_id (get_selected_frame (NULL)); > + old_cleanups =3D make_cleanup_restore_current_thread (inferior_p= tid, > saved_frame_id);=20 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);=20 > + check_scope (var, &within_scope); > + } > + else > + var->root->thread_id =3D -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 =3D 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 =3D varobj_get_thread_id (var); > + if (thread_id !=3D -2) > + ui_out_field_int (uiout, "thread-id", thread_id); > + > if (varobj_get_frozen (var)) > ui_out_field_int (uiout, "frozen", 1); > } - Volodya