From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 11798 invoked by alias); 26 Mar 2013 01:05:43 -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 11731 invoked by uid 89); 26 Mar 2013 01:05:35 -0000 Received: from mail-ob0-f182.google.com (HELO mail-ob0-f182.google.com) (209.85.214.182) by sourceware.org (qpsmtpd/0.84/v0.84-167-ge50287c) with ESMTP; Tue, 26 Mar 2013 01:05:35 +0000 Received: by mail-ob0-f182.google.com with SMTP id ef5so4002705obb.13 for ; Mon, 25 Mar 2013 18:05:34 -0700 (PDT) X-Received: by 10.182.72.5 with SMTP id z5mr1349672obu.24.1364259934467; Mon, 25 Mar 2013 18:05:34 -0700 (PDT) MIME-Version: 1.0 Received: by 10.60.13.133 with HTTP; Mon, 25 Mar 2013 18:04:54 -0700 (PDT) In-Reply-To: <515092F2.2000307@redhat.com> References: <514E8D6C.2010606@mentor.com> <514EEB43.6040101@redhat.com> <87620ftn9f.fsf@fleche.redhat.com> <515092F2.2000307@redhat.com> From: Hui Zhu Date: Tue, 26 Mar 2013 11:16:00 -0000 Message-ID: Subject: Re: [PATCH] Fix create pending breakpoint handle extra_string issue if not parse_condition_and_thread To: Keith Seitz Cc: Hui Zhu , Tom Tromey , gdb-patches ml Content-Type: text/plain; charset=ISO-8859-1 X-SW-Source: 2013-03/txt/msg00958.txt.bz2 On Tue, Mar 26, 2013 at 2:09 AM, Keith Seitz wrote: > On 03/25/2013 10:14 AM, Tom Tromey wrote: >>>>>>> >>>>>>> "Hui" == Hui Zhu writes: >> >> >> Hui> I am sorry that what you care about is the issue that affect the mi. >> Hui> But my patch is for the issue inside the function create_breakpoint. > > > Actually I /was/ talking about create_breakpoint. As you stated, the only > way to demonstrate the problem is via MI, so that's what I used to > demonstrate how I think the situation should be handled. > > Here's a patch which does exactly what I consider the "right" way to react > to having both cond_string and a condition inside arg: This function have parse_condition_and_thread. It already choice which part it will get the cond_string from, why we still need this check? Also it didn't handle this pending breakpoints. > > Index: breakpoint.c > =================================================================== > RCS file: /cvs/src/src/gdb/breakpoint.c,v > retrieving revision 1.747 > diff -u -p -r1.747 breakpoint.c > --- breakpoint.c 20 Mar 2013 22:17:18 -0000 1.747 > +++ breakpoint.c 25 Mar 2013 17:59:36 -0000 > @@ -9659,6 +9659,11 @@ create_breakpoint (struct gdbarch *gdbar > extra_string = xstrdup (extra_string); > make_cleanup (xfree, extra_string); > } > + else if (*arg != '\000') > + { > + extra_string = xstrdup (arg); > > + make_cleanup (xfree, extra_string); > + } > } > > ops->create_breakpoints_sal (gdbarch, &canonical, lsal, > > >> In this case, it seems to me that the API must be a bad one. > > > Yes, that API extension was a horribly implemented (quick and dirty), but > create_breakpoint is a bit of a mess, since it not only has to deal with > setting breakpoints (of various varieties), it also has to deal with parsing > user input. I'm not a fan of this (too common) paradigm. > And if we want add comments on this function to let people don't do that. We need also tell they, the pending breakpoints's extra_string will be dropped. > >> Can't we just tell callers, "don't do that"? >> To me it seems like a pathological case. > > > We can certainly enforce this, as my patchlet above demonstrates: > > > -break-insert -c "argc > 1" "main if argc > 2" > ^error,msg="Garbage 'if argc > 2' at end of command" > > Keith