From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 12507 invoked by alias); 23 Dec 2010 20:17:22 -0000 Received: (qmail 12494 invoked by uid 22791); 23 Dec 2010 20:17:21 -0000 X-SWARE-Spam-Status: No, hits=-1.9 required=5.0 tests=AWL,BAYES_00,T_RP_MATCHES_RCVD X-Spam-Check-By: sourceware.org Received: from mail.codesourcery.com (HELO mail.codesourcery.com) (38.113.113.100) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Thu, 23 Dec 2010 20:17:16 +0000 Received: (qmail 30096 invoked from network); 23 Dec 2010 20:17:14 -0000 Received: from unknown (HELO orlando.localnet) (pedro@127.0.0.2) by mail.codesourcery.com with ESMTPA; 23 Dec 2010 20:17:14 -0000 From: Pedro Alves To: gdb-patches@sourceware.org Subject: Re: [patch 2/3] Implement support for PowerPC BookE ranged watchpoints Date: Thu, 23 Dec 2010 22:18:00 -0000 User-Agent: KMail/1.13.5 (Linux/2.6.33-29-realtime; KDE/4.4.5; x86_64; ; ) Cc: Thiago Jung Bauermann , Jan Kratochvil , Joel Brobecker , Eli Zaretskii References: <1290549100.3164.47.camel@hactar> <201011271747.39053.pedro@codesourcery.com> <1293130182.14239.21.camel@hactar> In-Reply-To: <1293130182.14239.21.camel@hactar> MIME-Version: 1.0 Content-Type: Text/Plain; charset="utf-8" Content-Transfer-Encoding: 7bit Message-Id: <201012232017.11120.pedro@codesourcery.com> 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: 2010-12/txt/msg00445.txt.bz2 I think this looks great. Thanks a million for following through! The resource accounting bits, I still say that they should all just go away (at some point), and gdb should just try to insert the watchpoint immediately, and see if the target refuses. E.g., how could one sanely implement the accounting for remote targets? The target is free to do all sorts of smart merging, and resource reusing, and in fact, x86 gdbserver does so (which is why the x86 and most other ports don't actually make use of the resource accounting interfaces, they just always accept watchpoints). A few directed comments only: On Thursday 23 December 2010 18:49:42, Thiago Jung Bauermann wrote: > +int > +is_scalar_type_recursive (struct type *t) > +{ > + CHECK_TYPEDEF (t); > + > + if (is_scalar_type (t)) > + return 1; > + else if ((TYPE_CODE (t) == TYPE_CODE_ARRAY > + || TYPE_CODE (t) == TYPE_CODE_STRING) && TYPE_NFIELDS (t) == 1 > + && TYPE_CODE (TYPE_INDEX_TYPE (t)) == TYPE_CODE_RANGE) > + { > + LONGEST low_bound, high_bound; > + struct type *elt_type = check_typedef (TYPE_TARGET_TYPE (t)); > + > + get_discrete_bounds (TYPE_INDEX_TYPE (t), &low_bound, &high_bound); > + > + return high_bound == low_bound && is_scalar_type_recursive (elt_type); > + } What does the "TYPE_NFIELDS (t) == 1" do ? I think you're missing at least pointers and references. These should also get the "exact" treatment, no? Take a look at valprint.c:scalar_type_p. Could you please add small describing comments above these new functions? ("return true if type T is a blah, blah".) > +static void > +show_powerpc_exact_watchpoints (struct ui_file *file, int from_tty, > + struct cmd_list_element *c, > + const char *value) > +{ > + fprintf_filtered (file, _("\ > +Use of just one debug register per watchpoint is %s.\n"), value); > +} > + Should be "Use of exact watchpoints is ...", I think? Thus getting rid of the small lie that not all watchpoints (in the user perspective) will use just one debug register, leaving the just one debug register issue explanation to the help string (as you already have). -- Pedro Alves