From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 6310 invoked by alias); 25 Jun 2010 21:57:05 -0000 Received: (qmail 6116 invoked by uid 22791); 25 Jun 2010 21:57:03 -0000 X-SWARE-Spam-Status: No, hits=-5.9 required=5.0 tests=AWL,BAYES_00,RCVD_IN_DNSWL_HI,SPF_HELO_PASS,TW_BJ,TW_XD,T_RP_MATCHES_RCVD X-Spam-Check-By: sourceware.org Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Fri, 25 Jun 2010 21:56:56 +0000 Received: from int-mx01.intmail.prod.int.phx2.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) by mx1.redhat.com (8.13.8/8.13.8) with ESMTP id o5PLusl9007298 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Fri, 25 Jun 2010 17:56:54 -0400 Received: from ns3.rdu.redhat.com (ns3.rdu.redhat.com [10.11.255.199]) by int-mx01.intmail.prod.int.phx2.redhat.com (8.13.8/8.13.8) with ESMTP id o5PLuse2014612; Fri, 25 Jun 2010 17:56:54 -0400 Received: from opsy.redhat.com (ovpn01.gateway.prod.ext.phx2.redhat.com [10.5.9.1]) by ns3.rdu.redhat.com (8.13.8/8.13.8) with ESMTP id o5PLurK4005933; Fri, 25 Jun 2010 17:56:53 -0400 Received: by opsy.redhat.com (Postfix, from userid 500) id 379EA3792DB; Fri, 25 Jun 2010 15:56:53 -0600 (MDT) From: Tom Tromey To: Justin Lebar Cc: gdb-patches@sourceware.org Subject: Re: [Patch] Bug 8287: Skip uninteresting functions while debugging References: Reply-To: tromey@redhat.com Date: Fri, 25 Jun 2010 21:57:00 -0000 In-Reply-To: (Justin Lebar's message of "Fri, 18 Jun 2010 11:55:46 -0700") Message-ID: User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/23.1 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii 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-06/txt/msg00604.txt.bz2 >>>>> "Justin" == Justin Lebar writes: Justin> This adds support for a "blacklist" which contains files and Justin> functions which are skipped while single-stepping. Thanks for doing this. FWIW I think this is a very good first patch to gdb. One concern of mine is that this code will not perform well with many blacklists in place. It may make sense to keep a hash table indexed by name (function or file), to make lookups faster. What do you think? Another random thought is whether we want to be able to blacklist based on objfile name. Justin> This patch also fixes bug 11614: decode_variable() in linespec.c Justin> does not obey its contract It is somewhat preferred in gdb to submit things like this as separate patches. Now for the nits ... Justin> +#define ALL_BLACKLIST_ENTRIES(B) for (B = blacklist_entry_chain; B; B = B->next) I think this should be wrapped before the 'for'. Justin> + b->filename = strdup(filename); Space before open paren. Also, you must use xstrdup, not strdup. This occurs more than once. Justin> + if (sals.nelts == 0) Justin> + error (_("No function to blacklist.")); /* TODO can I trigger this? */ Justin> + if (sals.nelts > 1) Justin> + error (_("Specify just one function at a time.")); /* TODO can I Justin> trigger this? */ Good questions :-) It is great to be defensive here, but the comments should go before checkin. Maybe you can get nelts>1 with a C++ destructor? I'm not sure. Justin> + if (opts.addressprint) Justin> + { Justin> + if (b->pc != 0) Justin> + ui_out_field_core_addr (uiout, "addr", b->gdbarch, b->pc); /* 4 */ Justin> + else Justin> + ui_out_field_string (uiout, "addr", "n/a"); /* 4 */ Justin> + } A small nit, but I'm not sure about the exact string "n/a" here. If there is an analogous situation elsewhere it would be good to just do whatever is done there. Justin> +int Justin> +function_pc_is_blacklisted (CORE_ADDR pc) All functions should have an introductory comment explaining its contract -- purpose and meaning of arguments and result. For functions implementing commands or other helpers, this can be pretty short. Justin> + /* First, check whether the file or function this entry is pending on has Justin> + been loaded. It might be more sensible to do this on a solib load, Justin> + but that doesn't seem to work for some reason. */ Let's figure this out. Also, since the blacklist includes a PC value, I think you have to reset it when re-running the inferior, and on some other state changes as well. Justin> + add_cmd ("enable", class_blacklist, blacklist_enable_command, _("\ Justin> +Enable a blacklist entry.\n\ Justin> +blacklist enable [NUMBER]"), Justin> + &blacklistlist); I think it would be more usual to make the commands "enable blacklist ..." instead of "blacklist enable ...". Likewise for disable and delete. Justin> --- /dev/null Justin> +++ b/gdb/blacklist.h [...] Justin> +int Justin> +function_pc_is_blacklisted (CORE_ADDR pc); No newline after "int" in a declaration. A header file should have a #if guard. Justin> -int default_breakpoint_valid; Justin> -CORE_ADDR default_breakpoint_address; Justin> -struct symtab *default_breakpoint_symtab; Justin> -int default_breakpoint_line; Justin> -struct program_space *default_breakpoint_pspace; I'm not sure I understand the rationale for this set of changes. Is it just to make the naming more clear? I do like the new, more opaque, approach. Justin> - class_pseudo, class_tui, class_user, class_xdb Justin> + class_pseudo, class_tui, class_user, class_xdb, class_blacklist I don't think you need a new command class for this. I think class_breakpoint might be fine. Tom