From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 13758 invoked by alias); 12 Aug 2011 20:58:39 -0000 Received: (qmail 13750 invoked by uid 22791); 12 Aug 2011 20:58:38 -0000 X-SWARE-Spam-Status: No, hits=-7.3 required=5.0 tests=AWL,BAYES_00,RCVD_IN_DNSWL_HI,RP_MATCHES_RCVD,SPF_HELO_PASS,TW_BJ 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, 12 Aug 2011 20:58:20 +0000 Received: from int-mx02.intmail.prod.int.phx2.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id p7CKwIf9018562 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Fri, 12 Aug 2011 16:58:18 -0400 Received: from ns3.rdu.redhat.com (ns3.rdu.redhat.com [10.11.255.199]) by int-mx02.intmail.prod.int.phx2.redhat.com (8.13.8/8.13.8) with ESMTP id p7CKwIpa006461; Fri, 12 Aug 2011 16:58:18 -0400 Received: from barimba (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 p7CKwG8t004321; Fri, 12 Aug 2011 16:58:17 -0400 From: Tom Tromey To: Sanjoy Das Cc: gdb-patches@sourceware.org Subject: Re: [PATCH 3/7] New commands for loading and unloading a reader. References: <1312903509-25132-1-git-send-email-sanjoy@playingwithpointers.com> <1312903509-25132-4-git-send-email-sanjoy@playingwithpointers.com> Date: Fri, 12 Aug 2011 20:58:00 -0000 In-Reply-To: <1312903509-25132-4-git-send-email-sanjoy@playingwithpointers.com> (Sanjoy Das's message of "Tue, 9 Aug 2011 20:55:05 +0530") Message-ID: User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/24.0.50 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain 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-08/txt/msg00263.txt.bz2 >>>>> "Sanjoy" == Sanjoy Das writes: Sanjoy> Introduces two new GDB commands - `load-jit-reader' and Sanjoy> `unload-jit-reader'. I am not that fond of these command names. I'm not sure what else I would propose... maybe a "jit" prefix command, with "load" and "unload" subcommands? Do we actually need the ability to unload? Sanjoy> +#define GDB_READER_DIR (LIBDIR "/gdb/") This should go through the relocation process done for other directories. See main.c. Sanjoy> +/* One reader that has been loaded successfully, and can potentially be used to Sanjoy> + parse debug info. */ Sanjoy> +struct jit_reader Sanjoy> +{ Sanjoy> + struct gdb_reader_funcs *functions; Sanjoy> +} *loaded_jit_reader = NULL; Can this be static? Sanjoy> +typedef struct gdb_reader_funcs * (reader_init_fn_type) (void); Sanjoy> +const char *reader_init_fn_sym = "gdb_init_reader"; Likewise. Sanjoy> +/* Try to load FILE_NAME as a JIT debug info reader. Set ERROR_STRING Sanjoy> + in case of an error (and return NULL), else return a correcly Sanjoy> + formed struct jit_reader. */ Sanjoy> +static struct jit_reader * Sanjoy> +jit_reader_load (const char *file_name, char **error_string) It is more normal in gdb to simply call error when an error occurs. Then you don't need to worry about the return value, or passing in the error_string pointer. Sanjoy> + so = gdb_dlopen (file_name); Sanjoy> + Sanjoy> + if (!so) Sanjoy> + { Sanjoy> + *error_string = _("could not open reader file"); Sanjoy> + return NULL; Sanjoy> + } It might be nicer if gdb_dlopen called error, so that the different ports could give better error messages, say via dlerror. Sanjoy> + if (so) Sanjoy> + gdb_dlclose(so); With the error approach, you would install a cleanup that calls gdb_dlclose, then discard_cleanups at the point of normal return. Sanjoy> + strncat (so_name, args, PATH_MAX - sizeof(GDB_READER_DIR) - 4); Sanjoy> + strcat (so_name, ".so"); It seems to me that you want a directory separator in here. Sanjoy> + free (loaded_jit_reader); xfree. Sanjoy> @@ -510,10 +610,10 @@ jit_event_handler (struct gdbarch *gdbarch) Sanjoy> struct jit_code_entry code_entry; Sanjoy> CORE_ADDR entry_addr; Sanjoy> struct objfile *objf; Sanjoy> + struct jit_inferior_data *inf_data = get_jit_inferior_data (); Sanjoy> /* Read the descriptor from remote memory. */ Sanjoy> - jit_read_descriptor (gdbarch, &descriptor, Sanjoy> - get_jit_inferior_data ()->descriptor_addr); Sanjoy> + jit_read_descriptor (gdbarch, &descriptor, inf_data->descriptor_addr); Sanjoy> entry_addr = descriptor.relevant_entry; Sanjoy> /* Do the corresponding action. */ Sanjoy> @@ -526,15 +626,16 @@ jit_event_handler (struct gdbarch *gdbarch) Sanjoy> jit_register_code (gdbarch, entry_addr, &code_entry); Sanjoy> break; Sanjoy> case JIT_UNREGISTER: Sanjoy> - objf = jit_find_objf_with_entry_addr (entry_addr); Sanjoy> - if (objf == NULL) Sanjoy> - printf_unfiltered (_("Unable to find JITed code " Sanjoy> - "entry at address: %s\n"), Sanjoy> - paddress (gdbarch, entry_addr)); Sanjoy> - else Sanjoy> - jit_unregister_code (objf); Sanjoy> - Sanjoy> - break; Sanjoy> + { Sanjoy> + objf = jit_find_objf_with_entry_addr (entry_addr); Sanjoy> + if (objf == NULL) Sanjoy> + printf_unfiltered (_("Unable to find JITed code " Sanjoy> + "entry at address: %s\n"), Sanjoy> + paddress (gdbarch, entry_addr)); Sanjoy> + else Sanjoy> + jit_unregister_code (objf); Sanjoy> + break; Sanjoy> + } These hunks seem to be just formatting changes. Please drop. Tom