From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 8044 invoked by alias); 24 Apr 2013 14:55:28 -0000 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 Received: (qmail 8031 invoked by uid 89); 24 Apr 2013 14:55:28 -0000 X-Spam-SWARE-Status: No, score=-7.1 required=5.0 tests=AWL,BAYES_00,RCVD_IN_HOSTKARMA_W,RCVD_IN_HOSTKARMA_WL,RP_MATCHES_RCVD,SPF_HELO_PASS,SPF_PASS,TW_BJ,TW_RG autolearn=ham version=3.3.1 Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.84/v0.84-167-ge50287c) with ESMTP; Wed, 24 Apr 2013 14:55:27 +0000 Received: from int-mx12.intmail.prod.int.phx2.redhat.com (int-mx12.intmail.prod.int.phx2.redhat.com [10.5.11.25]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id r3OEtPeT018431 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Wed, 24 Apr 2013 10:55:25 -0400 Received: from barimba (ovpn01.gateway.prod.ext.phx2.redhat.com [10.5.9.1]) by int-mx12.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id r3OEtNJI020951 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES128-SHA bits=128 verify=NO); Wed, 24 Apr 2013 10:55:24 -0400 From: Tom Tromey To: Nicolas Blanc Cc: gdb-patches@sourceware.org Subject: Re: [PATCH 1/3] Added command remove-symbol-file. References: <1366098721-18302-1-git-send-email-nicolas.blanc@intel.com> <1366098721-18302-2-git-send-email-nicolas.blanc@intel.com> Date: Wed, 24 Apr 2013 18:54:00 -0000 In-Reply-To: <1366098721-18302-2-git-send-email-nicolas.blanc@intel.com> (Nicolas Blanc's message of "Tue, 16 Apr 2013 09:51:58 +0200") Message-ID: <87fvygnfl0.fsf@fleche.redhat.com> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/24.3 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain X-SW-Source: 2013-04/txt/msg00741.txt.bz2 >>>>> "Nicolas" == Nicolas Blanc writes: Nicolas> +/* Disable any breakpoints and tracepoints that are in SOLIB upon Nicolas> + notification of UNLOADED_SHLIB. Only apply to enabled breakpoints, Two periods after space -- Yao pointed out one instance of this, but there are a couple. Nicolas> + ALL_OBJFILE_OSECTIONS (objfile, osect) Nicolas> + { Nicolas> + if (obj_section_addr (osect) <= loc_addr Nicolas> + && loc_addr < obj_section_endaddr (osect)) Nicolas> + { Nicolas> + loc->shlib_disabled = 1; Nicolas> + loc->inserted = 0; Nicolas> + observer_notify_breakpoint_modified (loc->owner); Nicolas> + break; Nicolas> + } I liked Yao's comment about notifying just once per breakpoint. It doesn't seem difficult to do that. It's curious that this new function and breakpoint_free_objfile don't overlap at all. Nicolas> - observer_attach_solib_unloaded (clear_dangling_display_expressions); Nicolas> + observer_attach_free_objfile (clear_dangling_display_expressions); It isn't totally clear to me that these are equivalent. >From browsing solib.c it seems that an solib can be freed without the objfiles being purged. I am not sure but I think maybe the path core_close -> clear_solib can do this. If so, maybe this is simply a bug there; I am not sure. Nicolas> +static void remove_symbol_file_command (char *, int); I don't think you really need this declaration. It's better to leave them out unless they are needed for something specific, like mutual recursion. (There are lots of leftover ones, which I've always imagined to be from a time when GCC's prototype warnings worked differently.) Nicolas> + addr_low = parse_and_eval_address (arg); It seems like this parses and evaluates this argument twice. I think it would be better to take another approach, so that it is just evaluated once. Nicolas> + /* Set the low address of the object for identification. */ Nicolas> + objf->addr_low = addr_low; Is there any way to display this information? Otherwise, how should users find it? Nicolas> +static void Nicolas> +remove_symbol_file_command (char *args, int from_tty) [...] Nicolas> + argv = gdb_buildargv (args); Nicolas> + make_cleanup_freeargv (argv); If the syntax is really just an expression as an argument, then don't bother with gdb_buildargv. Using gdb_buildargv means that complicated expressions will have to be quoted unusually. I realize this is how add-symbol-file works, but I think it is a bad approach. Nicolas> + if (!objf) We recently decided to use the wordier "objf != NULL" style. Nicolas> + c = add_cmd ("remove-symbol-file", class_files, remove_symbol_file_command, _("\ I think this line is too long. Tom