From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 28666 invoked by alias); 30 Jan 2008 07:35:05 -0000 Received: (qmail 28655 invoked by uid 22791); 30 Jan 2008 07:35:03 -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; Wed, 30 Jan 2008 07:34:36 +0000 Received: (qmail 28794 invoked from network); 30 Jan 2008 07:34:33 -0000 Received: from unknown (HELO 172.16.unknown.plus.ru) (vladimir@127.0.0.2) by mail.codesourcery.com with ESMTPA; 30 Jan 2008 07:34:33 -0000 From: Vladimir Prus To: Nick Roberts Subject: Re: [PATCH] Variable objects in multi-threaded programs Date: Wed, 30 Jan 2008 07:35: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> <200801292040.13986.vladimir@codesourcery.com> <18335.53616.458206.932383@kahikatea.snap.net.nz> In-Reply-To: <18335.53616.458206.932383@kahikatea.snap.net.nz> MIME-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: 7bit Content-Disposition: inline Message-Id: <200801301034.54367.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/msg00773.txt.bz2 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 > > * 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