From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 29974 invoked by alias); 10 Apr 2013 10:31:04 -0000 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 Received: (qmail 29964 invoked by uid 89); 10 Apr 2013 10:31:03 -0000 X-Spam-SWARE-Status: No, score=-4.2 required=5.0 tests=AWL,BAYES_00,KHOP_RCVD_UNTRUST,KHOP_THREADED,RCVD_IN_HOSTKARMA_W,RCVD_IN_HOSTKARMA_WL autolearn=ham version=3.3.1 Received: from usevmg20.ericsson.net (HELO usevmg20.ericsson.net) (198.24.6.45) by sourceware.org (qpsmtpd/0.84/v0.84-167-ge50287c) with ESMTP; Wed, 10 Apr 2013 10:31:03 +0000 Received: from EUSAAHC003.ericsson.se (Unknown_Domain [147.117.188.81]) by usevmg20.ericsson.net (Symantec Mail Security) with SMTP id 17.4E.02430.56F35615; Wed, 10 Apr 2013 12:31:01 +0200 (CEST) Received: from EUSAAMB110.ericsson.se ([147.117.188.127]) by EUSAAHC003.ericsson.se ([147.117.188.81]) with mapi id 14.02.0318.004; Wed, 10 Apr 2013 06:31:00 -0400 From: Marc Khouzam To: Pedro Alves , Hui Zhu CC: Eli Zaretskii , Hui Zhu , "gdb-patches@sourceware.org" Subject: RE: [PATCH] add -s option to make -break-insert support dprintf Date: Wed, 10 Apr 2013 19:44:00 -0000 Message-ID: References: <515451EA.1000200@mentor.com> <83y5d7wpvq.fsf@gnu.org> ,<516454DA.9040109@redhat.com> In-Reply-To: <516454DA.9040109@redhat.com> Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 X-SW-Source: 2013-04/txt/msg00273.txt.bz2 > ________________________________________ > From: gdb-patches-owner@sourceware.org [gdb-patches-owner@sourceware.org]= on behalf of Pedro Alves [palves@redhat.com] > Sent: April 9, 2013 1:50 PM > To: Hui Zhu > Cc: Eli Zaretskii; Hui Zhu; gdb-patches@sourceware.org; Marc Khouzam > Subject: Re: [PATCH] add -s option to make -break-insert support dprintf >=20 > Hi Hui, >=20 > Thanks for the patch. >=20 > New MI features need a NEWS entry. >=20 > On 03/29/2013 08:01 AM, Hui Zhu wrote: >=20 > > + if (hardware && dprintf) > > + error (_("-break-insert: -h and -s cannot be use together")); >=20 > "cannot be used" >=20 > > @@ -180,11 +189,14 @@ mi_cmd_break_insert (char *command, char > > regular non-jump based tracepoints. */ > > type_wanted =3D (tracepoint > > ? (hardware ? bp_fast_tracepoint : bp_tracepoint) > > - : (hardware ? bp_hardware_breakpoint : bp_breakpoint)); > > - ops =3D tracepoint ? &tracepoint_breakpoint_ops : &bkpt_breakpoint_o= ps; > > + : (hardware ? bp_hardware_breakpoint > > + : (dprintf ? bp_dprintf : bp_breakpoint))); > > + ops =3D tracepoint ? &tracepoint_breakpoint_ops > > + : (dprintf ? &dprintf_breakpoint_ops > > + : &bkpt_breakpoint_ops); >=20 > This is getting unnecessarily hard for humans to grok. Write > instead as (untested): >=20 > if (tracepoint) > { > /* move existing comment on fast tracepoints here */ > type_wanted =3D hardware ? bp_fast_tracepoint : bp_tracepoint; > ops =3D &tracepoint_breakpoint_ops; > } > else if (dprintf) > { > type_wanted =3D bp_dprintf; > ops =3D &dprintf_breakpoint_ops; > } > else > { > type_wanted =3D hardware ? bp_hardware_breakpoint : bp_breakpoint; > ops =3D &bkpt_breakpoint_ops; > } In the orginal patch, having both 'hardware' and 'dprintf' true would=20 create a hardware breakpoint (not dprintf), but would still set 'ops' to &dprintf_breakpoint_ops. This didn't look right to me. A side-effect of Pedro's change is that the hardware dprintf case will be handled properly. I think that is a good thing. However, I wanted to mention it, as I don't know if there are other changes needed to handle a hardware dprintf (or if it really should be allowed). I am allowed to create a hardware breakpoint with a printf condition, so I guess a hardware dprintf would make sense, but I'm not sure. Marc