From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 27317 invoked by alias); 16 Apr 2008 21:34:13 -0000 Received: (qmail 27308 invoked by uid 22791); 16 Apr 2008 21:34:12 -0000 X-Spam-Check-By: sourceware.org Received: from rock.gnat.com (HELO rock.gnat.com) (205.232.38.15) by sourceware.org (qpsmtpd/0.31) with ESMTP; Wed, 16 Apr 2008 21:33:54 +0000 Received: from localhost (localhost.localdomain [127.0.0.1]) by filtered-rock.gnat.com (Postfix) with ESMTP id CA66D2A9FAB; Wed, 16 Apr 2008 17:33:52 -0400 (EDT) Received: from rock.gnat.com ([127.0.0.1]) by localhost (rock.gnat.com [127.0.0.1]) (amavisd-new, port 10024) with LMTP id cXKx4IlD4vce; Wed, 16 Apr 2008 17:33:52 -0400 (EDT) Received: from joel.gnat.com (localhost.localdomain [127.0.0.1]) by rock.gnat.com (Postfix) with ESMTP id 8E7FB2A9FA4; Wed, 16 Apr 2008 17:33:52 -0400 (EDT) Received: by joel.gnat.com (Postfix, from userid 1000) id 872A9E7ACD; Wed, 16 Apr 2008 14:33:50 -0700 (PDT) Date: Wed, 16 Apr 2008 22:15:00 -0000 From: Joel Brobecker To: Marc Khouzam Cc: gdb-patches@sourceware.org Subject: Re: [Patch] Watchpoint condition fix Message-ID: <20080416213350.GC3626@adacore.com> References: <6D19CA8D71C89C43A057926FE0D4ADAA04E1BCD0@ecamlmw720.eamcs.ericsson.se> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <6D19CA8D71C89C43A057926FE0D4ADAA04E1BCD0@ecamlmw720.eamcs.ericsson.se> User-Agent: Mutt/1.4.2.2i 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-04/txt/msg00311.txt.bz2 > Watchpoint conditions are broken because they get deleted when a > watchpoint gets re-inserted. When watchpoints get re-inserted, the > condition string should be always reparsed. I agree with you and Vladimir. > @@ -986,7 +986,7 @@ > value_free (v); > } > > - if (reparse && b->cond_string != NULL) > + if (b->cond_string != NULL) This part of the change is OK, but I would like to see a comment explaining why we have to re-evaluate the condition regardless of the value of reparse. Vladimir's explaination on the gdb@ mailing list pretty much covers it. > { > char *s = b->cond_string; > if (b->loc->cond) Also, you can remove the block of code that checks b->loc->cond and frees when non-NULL, since we've just re-created the bplocs earlier and we know that their cond field is NULL. As a result, the temporary variable "s" shouldn't be necessary either. As an aside, but this is not your doing, I think we have a memory leak. We appear to be doing an xfree of each bp_location of our watchpoint, but instead we should call free_bp_location (). Vladimir, do you agree? (I will take care of doing that if you agree) Thank you, -- Joel