From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 21997 invoked by alias); 6 Jan 2012 21:44:49 -0000 Received: (qmail 21987 invoked by uid 22791); 6 Jan 2012 21:44:48 -0000 X-SWARE-Spam-Status: No, hits=-7.0 required=5.0 tests=AWL,BAYES_00,RCVD_IN_DNSWL_HI,SPF_HELO_PASS,T_RP_MATCHES_RCVD X-Spam-Check-By: sourceware.org Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Fri, 06 Jan 2012 21:44:35 +0000 Received: from int-mx12.intmail.prod.int.phx2.redhat.com (int-mx12.intmail.prod.int.phx2.redhat.com [10.5.11.25]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id q06LiVDI010257 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Fri, 6 Jan 2012 16:44:31 -0500 Received: from ns3.rdu.redhat.com (ns3.rdu.redhat.com [10.11.255.199]) by int-mx12.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id q06LiVjg010439; Fri, 6 Jan 2012 16:44:31 -0500 Received: from barimba (ovpn01.gateway.prod.ext.phx2.redhat.com [10.5.9.1]) by ns3.rdu.redhat.com (8.13.8/8.13.8) with ESMTP id q06LiT1U032248; Fri, 6 Jan 2012 16:44:30 -0500 From: Tom Tromey 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> Date: Fri, 06 Jan 2012 21:44:00 -0000 In-Reply-To: <4F05BA10.3090107@mentor.com> (Luis Machado's message of "Thu, 05 Jan 2012 12:56:16 -0200") Message-ID: User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/24.0.92 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain 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/msg00250.txt.bz2 >>>>> "Luis" == Luis Machado writes: Luis> This patch handles GDB-specific changes to support stub-side Luis> breakpoint conditions. Nice series, I quite like it. Luis> I ran the testsuite and had no additional failures. I think the patches should include some tests for the new feature. 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.) 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. Luis> +int remote_supports_cond_breakpoints (void); Is it much uglier to rearrange things so no forward declaration is needed? 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. 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. Tom