Re: RFA: Use lrealpath instead of gdb_realpath

Daniel Jacobowitz
2005-05-29T14:21:25Z

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