From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 9989 invoked by alias); 3 Feb 2009 11:41:16 -0000 Received: (qmail 9980 invoked by uid 22791); 3 Feb 2009 11:41:16 -0000 X-SWARE-Spam-Status: No, hits=-2.4 required=5.0 tests=AWL,BAYES_00 X-Spam-Check-By: sourceware.org Received: from mel.act-europe.fr (HELO mel.act-europe.fr) (212.99.106.210) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Tue, 03 Feb 2009 11:41:07 +0000 Received: from localhost (localhost [127.0.0.1]) by filtered-smtp.eu.adacore.com (Postfix) with ESMTP id C7832290065; Tue, 3 Feb 2009 12:41:04 +0100 (CET) Received: from mel.act-europe.fr ([127.0.0.1]) by localhost (smtp.eu.adacore.com [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id GxwwPaPTr5d4; Tue, 3 Feb 2009 12:41:03 +0100 (CET) Received: from ulanbator.act-europe.fr (ulanbator.act-europe.fr [10.10.1.67]) (using TLSv1 with cipher AES128-SHA (128/128 bits)) (No client certificate requested) by mel.act-europe.fr (Postfix) with ESMTP id A38EF290060; Tue, 3 Feb 2009 12:40:39 +0100 (CET) Cc: gdb-patches@sourceware.org Message-Id: <83557EA1-5F1D-4E70-AAA8-D1D3E8235536@adacore.com> From: Tristan Gingold To: Joel Brobecker In-Reply-To: <20090203020401.GD3964@adacore.com> Content-Type: text/plain; charset=US-ASCII; format=flowed; delsp=yes Content-Transfer-Encoding: 7bit Mime-Version: 1.0 (Apple Message framework v930.3) Subject: Re: Ping: [RFA] Add support of shared lib for Darwin Date: Tue, 03 Feb 2009 11:41:00 -0000 References: <20090108140918.GA70183@ulanbator.act-europe.fr> <2BC5ADA3-B225-4760-822B-C8E31FD999A2@adacore.com> <20090203020401.GD3964@adacore.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: 2009-02/txt/msg00053.txt.bz2 On Feb 3, 2009, at 3:04 AM, Joel Brobecker wrote: > No review of this patch so far, so I took a look :) Thank you for reviewing. >>> +/* 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)? I will add the assertion. >>> + len = 4 + 4 + 2 * ptr_type->length; > > Can you explain the computation using little comments, maybe? Ok. >>> +/* 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. Ok. >>> +/* 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? Ok, I changed it to 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? I think this case won't happen once pid_to_exec_file is implemented (and I have an implementation for this target op). >>> +/* 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? Yes. >>> + 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. Unfortunately this doesn't look very easy. This method is not documented and I had to read the sources of the dynamic loader to find it. I really prefer to postpone the implementation. >>> +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. Oops, this is a left over previous cleanups. I plan to re-submit the patch soon. Tristan.