From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 31535 invoked by alias); 4 Jan 2008 15:30:49 -0000 Received: (qmail 31527 invoked by uid 22791); 4 Jan 2008 15:30:48 -0000 X-Spam-Check-By: sourceware.org Received: from rock.gnat.com (HELO rock.gnat.com) (205.232.38.15) by sourceware.org (qpsmtpd/0.31) with ESMTP; Fri, 04 Jan 2008 15:30:31 +0000 Received: from localhost (localhost.localdomain [127.0.0.1]) by filtered-rock.gnat.com (Postfix) with ESMTP id 4C12E2A9679; Fri, 4 Jan 2008 10:30:29 -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 m9jXwbWY5BMn; Fri, 4 Jan 2008 10:30:29 -0500 (EST) Received: from joel.gnat.com (localhost.localdomain [127.0.0.1]) by rock.gnat.com (Postfix) with ESMTP id 47B202A95EE; Fri, 4 Jan 2008 10:30:28 -0500 (EST) Received: by joel.gnat.com (Postfix, from userid 1000) id E9276E7ACB; Fri, 4 Jan 2008 07:30:20 -0800 (PST) Date: Fri, 04 Jan 2008 15:30:00 -0000 From: Joel Brobecker To: Vladimir Prus , gdb-patches@sources.redhat.com Subject: Re: [RFA] Handle solaris dynamic linker name change. Message-ID: <20080104153020.GB5975@adacore.com> References: <200712242048.33983.vladimir@codesourcery.com> <20080104054951.GB28411@adacore.com> <20080104124611.GA26872@caradoc.them.org> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20080104124611.GA26872@caradoc.them.org> 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: 2008-01/txt/msg00056.txt.bz2 > > I'd like the feedback from other maintainers about this. > > I think it's fine with that improvement or without it; I really doubt > the code will trigger anywhere else. My two cents. OK, in that case, let's leave it the way Volodya original proposed it. I'd just like us to add a comment in svr4_same to say that we don't restrict the check to solaris, but that the chances of facing this situation in any other OS are very small. Looking again at the patch: > Ignore change in name of dynamic linker during > execution. This also unbreaks pending breakpoints. > * solist.h (struct target_so_ops): New field > same. > * solib-svr4.c (svr4_same): New. > (_initialize_svr4_solib): Register svr4_same. > * solib.c (update_solib_list): Use ops->same, > if available. Just a very very minor comment: Your fill-column looks very small, and makes it harder to read your sentences. It's supposed to be 74. Don't go out of your way to fix, though, in the grand scheme of things, I don't think that you're breaking a GNU Coding standard rule (I just did a quick check of the section detailing ChangeLogs). > + /* On Solaris, when starting inferior we think that > + dynamic linker is /usr/lib/ld.so.1, but later on, > + the table of loaded shared libraries contains > + /lib/ld.so.1. > + Sometimes one file is a link to another, but sometimes > + they have identical content, but are not linked to each > + other. */ Same small fill-column ;-). Actually, the real comment is to remind you that an extra comment as explained at the beginning of this email would be appreciated. > + if (strcmp (gdb->so_original_name, "/usr/lib/ld.so.1") == 0 > + && strcmp (inferior->so_original_name, "/lib/ld.so.1") == 0) > + return 1; Is it always going to be in that order (original in /usr/lib and gdb in /usr)? Or was this from experience? > + /* Given two so_list, first from GDB thread list and another > + present in the list returned by current_sos, return 1 if > + they are equal -- referring to the same library. */ > + int (*same) (struct so_list *gdb, struct so_list *inferior); I am not a native English speaker, but the above sounds funny. I suggest a slightly different version: /* Given two so_list objects, one from the GDB thread list and another from the list returned by current_sos, return 1 if they represent the same library. */ This is all I have - your patch is pre-approved with the comment adjustments. We can discuss my question about /usr/lib and /lib as a separate patch (this is already an improvement that we don't need to delay). Thanks! -- Joel