From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 16020 invoked by alias); 21 Dec 2008 13:11:47 -0000 Received: (qmail 16010 invoked by uid 22791); 21 Dec 2008 13:11:46 -0000 X-SWARE-Spam-Status: No, hits=-2.1 required=5.0 tests=AWL,BAYES_00,J_CHICKENPOX_42 X-Spam-Check-By: sourceware.org Received: from rock.gnat.com (HELO rock.gnat.com) (205.232.38.15) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Sun, 21 Dec 2008 13:10:47 +0000 Received: from localhost (localhost.localdomain [127.0.0.1]) by filtered-rock.gnat.com (Postfix) with ESMTP id 8FE722A9614; Sun, 21 Dec 2008 08:10:45 -0500 (EST) 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 EUxUUqO1zuMJ; Sun, 21 Dec 2008 08:10:45 -0500 (EST) Received: from joel.gnat.com (localhost.localdomain [127.0.0.1]) by rock.gnat.com (Postfix) with ESMTP id 746572A9610; Sun, 21 Dec 2008 08:10:44 -0500 (EST) Received: by joel.gnat.com (Postfix, from userid 1000) id E776FE7ACD; Sun, 21 Dec 2008 17:10:36 +0400 (RET) Date: Sun, 21 Dec 2008 13:11:00 -0000 From: Joel Brobecker To: Emi SUZUKI Cc: gdb-patches@sourceware.org Subject: Re: Watchpoint on an unloaded shared library(1) Message-ID: <20081221131036.GB25416@adacore.com> References: <20081120.210657.01365938.emi-suzuki@tjsys.co.jp> <20081213150517.GE6866@adacore.com> <20081216.211520.207584606.emi-suzuki@tjsys.co.jp> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20081216.211520.207584606.emi-suzuki@tjsys.co.jp> 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-12/txt/msg00362.txt.bz2 > > How about replace all the code that's inside > > > > if (bpt->type == bp_watchpoint || > > bpt->type == bp_hardware_watchpoint || > > bpt->type == bp_read_watchpoint || > > bpt->type == bp_access_watchpoint) > > { > > [re-create a lot of stuff for our watchpoint...] > > > > by a call to update_watchpoint (bpt, 1)? Would that work in your case? > > It works without merging the missing code each other, as long as we > don't have to care the hardware watchpoint resource count here: if the > user sets other watchpoints while the disabled hardware watchpoints > exist, re-enabling the disabled ones might fail out of the shortage of > resources. That's a good point. I still think this might work. I am hanging on to the idea because I'm seeing a few cases in this file where we have two pieces of code that are almost the same and yet not quite the same. I really dislike this code semi-duplication. I'm also seeing potential disasters such as a reference to a free'ed value, which makes me want to share the (correct!) code more. So, what I think we should do, in this case, is call update_watchpoint within an exception wrapper. If we fail to enable the watchpoint, I think we want to print a message that's similar to the message printed when re-setting it, something like this: (gdb) enable 2 Cannot enable watchpoint 2: No symbol "sample_glob" in current context. Rather than just printing: (gdb) enable 2 No symbol "sample_glob" in current context. If the update_watchpoint completes without exception, then there are two cases: 1. The scope associated to the watchpoint no longer exists. update_watchpoint has therefore "deleted" it, which actually means has scheduled it for deletion at the next stop (by setting b->disposition to disp_del_at_next_stop). update_watchpoint has already printed an error message about it, so there isn't much else to do. 2. Everything went fine in terms of updating the watchpoint. We need to confirm that we still have enough hardware resources to effectively insert the watchpoint. So, in the end, the code should look like this: if (bpt->type == bp_watchpoint || bpt->type == bp_hardware_watchpoint || bpt->type == bp_read_watchpoint || bpt->type == bp_access_watchpoint) { struct gdb_exception e; TRY_CATCH (e, RETURN_MASK_ALL) { update_watchpoint (...); } if (e.reason < 0) { exception_fprintf (gdb_strerr, e, "Cannot enable watchpoint %d:", b->number); return; } else { [check hardware support here] if (target_resources_ok < 0) { printf_filtered (error_message); return; } } } Could you see if that works? -- Joel