Patchwork [libiberty,include] : Add additional helper functions for directory-separator searching

login
register
mail settings
Submitter Pedro Alves
Date March 9, 2011, 11:46 a.m.
Message ID <201103091146.36746.pedro@codesourcery.com>
Download mbox | patch
Permalink /patch/86102/
State New
Headers show

Comments

Pedro Alves - March 9, 2011, 11:46 a.m.
On Wednesday 09 March 2011 05:29:09, Eli Zaretskii wrote:
> > Actually, is there any case where lbasename wouldn't
> > work instead of filename_dirrchr?
> 
> Almost: lbasename returns a pointer one character after the last
> slash.  It also skips the drive letter on DOS/Windows (which might be
> TRT, actually).

I meant a valid use case in the code bases.  Might as
well cook up a (gdb) patch.  Find it pasted below.  Does it look good to you?

The one's left are: 1 in a linux-native only file (never cares
for other filesystem semantics), and a couple in the coff and
mdebug readers.  The latter could be rewritten in terms of
lbasename, but I'm not sure whether gcc outputs a literal '/' in
that case even when building on mingw.  If so, and we changed them,
we'd be breaking reading these files on Windows, so I'd rather
change them only if proven necessary.  (remember the source vs host
path distinction we're missing)

> It would be reasonable to rewrite filename_dirrchr in terms of
> lbasename, though, by backing up the pointer returned by lbasename if
> it points to a slash, and otherwise returning NULL.  The case of
> "d:foo" should also be resolved (probably, return a pointer to the
> colon).

The means it's just an adapter, and callers could be rewritten
a little to think in terms of lbasename.  The only case where
I imagine filename_dirrchr would be a little bit more efficient,
is when breaking up path directories components from right
to left, for step-wise comparison with another path, say.
IMO, the less code to maintain and the fewer the APIs
readers have to understand, the better.

I did a:

 grep strrchr * | grep "'/'"

under gcc and all the cases I saw could/should
be using lbasename.  I suggest gcc folks fix that first
before anything else.
Eli Zaretskii - March 9, 2011, 12:35 p.m.
> From: Pedro Alves <pedro@codesourcery.com>
> Date: Wed, 9 Mar 2011 11:46:36 +0000
> Cc: gdb-patches@sourceware.org,
>  dj@redhat.com,
>  ktietz70@googlemail.com,
>  binutils@sourceware.org,
>  gcc-patches@gcc.gnu.org
> 
> On Wednesday 09 March 2011 05:29:09, Eli Zaretskii wrote:
> > > Actually, is there any case where lbasename wouldn't
> > > work instead of filename_dirrchr?
> > 
> > Almost: lbasename returns a pointer one character after the last
> > slash.  It also skips the drive letter on DOS/Windows (which might be
> > TRT, actually).
> 
> I meant a valid use case in the code bases.

Sorry for my misunderstanding.

> Might as well cook up a (gdb) patch.  Find it pasted below.  Does it
> look good to you?

Yes, looks fine.  Thanks.

> The one's left are: 1 in a linux-native only file (never cares
> for other filesystem semantics), and a couple in the coff and
> mdebug readers.  The latter could be rewritten in terms of
> lbasename, but I'm not sure whether gcc outputs a literal '/' in
> that case even when building on mingw.  If so, and we changed them,
> we'd be breaking reading these files on Windows

Sorry, I don't understand how would that break on Windows.  Could you
elaborate?  And what "couple of coff and mdebug readers" did you have
in mind?
Pedro Alves - March 9, 2011, 12:58 p.m.
On Wednesday 09 March 2011 12:35:00, Eli Zaretskii wrote:
> > I meant a valid use case in the code bases.
> 
> Sorry for my misunderstanding.

NP.

> 
> > Might as well cook up a (gdb) patch.  Find it pasted below.  Does it
> > look good to you?
> 
> Yes, looks fine.  Thanks.

Thanks.  I've applied it.

> > The one's left are: 1 in a linux-native only file (never cares
> > for other filesystem semantics), and a couple in the coff and
> > mdebug readers.  The latter could be rewritten in terms of
> > lbasename, but I'm not sure whether gcc outputs a literal '/' in
> > that case even when building on mingw.  If so, and we changed them,
> > we'd be breaking reading these files on Windows
> 
> Sorry, I don't understand how would that break on Windows.  Could you
> elaborate?  And what "couple of coff and mdebug readers" did you have
> in mind?

Sorry, in the hurry, I had a (another) brain cramp.  Wouldn't break.
Still it'd be useless to change this _if_ gcc hardcodes '/'.  Dunno
whether it does.

>find . -name "*.c" | xargs grep strrchr | grep "'/'"
./linux-thread-db.c:    cp = strrchr (path, '/');
./mdebugread.c:               p = strrchr (namestring, '/');
./dbxread.c:        p = strrchr (namestring, '/');

Both look like this:

	    /* Some compilers (including gcc) emit a pair of initial N_SOs.
	       The first one is a directory name; the second the file name.
	       If pst exists, is empty, and has a filename ending in '/',
	       we assume the previous N_SO was a directory name.  */

	    p = strrchr (namestring, '/');
	    if (p && *(p + 1) == '\000')
	      {
		/* Save the directory name SOs locally, then save it into
		   the psymtab when it's created below.  */
	        dirname_nso = namestring;
	        continue;		
	      }
Eli Zaretskii - March 9, 2011, 1:35 p.m.
> From: Pedro Alves <pedro@codesourcery.com>
> Date: Wed, 9 Mar 2011 12:58:38 +0000
> Cc: gdb-patches@sourceware.org,
>  dj@redhat.com,
>  ktietz70@googlemail.com,
>  binutils@sourceware.org,
>  gcc-patches@gcc.gnu.org
> 
> > > The one's left are: 1 in a linux-native only file (never cares
> > > for other filesystem semantics), and a couple in the coff and
> > > mdebug readers.  The latter could be rewritten in terms of
> > > lbasename, but I'm not sure whether gcc outputs a literal '/' in
> > > that case even when building on mingw.  If so, and we changed them,
> > > we'd be breaking reading these files on Windows
> > 
> > Sorry, I don't understand how would that break on Windows.  Could you
> > elaborate?  And what "couple of coff and mdebug readers" did you have
> > in mind?
> 
> Sorry, in the hurry, I had a (another) brain cramp.  Wouldn't break.
> Still it'd be useless to change this _if_ gcc hardcodes '/'.  Dunno
> whether it does.

At least on MinGW, GCC simply uses whatever was passed on the command
line.  I tested that by compiling the same source file, passing it to
GCC with different flavors of slashes, including mixed ones.  Then in
GDB I typed "info sources" and saw the source file with exactly the
same flavor of slashes as what I typed on the GCC command line.

Funnily enough, when the file name given to GCC includes at least one
backslash, "info sources" shows the same file twice, like this:

  (gdb) info sources
  Source files for which symbols have been read in:



  Source files for which symbols will be read in on demand:

  d:/usr/eli/data/dbw.c, d:\usr\eli/data\dbw.c

This is with GDB 7.2 and GCC 3.4.2.  That means we compare files with
strcmp/strcasecmp somewhere, and don't know that / and \ are
equivalent here.  Or maybe it's a bug in the ancient version of GCC I
use.
Jan Kratochvil - March 9, 2011, 2:37 p.m.
On Wed, 09 Mar 2011 13:58:38 +0100, Pedro Alves wrote:
> Thanks.  I've applied it.

nto-tdep.c:130:8: error: assignment discards ‘const’ qualifier from pointer target type [-Werror]

gcc-4.6.0-0.12.fc15.x86_64
--with-system-zlib --enable-64-bit-bfd --enable-targets=all --enable-static --disable-shared --enable-debug --disable-sim --enable-gold --with-separate-debug-dir=/usr/lib/debug CC=gcc CFLAGS=-m64 -ggdb2 -pipe -Wall -Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector --param=ssp-buffer-size=4 LDFLAGS= -lmcheck

commit 7403e6b3f0f7d4c4f80703486f602ee5e2c9a3dd
Author: Pedro Alves <pedro@codesourcery.com>
Date:   Wed Mar 9 12:48:53 2011 +0000

        * cli/cli-cmds.c (shell_escape): Use lbasename.
        * coffread.c (coff_start_symtab): Constify parameter.
        (complete_symtab): Constify `name' parameter.
        (coff_symtab_read): Constify `filestring' local.
        (coff_getfilename): Constify return and `result' local.
        Use lbasename.
        * fbsd-nat.c (fbsd_make_corefile_notes): Use lbasename.
        * linux-fork.c (info_checkpoints_command): Use lbasename.
        * linux-nat.c (linux_nat_make_corefile_notes): Use lbasename.
        * minsyms.c (lookup_minimal_symbol): Use lbasename.
        * nto-tdep.c (nto_find_and_open_solib): Use lbasename.
        * procfs.c (procfs_make_note_section): Use lbasename.
        * tui/tui-io.c (printable_part): Constity return and parameter.
        Use lbasename.
        (print_filename): Constify parameters, and local `s'.
        (tui_rl_display_match_list): Constify local `temp'.


Thanks,
Jan
Kai Tietz - March 12, 2011, 4:40 p.m.
2011/3/9 Eli Zaretskii <eliz@gnu.org>:
>> From: Pedro Alves <pedro@codesourcery.com>
>> Date: Wed, 9 Mar 2011 12:58:38 +0000
>> Cc: gdb-patches@sourceware.org,
>>  dj@redhat.com,
>>  ktietz70@googlemail.com,
>>  binutils@sourceware.org,
>>  gcc-patches@gcc.gnu.org
>>
>> > > The one's left are: 1 in a linux-native only file (never cares
>> > > for other filesystem semantics), and a couple in the coff and
>> > > mdebug readers.  The latter could be rewritten in terms of
>> > > lbasename, but I'm not sure whether gcc outputs a literal '/' in
>> > > that case even when building on mingw.  If so, and we changed them,
>> > > we'd be breaking reading these files on Windows
>> >
>> > Sorry, I don't understand how would that break on Windows.  Could you
>> > elaborate?  And what "couple of coff and mdebug readers" did you have
>> > in mind?
>>
>> Sorry, in the hurry, I had a (another) brain cramp.  Wouldn't break.
>> Still it'd be useless to change this _if_ gcc hardcodes '/'.  Dunno
>> whether it does.
>
> At least on MinGW, GCC simply uses whatever was passed on the command
> line.  I tested that by compiling the same source file, passing it to
> GCC with different flavors of slashes, including mixed ones.  Then in
> GDB I typed "info sources" and saw the source file with exactly the
> same flavor of slashes as what I typed on the GCC command line.
>
> Funnily enough, when the file name given to GCC includes at least one
> backslash, "info sources" shows the same file twice, like this:
>
>  (gdb) info sources
>  Source files for which symbols have been read in:
>
>
>
>  Source files for which symbols will be read in on demand:
>
>  d:/usr/eli/data/dbw.c, d:\usr\eli/data\dbw.c
>
> This is with GDB 7.2 and GCC 3.4.2.  That means we compare files with
> strcmp/strcasecmp somewhere, and don't know that / and \ are
> equivalent here.  Or maybe it's a bug in the ancient version of GCC I
> use.

Yes, this observation is related to some comparision tweaks in libcpp
and in some other parts in gcc about filenames. When gcc gets into
stage 1, I will post the prepared patch for this.

Kai

Patch

Index: src/gdb/cli/cli-cmds.c
===================================================================
--- src.orig/gdb/cli/cli-cmds.c	2011-03-09 10:53:22.062800003 +0000
+++ src/gdb/cli/cli-cmds.c	2011-03-09 11:14:37.672800007 +0000
@@ -730,16 +730,13 @@  shell_escape (char *arg, int from_tty)
 
   if ((pid = vfork ()) == 0)
     {
-      char *p, *user_shell;
+      const char *p, *user_shell;
 
       if ((user_shell = (char *) getenv ("SHELL")) == NULL)
 	user_shell = "/bin/sh";
 
       /* Get the name of the shell for arg0.  */
-      if ((p = strrchr (user_shell, '/')) == NULL)
-	p = user_shell;
-      else
-	p++;			/* Get past '/' */
+      p = lbasename (user_shell);
 
       if (!arg)
 	execl (user_shell, p, (char *) 0);
Index: src/gdb/coffread.c
===================================================================
--- src.orig/gdb/coffread.c	2011-03-09 10:53:22.062800003 +0000
+++ src/gdb/coffread.c	2011-03-09 11:14:37.672800007 +0000
@@ -178,7 +178,7 @@  static int init_lineno (bfd *, long, int
 
 static char *getsymname (struct internal_syment *);
 
-static char *coff_getfilename (union internal_auxent *);
+static const char *coff_getfilename (union internal_auxent *);
 
 static void free_stringtab (void);
 
@@ -366,7 +366,7 @@  coff_alloc_type (int index)
    it indicates the start of data for one original source file.  */
 
 static void
-coff_start_symtab (char *name)
+coff_start_symtab (const char *name)
 {
   start_symtab (
   /* We fill in the filename later.  start_symtab puts this pointer
@@ -388,7 +388,7 @@  coff_start_symtab (char *name)
    text.  */
 
 static void
-complete_symtab (char *name, CORE_ADDR start_addr, unsigned int size)
+complete_symtab (const char *name, CORE_ADDR start_addr, unsigned int size)
 {
   if (last_source_file != NULL)
     xfree (last_source_file);
@@ -713,7 +713,7 @@  coff_symtab_read (long symtab_offset, un
   int in_source_file = 0;
   int next_file_symnum = -1;
   /* Name of the current file.  */
-  char *filestring = "";
+  const char *filestring = "";
   int depth = 0;
   int fcn_first_line = 0;
   CORE_ADDR fcn_first_line_addr = 0;
@@ -1308,12 +1308,12 @@  getsymname (struct internal_syment *symb
    Return only the last component of the name.  Result is in static
    storage and is only good for temporary use.  */
 
-static char *
+static const char *
 coff_getfilename (union internal_auxent *aux_entry)
 {
   static char buffer[BUFSIZ];
   char *temp;
-  char *result;
+  const char *result;
 
   if (aux_entry->x_file.x_n.x_zeroes == 0)
     {
@@ -1331,8 +1331,7 @@  coff_getfilename (union internal_auxent
   /* FIXME: We should not be throwing away the information about what
      directory.  It should go into dirname of the symtab, or some such
      place.  */
-  if ((temp = strrchr (result, '/')) != NULL)
-    result = temp + 1;
+  result = lbasename (result);
   return (result);
 }
 
Index: src/gdb/fbsd-nat.c
===================================================================
--- src.orig/gdb/fbsd-nat.c	2011-03-09 10:53:22.062800003 +0000
+++ src/gdb/fbsd-nat.c	2011-03-09 11:14:37.682800003 +0000
@@ -202,7 +202,7 @@  fbsd_make_corefile_notes (bfd *obfd, int
 
   if (get_exec_file (0))
     {
-      char *fname = strrchr (get_exec_file (0), '/') + 1;
+      char *fname = lbasename (get_exec_file (0));
       char *psargs = xstrdup (fname);
 
       if (get_inferior_args ())
Index: src/gdb/linux-fork.c
===================================================================
--- src.orig/gdb/linux-fork.c	2011-03-09 10:53:22.062800003 +0000
+++ src/gdb/linux-fork.c	2011-03-09 11:14:37.682800003 +0000
@@ -584,14 +584,7 @@  info_checkpoints_command (char *arg, int
 
       sal = find_pc_line (pc, 0);
       if (sal.symtab)
-	{
-	  char *tmp = strrchr (sal.symtab->filename, '/');
-
-	  if (tmp)
-	    printf_filtered (_(", file %s"), tmp + 1);
-	  else
-	    printf_filtered (_(", file %s"), sal.symtab->filename);
-	}
+	printf_filtered (_(", file %s"), lbasename (sal.symtab->filename));
       if (sal.line)
 	printf_filtered (_(", line %d"), sal.line);
       if (!sal.symtab && !sal.line)
Index: src/gdb/linux-nat.c
===================================================================
--- src.orig/gdb/linux-nat.c	2011-03-09 10:53:22.062800003 +0000
+++ src/gdb/linux-nat.c	2011-03-09 11:14:37.682800003 +0000
@@ -4491,7 +4491,7 @@  linux_nat_make_corefile_notes (bfd *obfd
 
   if (get_exec_file (0))
     {
-      strncpy (fname, strrchr (get_exec_file (0), '/') + 1, sizeof (fname));
+      strncpy (fname, lbasename (get_exec_file (0)), sizeof (fname));
       strncpy (psargs, get_exec_file (0), sizeof (psargs));
       if (get_inferior_args ())
 	{
Index: src/gdb/minsyms.c
===================================================================
--- src.orig/gdb/minsyms.c	2011-03-09 10:53:22.072800005 +0000
+++ src/gdb/minsyms.c	2011-03-09 11:14:37.682800003 +0000
@@ -198,12 +198,7 @@  lookup_minimal_symbol (const char *name,
   const char *modified_name;
 
   if (sfile != NULL)
-    {
-      char *p = strrchr (sfile, '/');
-
-      if (p != NULL)
-	sfile = p + 1;
-    }
+    sfile = lbasename (sfile);
 
   /* For C++, canonicalize the input name.  */
   modified_name = name;
Index: src/gdb/nto-tdep.c
===================================================================
--- src.orig/gdb/nto-tdep.c	2011-03-09 10:53:22.062800003 +0000
+++ src/gdb/nto-tdep.c	2011-03-09 11:14:37.682800003 +0000
@@ -127,13 +127,7 @@  nto_find_and_open_solib (char *solib, un
   sprintf (buf, PATH_FMT, arch_path, arch_path, arch_path, arch_path,
 	   arch_path);
 
-  /* Don't assume basename() isn't destructive.  */
-  base = strrchr (solib, '/');
-  if (!base)
-    base = solib;
-  else
-    base++;			/* Skip over '/'.  */
-
+  base = lbasename (solib);
   ret = openp (buf, 1, base, o_flags, temp_pathname);
   if (ret < 0 && base != solib)
     {
Index: src/gdb/procfs.c
===================================================================
--- src.orig/gdb/procfs.c	2011-03-09 10:53:22.062800003 +0000
+++ src/gdb/procfs.c	2011-03-09 11:14:37.682800003 +0000
@@ -5734,7 +5734,7 @@  procfs_make_note_section (bfd *obfd, int
 
   if (get_exec_file (0))
     {
-      strncpy (fname, strrchr (get_exec_file (0), '/') + 1, sizeof (fname));
+      strncpy (fname, lbasename (get_exec_file (0)), sizeof (fname));
       strncpy (psargs, get_exec_file (0),
 	       sizeof (psargs));
 
Index: src/gdb/tui/tui-io.c
===================================================================
--- src.orig/gdb/tui/tui-io.c	2011-03-09 10:53:22.062800003 +0000
+++ src/gdb/tui/tui-io.c	2011-03-09 11:14:37.682800003 +0000
@@ -324,20 +324,10 @@  tui_readline_output (int error, gdb_clie
    final slash.  Otherwise, we return what we were passed.
 
    Comes from readline/complete.c.  */
-static char *
-printable_part (char *pathname)
+static const char *
+printable_part (const char *pathname)
 {
-  char *temp;
-
-  temp = rl_filename_completion_desired
-    ? strrchr (pathname, '/') : (char *)NULL;
-#if defined (__MSDOS__)
-  if (rl_filename_completion_desired 
-      && temp == 0 && isalpha (pathname[0]) 
-      && pathname[1] == ':')
-    temp = pathname + 1;
-#endif
-  return (temp ? ++temp : pathname);
+  return rl_filename_completion_desired ? lbasename (pathname) : pathname;
 }
 
 /* Output TO_PRINT to rl_outstream.  If VISIBLE_STATS is defined and
@@ -366,10 +356,10 @@  printable_part (char *pathname)
     } while (0)
 
 static int
-print_filename (char *to_print, char *full_pathname)
+print_filename (const char *to_print, const char *full_pathname)
 {
   int printed_len = 0;
-  char *s;
+  const char *s;
 
   for (s = to_print; *s; s++)
     {
@@ -416,7 +406,7 @@  tui_rl_display_match_list (char **matche
   
   int count, limit, printed_len;
   int i, j, k, l;
-  char *temp;
+  const char *temp;
 
   /* Screen dimension correspond to the TUI command window.  */
   int screenwidth = TUI_CMD_WIN->generic.width;