From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 22023 invoked by alias); 6 Jan 2012 16:23:22 -0000 Received: (qmail 22014 invoked by uid 22791); 6 Jan 2012 16:23:17 -0000 X-SWARE-Spam-Status: No, hits=-2.6 required=5.0 tests=AWL,BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,FREEMAIL_FROM,RCVD_IN_DNSWL_LOW X-Spam-Check-By: sourceware.org Received: from mail-ww0-f41.google.com (HELO mail-ww0-f41.google.com) (74.125.82.41) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Fri, 06 Jan 2012 16:23:04 +0000 Received: by wgbdt12 with SMTP id dt12so1869010wgb.0 for ; Fri, 06 Jan 2012 08:23:03 -0800 (PST) Received: by 10.181.12.43 with SMTP id en11mr12545918wid.6.1325866983027; Fri, 06 Jan 2012 08:23:03 -0800 (PST) Received: from [192.168.0.103] ([2.82.184.26]) by mx.google.com with ESMTPS id g11sm34740105wbo.6.2012.01.06.08.23.01 (version=SSLv3 cipher=OTHER); Fri, 06 Jan 2012 08:23:01 -0800 (PST) Message-ID: <4F071FE4.5020009@gmail.com> Date: Fri, 06 Jan 2012 16:23:00 -0000 From: Pedro Alves User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:9.0) Gecko/20111222 Thunderbird/9.0 MIME-Version: 1.0 To: luis_gustavo@mentor.com CC: gdb-patches@sourceware.org Subject: Re: [RFC stub-side break conditions 3/5] GDB-side changes References: <4F05BA10.3090107@mentor.com> In-Reply-To: <4F05BA10.3090107@mentor.com> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit 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: 2012-01/txt/msg00240.txt.bz2 On 01/05/2012 02:56 PM, Luis Machado wrote: > Regarding always-inserted mode, if it is "on" we must send condition changes right away, or else the target will potentially miss breakpoint hits. Could you elaborate a bit more on how will it miss breakpoint hits? > I'd like to hear about the change to "info break" As they say, a picture is worth a thousand words. How does the new output look like? What about MI? > and also the way we should pass conditions down to the stub. Currently i'm adding the agent expressions to a condition list in the target info field of bp locations. Sounds good at first sight. I'm mostly wondering whether the assumption that we can re-insert a breakpoint at the same address is solid. > +static int > +gdb_evaluates_breakpoint_condition_p (void) > +{ > + const char *mode = breakpoint_condition_evaluation_mode (); > + > + return (mode == condition_evaluation_gdb? 1 : 0); Missing space before `?'. Or just write return (mode == condition_evaluation_gdb); Same thing. > +} > +/* Iterates through locations with address ADDRESS. BP_LOCP_TMP points to > + each object. BP_LOCP_START points to where the loop should start from. > + If BP_LOCP_START is a NULL pointer, the macro automatically seeks the > + appropriate location to start with. */ > + > +#define ALL_BP_LOCATIONS_AT_ADDR(BP_LOCP_TMP, BP_LOCP_START, ADDRESS) \ > + if (!BP_LOCP_START) \ > + BP_LOCP_START = get_first_locp_gte_addr (ADDRESS); \ > + for (BP_LOCP_TMP = BP_LOCP_START; \ > + (BP_LOCP_TMP< bp_location + bp_location_count \ > + && (*BP_LOCP_TMP)->address == ADDRESS); \ > + BP_LOCP_TMP++) > + The address alone is not enough when you consider multi-process. The users of this macro aren't considering that. Several instances of this problem. > +/* Based on location BL, create a list of breakpoint conditions to be > + passed on to the target. If we have duplicated locations with different > + conditions, we will add such conditions to the list. The idea is that the > + target will evaluate the list of conditions and will only notify GDB when > + one of them is true. */ What does the return code mean? > + > +static int > +build_target_condition_list (struct bp_location *bl) > +{ On 01/05/2012 02:56 PM, Luis Machado wrote: > + } > + /* We will never reach this point. */ > + } Then by all means gdb_assert. :-) > + /* Even if the target evaluated the condition on its end and notified GDB, we > + need to do so again since GDB does not know if we stopped due to a > + breakpoint or a single step. > + This will only be done when we single step straight to a conditional breakpoint. */ Where is the code that makes this true? > + > if (frame_id_p (b->frame_id) > && !frame_id_eq (b->frame_id, get_stack_frame_id (get_current_frame ()))) > bs->stop = 0; > @@ -4905,6 +5219,19 @@ print_one_breakpoint_location (struct br > else > ui_out_text (uiout, "\tstop only if "); > ui_out_field_string (uiout, "cond", b->cond_string); > + > + /* Print whether the remote stub is doing the breakpoint's condition > + evaluation. If GDB is doing the evaluation, don't print anything. */ > + if (loc&& is_breakpoint (b)&& loc->cond_bytecode > + && breakpoint_condition_evaluation_mode () > + != condition_evaluation_gdb) What if you set a conditional breakpoint, have it evaluate the in target, and then, flip the setting to gdb-side, while the breakpoint's condition is still being evaluated on the target side? Shouldn't this always print "target"/"stub" when loc->cond_bytecode is non-NULL? > + { > + ui_out_text (uiout, " ("); > + ui_out_field_string (uiout, "condeval", > + breakpoint_condition_evaluation_mode ()); > + ui_out_text (uiout, ")"); > + } > + > ui_out_text (uiout, "\n"); > } > @@ -10366,12 +10696,56 @@ swap_insertion (struct bp_location *left > > left->inserted = right->inserted; > left->duplicate = right->duplicate; > + left->needs_update = right->needs_update; > left->target_info = right->target_info; > right->inserted = left_inserted; > right->duplicate = left_duplicate; > + left->needs_update = right->needs_update; Doesn't look right. Should probably be const int left_needs_update = left->needs_update; ... left->needs_update = right->needs_update; ... right->needs_update = left_needs_update; > + modified = modified? 1 : (*loc2p)->condition_changed; Space before `?'. Perhaps clearer alternatives are: modified = modified || (*loc2p)->condition_changed; or even the common: if (!modified) modified = (*loc2p)->condition_changed; > + /* Check if this is a new/deleted duplicated location or a duplicated > + location that had its condition modified. If so, we want to send its Spurious space. > @@ -11387,7 +11810,10 @@ delete_breakpoint (struct breakpoint *bp > itself, since remove_breakpoint looks at location's owner. It > might be better design to have location completely > self-contained, but it's not the case now. */ > - update_global_location_list (0); > + if (is_breakpoint (bpt)) > + update_global_location_list (1); The whole point of the update_global_location_list parameter is being able to say "don't insert locations new locations because I'm just deleting a breakpoint". This was necessary for situations like following an exec, IIRC. > + else > + update_global_location_list (0); > > +/* List of conditions used to pass conditions down to the target. */ > +struct condition_list > +{ > + /* Breakpoint condition. */ > + struct agent_expr *expr; > + /* Pointer to the next condition. */ > + struct condition_list *next; > +}; Can this be a VEC of `struct agent_expr *' instead?