From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 16351 invoked by alias); 30 Jan 2008 01:23:38 -0000 Received: (qmail 16285 invoked by uid 22791); 30 Jan 2008 01:23:36 -0000 X-Spam-Check-By: sourceware.org Received: from viper.snap.net.nz (HELO viper.snap.net.nz) (202.37.101.8) by sourceware.org (qpsmtpd/0.31) with ESMTP; Wed, 30 Jan 2008 01:23:06 +0000 Received: from kahikatea.snap.net.nz (192.31.255.123.static.snap.net.nz [123.255.31.192]) by viper.snap.net.nz (Postfix) with ESMTP id 547833DA32E; Wed, 30 Jan 2008 14:23:00 +1300 (NZDT) Received: by kahikatea.snap.net.nz (Postfix, from userid 1000) id 0359E8FC6D; Wed, 30 Jan 2008 14:22:57 +1300 (NZDT) From: Nick Roberts MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Transfer-Encoding: 7bit Message-ID: <18335.53616.458206.932383@kahikatea.snap.net.nz> Date: Wed, 30 Jan 2008 01:49:00 -0000 To: Vladimir Prus Cc: gdb-patches@sources.redhat.com Subject: Re: [PATCH] Variable objects in multi-threaded programs In-Reply-To: <200801292040.13986.vladimir@codesourcery.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> <200801292040.13986.vladimir@codesourcery.com> X-Mailer: VM 7.19 under Emacs 23.0.60.5 X-IsSubscribed: yes 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/msg00753.txt.bz2 > > --- 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 * 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); }