From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 17215 invoked by alias); 3 Feb 2009 02:04:13 -0000 Received: (qmail 17160 invoked by uid 22791); 3 Feb 2009 02:04:12 -0000 X-SWARE-Spam-Status: No, hits=-2.4 required=5.0 tests=AWL,BAYES_00 X-Spam-Check-By: sourceware.org Received: from rock.gnat.com (HELO rock.gnat.com) (205.232.38.15) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Tue, 03 Feb 2009 02:04:06 +0000 Received: from localhost (localhost.localdomain [127.0.0.1]) by filtered-rock.gnat.com (Postfix) with ESMTP id 0E3812A9629; Mon, 2 Feb 2009 21:04:05 -0500 (EST) Received: from rock.gnat.com ([127.0.0.1]) by localhost (rock.gnat.com [127.0.0.1]) (amavisd-new, port 10024) with LMTP id R001dEXz4GcE; Mon, 2 Feb 2009 21:04:04 -0500 (EST) Received: from joel.gnat.com (localhost.localdomain [127.0.0.1]) by rock.gnat.com (Postfix) with ESMTP id 9B82E2A960D; Mon, 2 Feb 2009 21:04:04 -0500 (EST) Received: by joel.gnat.com (Postfix, from userid 1000) id D30D5E7ACD; Mon, 2 Feb 2009 18:04:01 -0800 (PST) Date: Tue, 03 Feb 2009 02:04:00 -0000 From: Joel Brobecker To: Tristan Gingold Cc: gdb-patches@sourceware.org Subject: Re: Ping: [RFA] Add support of shared lib for Darwin Message-ID: <20090203020401.GD3964@adacore.com> References: <20090108140918.GA70183@ulanbator.act-europe.fr> <2BC5ADA3-B225-4760-822B-C8E31FD999A2@adacore.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <2BC5ADA3-B225-4760-822B-C8E31FD999A2@adacore.com> User-Agent: Mutt/1.4.2.2i 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: 2009-02/txt/msg00050.txt.bz2 No review of this patch so far, so I took a look :) > >2009-01-08 Tristan Gingold > > > > * machoread.c (macho_symfile_read): Read minsymtab also from > > shared libraries. > > (macho_symfile_read): Try to read dwarf2 frame info from main > > object file, but not from OSO files. > > (macho_symfile_offsets): Update section names for latest BFD > > changes. > > * i386-darwin-tdep.c (i386_darwin_init_abi): Call set_solib_ops. > > (x86_darwin_init_abi_64): Ditto. > > * configure.tgt: Add solib.o solib-darwin.o for Darwin. Generally speaking, this looks fine. I have a few minor comments... > >+/* Read dyld_all_image from inferior. */ > >+static void > >+darwin_load_image_infos (void) > >+{ > >+ gdb_byte buf[24]; I'm always nervous when I see hard-coded constants like this in buffer declarations? Would it make sense to use alloca? Or maybe add an assertion that len <= sizeof (buf)? > >+ len = 4 + 4 + 2 * ptr_type->length; Can you explain the computation using little comments, maybe? > >+/* Return non-zero if GDB_SO_NAME and INFERIOR_SO_NAME represent > >+ the same shared library. */ > >+ > >+static int > >+darwin_same (struct so_list *gdb, struct so_list *inferior) > >+{ > >+ return strcmp (gdb->so_original_name, inferior->so_original_name) > >== 0; > >+} I think that this function is not necessary. if so_ops.same is set to NULL, then GDB falls back to using strcmp like you did... Perhaps we could add a comment about that in solist.h, in fact. > >+/* Lookup the value for a specific symbol. */ of? > >+static CORE_ADDR > >+bfd_lookup_symbol (bfd *abfd, char *symname) The name of this function annoys me a little. With GDB's current conventions, it seems to suggest that this function is part of bfd. Can we call is darwin_lookup_symbol or darwin_lookup_symbol_from_bfd? > >+/* Return program interpreter string. */ > >+static gdb_byte * > >+find_program_interpreter (void) > >+{ [...] > >+ /* If we didn't find it, read from memory. > >+ FIXME: todo. */ Would it be complicated to do this now? I'm OK with looking at this later, if you think it's easier. I suppose this only really matter in the "attach" case, right? > >+/* Build a list of currently loaded shared objects. See solib- > >svr4.c */ > >+static struct so_list * > >+darwin_current_sos (void) > >+{ [...] > >+ /* Read infos for each solib. */ > >+ for (i = 0; i < dyld_all_image.count; i++) > >+ { > >+ CORE_ADDR info = dyld_all_image.info + i * image_info_size; > >+ char buf[image_info_size]; > >+ CORE_ADDR load_addr; [...] > >+ > >+ /* Read image info from inferior. */ > >+ if (target_read_memory (info, buf, image_info_size)) > >+ break; > >+ > >+ load_addr = extract_typed_address (buf, ptr_type); > >+ path_addr = extract_typed_address (buf + ptr_len, ptr_type); > >+ > >+ target_read_string (path_addr, &file_path, > >+ SO_NAME_MAX_PATH_SIZE - 1, &errcode); > >+ if (errcode) > >+ break; > >+ > >+ /* Ignore first entry as this is the executable itself. */ > >+ if (i == 0) > >+ continue; Is there a reason for reading the info about the first entry at all? Can we for instance start the loop with i = 1? > >+ if (!inf->attach_flag) > >+ { > >+ /* We find the dynamic linker's base address by examining > >+ the current pc (which should point at the entry point for the > >+ dynamic linker) and subtracting the offset of the entry point. */ > >+ load_addr = (read_pc () - bfd_get_start_address (dyld_bfd)); > >+ } > >+ else > >+ { > >+ /* FIXME: todo. > >+ Get address of __DATA.__dyld in exec_bfd, read address at offset 0 > >+ */ > >+ xfree (interp_name); > >+ return; > >+ } Can we implement this part as well? Same remark as above. OK to push to a separate patch if it helps, but might as well if it's easy. > >+extern initialize_file_ftype _initialize_svr4_solib; /* -Wmissing- > >prototypes */ Looks like an unused declaration. Unwanted copy/paste? The corresponding advance prototype for _initialize_darwin_solib really isn't necessary - I think. We have lots of files that don't provide this advance declaration. -- Joel