From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 14589 invoked by alias); 10 Jan 2012 11:14:10 -0000 Received: (qmail 14577 invoked by uid 22791); 10 Jan 2012 11:14:08 -0000 X-SWARE-Spam-Status: No, hits=-1.7 required=5.0 tests=AWL,BAYES_00 X-Spam-Check-By: sourceware.org Received: from relay1.mentorg.com (HELO relay1.mentorg.com) (192.94.38.131) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Tue, 10 Jan 2012 11:13:54 +0000 Received: from svr-orw-exc-10.mgc.mentorg.com ([147.34.98.58]) by relay1.mentorg.com with esmtp id 1RkZeL-0001NK-NO from Luis_Gustavo@mentor.com ; Tue, 10 Jan 2012 03:13:53 -0800 Received: from NA1-MAIL.mgc.mentorg.com ([147.34.98.181]) by SVR-ORW-EXC-10.mgc.mentorg.com with Microsoft SMTPSVC(6.0.3790.4675); Tue, 10 Jan 2012 03:13:33 -0800 Received: from [0.0.0.0] ([172.16.63.104]) by NA1-MAIL.mgc.mentorg.com with Microsoft SMTPSVC(6.0.3790.4675); Tue, 10 Jan 2012 03:13:52 -0800 Message-ID: <4F0C1D39.7050709@mentor.com> Date: Tue, 10 Jan 2012 14:51:00 -0000 From: Luis Machado Reply-To: "Gustavo, Luis" User-Agent: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.2.23) Gecko/20110922 Lightning/1.0b2 Thunderbird/3.1.15 MIME-Version: 1.0 To: Tom Tromey 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: 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/msg00291.txt.bz2 Hi Tom, On 01/06/2012 07:44 PM, Tom Tromey wrote: > Luis> I ran the testsuite and had no additional failures. > > I think the patches should include some tests for the new feature. Yes. I still need to think about those. I'll probably add a few GDB-side tests for safety, but mostly target-side ones like the tracepoint tests. > Luis> I'd like to hear about the change to "info break" and also the way we > Luis> should pass conditions down to the stub. > > I looked at this code and, while I think the output bits are fine, there > is something I don't understand: > > + /* 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) > > (First, there should be parens around that last subexpression.) Fixed. > If the user changes 'condition-evaluation', then the new setting might > affect what "info break" prints. But this seems weird, since what is > relevant is what is actually going on under the hood. > > Or, on the other hand, changing 'condition-evaluation' should change the > state of all breakpoints; but this requires additional code AFAICT. This has been addressed according to Eli's and Pedro's comments. If the user switches to a different mode, we will take appropriate action. Either we remove conditions or we add them to the target. > Luis> +int remote_supports_cond_breakpoints (void); > > Is it much uglier to rearrange things so no forward declaration is > needed? In this case, unfortunately, i think so. I'd have to move the code that adds the conditions to the packet buffer to the far bottom of the file. If keeping similar functions apart is no issue, i can do this change. > Luis> + /* List of conditions the target should evaluate if it supports target-side > Luis> + breakpoint conditions. */ > Luis> + struct condition_list *cond_list; > > It seems that the callee must free these. I think that should be > documented here. This should be addressed now with the use of VEC's. We have a vector of agent expressions. > Luis> + /* Conditional expression in agent expression > Luis> + bytecode form. This is used for stub-side breakpoint > Luis> + condition evaluation. */ > Luis> + struct agent_expr *cond_bytecode; > Luis> + > Luis> + /* Signals that the condition has changed since the last time > Luis> + we updated the global location list. This means the condition > Luis> + needs to be sent to the target again. This is used together > Luis> + with target-side breakpoint conditions. */ > Luis> + int condition_changed; > Luis> + > Luis> + /* Signals that breakpoint conditions need to be re-synched with the > Luis> + target. This has no use other than target-side breakpoints. */ > Luis> + int needs_update; > > I still wish we had 'bool'. Maybe this year. > > pahole says there are holes in bp_location on my x86-64 box. I think it > is somewhat preferable to fill the holes and to follow existing > practice, that is, use 'char' for boolean fields here. > > My preference would be to use 'unsigned int : 1' for boolean fields, but > I don't know how others feel about this. I'm fine with either of those. I'll switch to char meanwhile, but will use bitfields if OK. Thanks! I'll send an updated patch soon, cc-ing everyone. Luis lgustavo@codesourcery.com