[RFA/libiberty] Enhance FILENAME_CMP for Windows filesystems

classic Classic list List threaded Threaded
17 messages Options
Reply | Threaded
Open this post in threaded view
|

[RFA/libiberty] Enhance FILENAME_CMP for Windows filesystems

Joel Brobecker
Hello,

We are using the FILENAME_CMP macro in GDB quite a bit, and it is
currently defined in include/filenames.h as:

    #define FILENAME_CMP(s1, s2)    strcasecmp(s1, s2)

This is for "DOSish" filesystems. For other filesystems, it's defined as
a straight strcmp:

    #define FILENAME_CMP(s1, s2)    strcmp(s1, s2)

I came across a case where the DWARF debug info unfortunately used '\'
as the directory separator in one case (in the .debug_info section),
while using '/' in another case (in the .debug_line section).

As a result, FILENAME_CMP returned that the following two files were
not equal:

        c:\cygwin\home\brobecke\tgdb\ex\\foo.adb
        c:/cygwin/home/brobecke/tgdb/ex//foo.adb

This causes the following problem in GDB, when we compile the unit
by giving GCC the full path name to the associated file, such as:

        % gcc -c -g c:\full\path\to\foo.adb

The GDB error looks like this:

        % gdb foo
        (gdb) b foo.adb:3
        No line 3 in file "c:\cygwin\home\brobecke\tgdb\ex\\foo.adb".


I propose to add a new filename_cmp function to libiberty:
  - On Unix, no change, it calls strcmp
  - On Windows, we make slash and backslash equal.

This is a modest improvement, since there are many other things we
can do to enhance it (such as normalizing the path so that '//' and '/'
are treated as equal for instance). But this paves the path for
further improvements of that sort. And in the meantime, if fixes
the problem we are facing.

For that, I added a new file, filename_cmp.c, updated the Makefile
to add this file to the list of required objects, and then changed
the FILENAME_CMP define to unconditionally call this new function.

libiberty/ change:
2007-03-28  Joel Brobecker  <[hidden email]>

        * filename_cmp.c: New file.
        * Makefile.in (CFILES): Add filename_cmp.c.
        (REQUIRED_OFILES): Add filename_cmp.o
        (filename_cmp.o): New rule.

include/ change:
2007-03-28  Joel Brobecker  <[hidden email]>

        * filenames.h (FILENAME_CMP): Adjust define to call filename_cmp
        regardless of the type of file system.

Tested on x86-linux and x86-windows, without any regression (using
the GDB testsuite).

OK to apply?

Thank you,
--
Joel

filename_cmp.c (1K) Download Attachment
filename_cmp.diff (3K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [RFA/libiberty] Enhance FILENAME_CMP for Windows filesystems

DJ Delorie-2

Is this the right exception?  I think we want the gcc-only version,
like in vsprintf.c.  IIRC this one is for the files that are required
as part of the C++ ABI.

Unless this new file is used as part of the C++ runtime ABI?

>    In addition to the permissions in the GNU General Public License, the
>    Free Software Foundation gives you unlimited permission to link the
>    compiled version of this file into combinations with other programs,
>    and to distribute those combinations without any restriction coming
>    from the use of this file.  (The General Public License restrictions
>    do apply in other respects; for example, they cover modification of
>    the file, and distribution when not linked into a combined
>    executable.)

Also, please include inline documentation for functions.texi.
Reply | Threaded
Open this post in threaded view
|

Re: [RFA/libiberty] Enhance FILENAME_CMP for Windows filesystems

Joel Brobecker
> Is this the right exception?  I think we want the gcc-only version,
> like in vsprintf.c.  IIRC this one is for the files that are required
> as part of the C++ ABI.
>
> Unless this new file is used as part of the C++ runtime ABI?

Hmmm, You are probably right. I copied this copyright blurb from
another file without paying enough attention. I think the physmem.c
copyright notice will be more appropriate, but I will double-check
where the macro is being used.

   This program is free software; you can redistribute it and/or modify
   it under the terms of the GNU General Public License as published by
   the Free Software Foundation; either version 2, or (at your option)
   any later version.

   This program is distributed in the hope that it will be useful,
   but WITHOUT ANY WARRANTY; without even the implied warranty of
   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
   GNU General Public License for more details.

   You should have received a copy of the GNU General Public License
   along with this program; if not, write to the Free Software Foundation,
   Inc., 51 Franklin Street - Fifth Floor, Boston, MA 02110-1301, USA.  */

> Also, please include inline documentation for functions.texi.

Ah, yes, I forgot that part. Thanks for reminding me.

Thanks,
--
Joel
Reply | Threaded
Open this post in threaded view
|

Re: [RFA/libiberty] Enhance FILENAME_CMP for Windows filesystems

Joel Brobecker
> Hmmm, You are probably right. I copied this copyright blurb from
> another file without paying enough attention. I think the physmem.c
> copyright notice will be more appropriate, but I will double-check
> where the macro is being used.

The macro is only used in java/jcf-path.c and protoize.c, so I don't
think it needs the exclusion.

Here is a revised version of the patch that should address both
comments (copyright notice, and lack of documentation):

libiberty/ change:
2007-03-28  Joel Brobecker  <[hidden email]>

        * filename_cmp.c: New file.
        * Makefile.in (CFILES): Add filename_cmp.c.
        (REQUIRED_OFILES): Add filename_cmp.o
        (filename_cmp.o): New rule.
        * functions.texi: Regenerate.

include/ change:
2007-03-28  Joel Brobecker  <[hidden email]>

        * filenames.h (FILENAME_CMP): Adjust define to call filename_cmp
        regardless of the type of file system.

The documentation change was tested by regenerating function.texi
then regenerating the .info documentation, and finally by browsing
the resulting document with the "info" program.

OK to apply?

Thank you,
--
Joel

filename_cmp.c (2K) Download Attachment
filename_cmp.diff (4K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [RFA/libiberty] Enhance FILENAME_CMP for Windows filesystems

DJ Delorie-2

Ok.
Reply | Threaded
Open this post in threaded view
|

Re: [RFA/libiberty] Enhance FILENAME_CMP for Windows filesystems

Joel Brobecker
> Ok.

Thank you. I just checked everything in.

--
Joel
Reply | Threaded
Open this post in threaded view
|

Re: [RFA/libiberty] Enhance FILENAME_CMP for Windows filesystems

Joel Brobecker
Re:

    libiberty/ change:
    2007-03-28  Joel Brobecker  <[hidden email]>
   
            * filename_cmp.c: New file.
            * Makefile.in (CFILES): Add filename_cmp.c.
            (REQUIRED_OFILES): Add filename_cmp.o
            (filename_cmp.o): New rule.
            * functions.texi: Regenerate.
   
    include/ change:
    2007-03-28  Joel Brobecker  <[hidden email]>
   
            * filenames.h (FILENAME_CMP): Adjust define to call filename_cmp
            regardless of the type of file system.

> > Ok.
>
> Thank you. I just checked everything in.

I checked the change in gcc, but are these checkins propagated to GDB
as well? It doesn't seem like it.

How do we handle synchronization with libiberty changes?

--
Joel
Reply | Threaded
Open this post in threaded view
|

Re: [RFA/libiberty] Enhance FILENAME_CMP for Windows filesystems

Daniel Jacobowitz-2
On Thu, Mar 29, 2007 at 02:03:14PM -0700, Joel Brobecker wrote:
> > > Ok.
> >
> > Thank you. I just checked everything in.
>
> I checked the change in gcc, but are these checkins propagated to GDB
> as well? It doesn't seem like it.
>
> How do we handle synchronization with libiberty changes?

Manually; please commit it to src too.

Thanks for fixing this, by the way.  We had a similar patch in our
queue of things to post...

--
Daniel Jacobowitz
CodeSourcery
Reply | Threaded
Open this post in threaded view
|

Re: [RFA/libiberty] Enhance FILENAME_CMP for Windows filesystems

DJ Delorie-2
In reply to this post by Joel Brobecker

> I checked the change in gcc, but are these checkins propagated to GDB
> as well? It doesn't seem like it.

Normally, my cron job does it within an hour, if I notice.  If you
commit to both, the cron job lets you ;-)

> How do we handle synchronization with libiberty changes?

gcc is the master.  Whenever gcc and src differ, my cron job tells me
and prepares a script that migrates the changes to src.  I just run
the script.
Reply | Threaded
Open this post in threaded view
|

Re: [RFA/libiberty] Enhance FILENAME_CMP for Windows filesystems

Joel Brobecker
> > I checked the change in gcc, but are these checkins propagated to GDB
> > as well? It doesn't seem like it.
>
> Normally, my cron job does it within an hour, if I notice.  If you
> commit to both, the cron job lets you ;-)

Ah ha, I see now. Thanks! I will simply commit to gdb as well.

--
Joel
Reply | Threaded
Open this post in threaded view
|

Re: [RFA/libiberty] Enhance FILENAME_CMP for Windows filesystems

Eli Zaretskii
In reply to this post by Joel Brobecker
> Date: Wed, 28 Mar 2007 13:24:43 -0700
> From: Joel Brobecker <[hidden email]>
> Cc: [hidden email], [hidden email]
>
> Here is a revised version of the patch that should address both
> comments (copyright notice, and lack of documentation):

Sorry for chiming in only now, but I have a few minor comments:

> @deftypefn Extension int filename_cmp (const char *@var{s1}, const char *@var{s2})
>
> Return zero if the two paths @var{s1} and @var{s2} are equivalent.

The GNU project frowns on using `path' to mean a file name.  `Path' is
reserved to $PATH-like lists of directories.  In this context, I
suggest to use `file name' instead.  (There's one more instance of
using `path' in the documentation of this function.)

> If not equivalent, the returned value is similar to what strcmp would

"strcmp" should be in @code{}, as it is a C symbol.

> This function does not normalize path names. As a result, this function
                                              ^
Two blanks, please.

> int
> filename_cmp (const char *s1, const char *s2)
> {
> #ifndef HAVE_DOS_BASED_FILE_SYSTEM
>   return strcmp(s1, s2);

While I realize that the original FILENAME_CMP macro did the same, as
long as we are trying to do better, wouldn't it be nice if this
function also collapsed multiple consecutive slashes or backslashes?

>   for (;;)
>     {
>       int c1 = tolower (*s1);
>       int c2 = tolower (*s2);

Are we sure that strncasecmp's behavior wrt to locales is identical to
that of tolower's?  If not, the above will introduce a bug in
non-English locales.
Reply | Threaded
Open this post in threaded view
|

Re: [RFA/libiberty] Enhance FILENAME_CMP for Windows filesystems

Christopher Faylor-9
>On Wed, 28 Mar 2007 12:18:45 -0700, Joel Brobaker wrote:
>>This is a modest improvement, since there are many other things we
>>can do to enhance it (such as normalizing the path so that '//' and '/'
>>are treated as equal for instance). But this paves the path for
>>further improvements of that sort.

...

On Sat, Mar 31, 2007 at 02:57:01PM +0300, Eli Zaretskii wrote:
>While I realize that the original FILENAME_CMP macro did the same, as
>long as we are trying to do better, wouldn't it be nice if this
>function also collapsed multiple consecutive slashes or backslashes?

Joel already acknowledge that this was a possible future improvement.

If this is done, please be sure to preserve two slashes at the beginning
of a filename since Windows uses those.

cgf
Reply | Threaded
Open this post in threaded view
|

Re: [RFA/libiberty] Enhance FILENAME_CMP for Windows filesystems

Eli Zaretskii
> Date: Sat, 31 Mar 2007 18:39:59 -0400
> From: Christopher Faylor <[hidden email]>
>
> On Sat, Mar 31, 2007 at 02:57:01PM +0300, Eli Zaretskii wrote:
> >While I realize that the original FILENAME_CMP macro did the same, as
> >long as we are trying to do better, wouldn't it be nice if this
> >function also collapsed multiple consecutive slashes or backslashes?
>
> Joel already acknowledge that this was a possible future improvement.
>
> If this is done, please be sure to preserve two slashes at the beginning
> of a filename since Windows uses those.

Right, sorry I forgot to mention that.
Reply | Threaded
Open this post in threaded view
|

Re: [RFA/libiberty] Enhance FILENAME_CMP for Windows filesystems

Andreas Schwab
In reply to this post by Joel Brobecker
Joel Brobecker <[hidden email]> writes:

> /* File name comparison routine.
>
>    Copyright (C) 2007 Free Software Foundation, Inc.
>
>    This program is free software; you can redistribute it and/or modify
>    it under the terms of the GNU General Public License as published by
>    the Free Software Foundation; either version 2, or (at your option)
>    any later version.
>
>    This program is distributed in the hope that it will be useful,
>    but WITHOUT ANY WARRANTY; without even the implied warranty of
>    MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>    GNU General Public License for more details.
>
>    You should have received a copy of the GNU General Public License
>    along with this program; if not, write to the Free Software Foundation,
>    Inc., 51 Franklin Street - Fifth Floor, Boston, MA 02110-1301, USA.  */
>
> #ifdef HAVE_STRING_H
> #include <string.h>
> #endif

That won't work without #include "config.h".

Andreas.

--
Andreas Schwab, SuSE Labs, [hidden email]
SuSE Linux Products GmbH, Maxfeldstraße 5, 90409 Nürnberg, Germany
PGP key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."
Reply | Threaded
Open this post in threaded view
|

Re: [RFA/libiberty] Enhance FILENAME_CMP for Windows filesystems

Joel Brobecker
> > #ifdef HAVE_STRING_H
> > #include <string.h>
> > #endif
>
> That won't work without #include "config.h".

Ah ha! Ben Elliston wrote me privately that he gets a warning
on x86-linux (Ubunty) that strcmp.h is not defined. That would
probably explain it. Thank you!

Unfortunately, I'm going to be unavailable for the next two of three
days. I'll try to fix it ASAP, but if someone could do this for me,
I would really appreciate it.

--
Joel
Reply | Threaded
Open this post in threaded view
|

Re: [RFA/libiberty] Enhance FILENAME_CMP for Windows filesystems

Joel Brobecker
In reply to this post by Eli Zaretskii
> Sorry for chiming in only now, but I have a few minor comments:

Not a problem at all, Eli. I am not able to followup on them right
now, but I promise I will in a few days.

> >   for (;;)
> >     {
> >       int c1 = tolower (*s1);
> >       int c2 = tolower (*s2);
>
> Are we sure that strncasecmp's behavior wrt to locales is identical to
> that of tolower's?  If not, the above will introduce a bug in
> non-English locales.

For this question, I'm not sure, actually.

--
Joel
Reply | Threaded
Open this post in threaded view
|

Re: [RFA/libiberty] Enhance FILENAME_CMP for Windows filesystems

Andreas Schwab
In reply to this post by Joel Brobecker
Joel Brobecker <[hidden email]> writes:

>> > #ifdef HAVE_STRING_H
>> > #include <string.h>
>> > #endif
>>
>> That won't work without #include "config.h".
>
> Ah ha! Ben Elliston wrote me privately that he gets a warning
> on x86-linux (Ubunty) that strcmp.h is not defined. That would
> probably explain it. Thank you!
>
> Unfortunately, I'm going to be unavailable for the next two of three
> days. I'll try to fix it ASAP, but if someone could do this for me,
> I would really appreciate it.

I've checked this in.

Andreas.

2007-04-02  Andreas Schwab  <[hidden email]>

        * filename_cmp.c: Include "config.h".

--- libiberty/filename_cmp.c.~1.1.~ 2007-03-29 23:03:48.000000000 +0200
+++ libiberty/filename_cmp.c 2007-04-01 22:59:02.000000000 +0200
@@ -16,6 +16,10 @@
    along with this program; if not, write to the Free Software Foundation,
    Inc., 51 Franklin Street - Fifth Floor, Boston, MA 02110-1301, USA.  */
 
+#ifdef HAVE_CONFIG_H
+#include "config.h"
+#endif
+
 #ifdef HAVE_STRING_H
 #include <string.h>
 #endif

--
Andreas Schwab, SuSE Labs, [hidden email]
SuSE Linux Products GmbH, Maxfeldstraße 5, 90409 Nürnberg, Germany
PGP key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."