From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 29508 invoked by alias); 29 May 2005 14:21:25 -0000 Mailing-List: contact gdb-patches-help@sources.redhat.com; run by ezmlm Precedence: bulk List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sources.redhat.com Received: (qmail 29498 invoked by uid 22791); 29 May 2005 14:21:22 -0000 Received: from nevyn.them.org (HELO nevyn.them.org) (66.93.172.17) by sourceware.org (qpsmtpd/0.30-dev) with ESMTP; Sun, 29 May 2005 14:21:22 +0000 Received: from drow by nevyn.them.org with local (Exim 4.50) id 1DcOfA-0006IL-57; Sun, 29 May 2005 10:21:12 -0400 Date: Sun, 29 May 2005 14:23:00 -0000 From: Daniel Jacobowitz To: Eli Zaretskii Cc: gdb-patches@sourceware.org, Christopher Faylor Subject: Re: RFA: Use lrealpath instead of gdb_realpath Message-ID: <20050529142112.GB23858@nevyn.them.org> Mail-Followup-To: Eli Zaretskii , gdb-patches@sourceware.org, Christopher Faylor References: <20050528234233.GA3440@nevyn.them.org> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.8i X-SW-Source: 2005-05/txt/msg00616.txt.bz2 On Sun, May 29, 2005 at 11:36:27AM +0300, Eli Zaretskii wrote: > As your patch clearly shows, gdb_realpath is _not_ identical to > lrealpath. For example, it doesn't have the Windows code that calls > GetFullPathName. So we are not replacing a duplicated function, not > really. I don't know how much clearer I can get. They were duplicated functions. I can say that with great certainty, since I used gdb_realpath as a basis for lrealpath when I needed its functionality in libiberty. They have diverged over time. That creates a maintenance burden - this particular one in fact. > Let me explain my concerns about using lrealpath in its current shape: > > . The Windows code in lrealpath converts all forward slashes to > backslashes, which I think is a bad idea. By contrast, the current > gdb_realpath would leave existing forward slashes alone. That is > an incompatible change, and my gut feeling is that we should not > risk it. On top of that, I think we always use '/' to generate > file names, so with this change we will produce ugly file names > like D:\foo\bar/baz.c. The Windows maintainers disagree with you. So, for the record, do I. > . The Windows code in lrealpath downcases the file name returned by > GetFullPathName. That might be okay for when we need a canonical > file name suitable for comparison by strcmp, but in the case of GDB > it is IMHO a misservice to the user, since (a) we correctly use > FILENAME_CMP to compare file names, and (b) presenting downcased > file names to the user is a gratuitous ugliness that we should try > to avoid. > > . GetFullPathName is documented to work even if the path and/or the > file name do not exist. By contrast, both realpath and > canonicalize_file_name are documented to fail if some or all of the > components of the resulting file name do not exist. Thus, I submit > that the Windows code is not a faithful emulation of the Posix > code, and thus could cause subtle bugs/misfeatures. These were both implemented by Danny Smith. You were furious with me for daring to disagree with you on how DJGPP handles filenames; and now you're critiquing the mingw maintainers for how they choose to handle filenames? Please, pardon me if I find that a bit ironic. > . lrealpath uses strdup while gdb_realpath uses xstrdup. Thus, if > we use lrealpath, we will behave differently when we run out of > memory. That said, thank you for catching this! I will not apply the patch. This is definitely a problem since existing users assume this behavior. I was asked to make this change when I moved the function into libiberty, since libiberty functions should not abort on their own initiative. It is not worth my time to continue arguing about filename handling; consider the patch withdrawn indefinitely. > > --- symtab.c 8 Mar 2005 04:34:44 -0000 1.145 > > +++ symtab.c 28 May 2005 23:13:40 -0000 > > @@ -163,7 +163,7 @@ lookup_symtab (const char *name) > > { > > full_path = xfullpath (name); > > make_cleanup (xfree, full_path); > > - real_path = gdb_realpath (name); > > + real_path = lrealpath (name); > > make_cleanup (xfree, real_path); > > } > > Can we please talk about this snippet? I don't understand the need > for calling both xfullpath and lrealpath here. If that's because > lrealpath might fail if the basename does not exist, I think we should > call lrealpath first and fall back on xfullpath only after that. We can't talk about it in the context of a patch replacing gdb_realpath with lrealpath. If you'd like to talk about it separately, go ahead. Having looked at the context, I have no opinion either way. -- Daniel Jacobowitz CodeSourcery, LLC