From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 10935 invoked by alias); 14 May 2008 18:27:28 -0000 Received: (qmail 10905 invoked by uid 22791); 14 May 2008 18:27:27 -0000 X-Spam-Check-By: sourceware.org Received: from main.gmane.org (HELO ciao.gmane.org) (80.91.229.2) by sourceware.org (qpsmtpd/0.31) with ESMTP; Wed, 14 May 2008 18:27:01 +0000 Received: from list by ciao.gmane.org with local (Exim 4.43) id 1JwLfz-0000Wv-7y for gdb-patches@sources.redhat.com; Wed, 14 May 2008 18:26:07 +0000 Received: from 78.158.192.230 ([78.158.192.230]) by main.gmane.org with esmtp (Gmexim 0.1 (Debian)) id 1AlnuQ-0007hv-00 for ; Wed, 14 May 2008 18:26:07 +0000 Received: from vladimir by 78.158.192.230 with local (Gmexim 0.1 (Debian)) id 1AlnuQ-0007hv-00 for ; Wed, 14 May 2008 18:26:07 +0000 To: gdb-patches@sources.redhat.com From: Vladimir Prus Subject: Re: Patch for bug2457 Date: Wed, 14 May 2008 21:59:00 -0000 Message-ID: References: <578d90ee0805141109y1b768c67jc64828923162cea9@mail.gmail.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8Bit User-Agent: KNode/0.10.5 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: 2008-05/txt/msg00443.txt.bz2 > Hi, > > Here's a simple patch for bug2457 (Incorrect handling of erroneous > breakpoint conditions). > > It just postpones the update of the breakpoint condition until it has > been parsed successfully. > > If there is a parsing error, the condition is set to NULL. > > Best Regards, > /fc > gdb-bug2457.patch > ### Eclipse Workspace Patch 1.0 > #P gdb > Index: breakpoint.c > =================================================================== > RCS file: /cvs/src/src/gdb/breakpoint.c,v > retrieving revision 1.321 > diff -u -r1.321 breakpoint.c > --- breakpoint.c        4 May 2008 19:38:59 -0000       1.321 > +++ breakpoint.c        14 May 2008 17:32:06 -0000 > @@ -596,11 +596,13 @@ >               } >           } >         if (b->cond_string != NULL) > -         xfree (b->cond_string); > +         { > +           xfree (b->cond_string); > +           b->cond_string = NULL;      /* Keep in sync */ > +         } Is this actually what we want? If we want the breakpoint condition to be unchanged on error, then we should not clear b->cond_string here. >   >         if (*p == 0) >           { > -           b->cond_string = NULL; >             if (from_tty) >               printf_filtered (_("Breakpoint %d now unconditional.\n"), bnum); >           } > @@ -609,7 +611,6 @@ >             arg = p; >             /* I don't know if it matters whether this is the string the user >                typed in or the decompiled expression.  */ > -           b->cond_string = savestring (arg, strlen (arg)); >             b->condition_not_parsed = 0; >             for (loc = b->loc; loc; loc = loc->next) >               { > @@ -619,6 +620,9 @@ >                 if (*arg) >                   error (_("Junk at end of expression")); >               } > +           /* If we get here, the condition was parsed successfully and > +           no exception was thrown. See bug 2457. */ > +           b->cond_string = savestring (p, strlen (p)); Presumably, if an exception is thrown, then we should undo the changes to breakpoint location's 'cond' fields that we did above? Alternatively, we can: 1. Re-evaluate new condition string at all locations, and store the results in a vector 2. If successfull, store new expressions in loc->cond, foreach loc. 3. Update b->cond_string That would be actually "strong exception safety" in C++ terms. There are some technical issues with the patch: 1. There should be a changelog entry 2. Comments should be meaningful on their own, so I think "See bug 2457" is not good -- there should be explanation that we want exception-safety. 3. Comment like "Keep in sync" also does not say anything. Thanks, Volodya >           } >         breakpoints_changed (); >         breakpoint_modify_event (b->number);