From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 3083 invoked by alias); 23 Jul 2011 02:40:04 -0000 Received: (qmail 3072 invoked by uid 22791); 23 Jul 2011 02:40:03 -0000 X-SWARE-Spam-Status: No, hits=-2.3 required=5.0 tests=AWL,BAYES_00,RP_MATCHES_RCVD X-Spam-Check-By: sourceware.org Received: from e24smtp02.br.ibm.com (HELO e24smtp02.br.ibm.com) (32.104.18.86) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Sat, 23 Jul 2011 02:39:49 +0000 Received: from d24relay01.br.ibm.com (d24relay01.br.ibm.com [9.8.31.16]) by e24smtp02.br.ibm.com (8.14.4/8.13.1) with ESMTP id p6N3p4nE013574 for ; Sat, 23 Jul 2011 00:51:04 -0300 Received: from d24av05.br.ibm.com (d24av05.br.ibm.com [9.18.232.44]) by d24relay01.br.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id p6N2a6BJ4976758 for ; Fri, 22 Jul 2011 23:36:06 -0300 Received: from d24av05.br.ibm.com (loopback [127.0.0.1]) by d24av05.br.ibm.com (8.14.4/8.13.1/NCO v10.0 AVout) with ESMTP id p6N2deIL013354 for ; Fri, 22 Jul 2011 23:39:40 -0300 Received: from [9.12.227.43] ([9.12.227.43]) by d24av05.br.ibm.com (8.14.4/8.13.1/NCO v10.0 AVin) with ESMTP id p6N2dbkA013329; Fri, 22 Jul 2011 23:39:38 -0300 Subject: Re: x86 watchpoints bug (Re: ping: Re: PATCH : allow to set length of hw watchpoints (e.g. for Valgrind gdbserver)) From: Thiago Jung Bauermann To: Pedro Alves Cc: gdb-patches ml , Philippe Waroquiers , yao@codesourcery.com In-Reply-To: <201107221740.07110.pedro@codesourcery.com> References: <201107211712.26443.pedro@codesourcery.com> <201107221740.07110.pedro@codesourcery.com> Content-Type: text/plain; charset="UTF-8" Date: Sat, 23 Jul 2011 16:28:00 -0000 Message-ID: <1311388735.3205.29.camel@hactar> Mime-Version: 1.0 Content-Transfer-Encoding: 7bit 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: 2011-07/txt/msg00650.txt.bz2 On Fri, 2011-07-22 at 17:40 +0100, Pedro Alves wrote: > On Friday 22 July 2011 17:02:42, Philippe Waroquiers wrote: > int > works_in_software_mode_watchpoint (const struct breakpoint *b) > { > return b->type == bp_hardware_watchpoint; > } > > (top-gdb) p b->type > $5 = bp_watchpoint > > From the error string, looks like the check should be something like: > > else if (b->type == bp_read_watchpoint > || b->type == bp_access_watchpoint) > error (_("Expression cannot be implemented with " > "read/access watchpoint.")); > > instead, as those watchpoints can't indeed be implemented > as software watchpoints. Though the intention may have > been to catch something about masked watchpoints. Yes, that was indeed the intention. And I agree that the error string is wrong when it is shown for a masked watchpoint (which can happen if can-use-hw-watchpoints is 0). > Maybe better would be to change works_in_software_mode_watchpoint to: > > int > works_in_software_mode_watchpoint (const struct breakpoint *b) > { > - return b->type == bp_hardware_watchpoint; > + return (b->type == bp_watchpoint || b->type == bp_hardware_watchpoint); > } Agreed. I would only comment that the parenthesis are not necessary. :-) Theoretically resources_needed_watchpoint would have to be adapted for software watchpoints too, but in practice that function is only called in hw_watchpoint_used_count, which is never called with bp_watchpoint as an argument. FWIW, my local branch with my rework of debug registers accounting doesn't have hw_watchpoint_used_count anymore. > The error string could also be enhanced to include the real > watchpoint type (so a user of masked watchpoints doesn't get > confused). I tried to keep that code agnostic to the type of watchpoint at hand (hence the breakpoint_ops methods), so what about a more generic error message, like "There is no hardware debug support for this watchpoint." or "Expression cannot be implemented with hardware debug resources."? Otherwise, we could use something like: else if (b->type == bp_read_watchpoint || b->type == bp_access_watchpoint) error (_("Expression cannot be implemented with " "read/access watchpoint.")); else if (is_masked_watchpoint (b)) error (_("Expression cannot be implemented with masked watchpoint.")); else if (b->ops && b->ops->works_in_software_mode && !b->ops->works_in_software_mode (b)) error (_("Expression cannot be implemented with this type of watchpoint.")); else b->type = bp_watchpoint; The last else if is currently dead code, since only regular watchpoints and masked watchpoints implement the works_in_software_mode method. So either it or the one above it could be dropped. Or the last one could replace all the else ifs above it. I don't have a strong opinion on this one. Pick what you think is more reasonable. -- []'s Thiago Jung Bauermann IBM Linux Technology Center