[6/7] Use canonicalize_file_name unconditionally

Message ID 1416980800-21408-7-git-send-email-yao@codesourcery.com
State New, archived
Headers

Commit Message

Yao Qi Nov. 26, 2014, 5:46 a.m. UTC
  As gnulib module canonicalize-lgpl is imported, we can use
canonicalize_file_name and realpath unconditionally.

gdb:

2014-11-26  Yao Qi  <yao@codesourcery.com>

	* configure.ac (AC_CHECK_FUNCS): Remove canonicalize_file_name
	and realpath.
	* config.in: Re-generated.
	* configure: Re-generated.
	* utils.c (gdb_realpath): Remove code calling realpath,
	canonicalize_file_name and pathconf.
	[!_WIN32]: Call canonicalize_file_name.
---
 gdb/config.in    |  6 -----
 gdb/configure    |  2 +-
 gdb/configure.ac |  2 +-
 gdb/utils.c      | 68 ++++++--------------------------------------------------
 4 files changed, 9 insertions(+), 69 deletions(-)
  

Comments

Joel Brobecker Nov. 27, 2014, 7:36 a.m. UTC | #1
> 2014-11-26  Yao Qi  <yao@codesourcery.com>
> 
> 	* configure.ac (AC_CHECK_FUNCS): Remove canonicalize_file_name
> 	and realpath.
> 	* config.in: Re-generated.
> 	* configure: Re-generated.
> 	* utils.c (gdb_realpath): Remove code calling realpath,
> 	canonicalize_file_name and pathconf.
> 	[!_WIN32]: Call canonicalize_file_name.

>    /* The MS Windows method.  If we don't have realpath, we assume we
>       don't have symlinks and just canonicalize to a Windows absolute
>       path.  GetFullPath converts ../ and ./ in relative paths to
> @@ -2946,6 +2885,13 @@ gdb_realpath (const char *filename)
>      if (len > 0 && len < MAX_PATH)
>        return xstrdup (buf);
>    }
> +#else
> +  {
> +    char *rp = canonicalize_file_name (filename);
> +
> +    if (rp != NULL)
> +      return rp;
> +  }
>  #endif

I guess one step at a time, but why not use canonicalize_file_name
on Windows hosts as well? My question is not necessarily to suggest
that we should be doing it, but rather whether you know of a reason
why we should not be doing it...
  
Yao Qi Nov. 28, 2014, 3:07 a.m. UTC | #2
Joel Brobecker <brobecker@adacore.com> writes:

> I guess one step at a time, but why not use canonicalize_file_name
> on Windows hosts as well? My question is not necessarily to suggest
> that we should be doing it, but rather whether you know of a reason
> why we should not be doing it...

Hi Joel,
The code handling Windows host was added by your patch below

  [RFA/commit] Improve gdb_realpath for Windows hosts
  https://sourceware.org/ml/gdb-patches/2011-12/msg00785.html

in order to handle a weird case that compiler produces some debug
info where the path has double backslashes.  I am not sure gnulib
realpath is able to handle such case, so I leave the code for Windows
host there.
  
Joel Brobecker Nov. 28, 2014, 3:43 a.m. UTC | #3
Hi Yao,

> > I guess one step at a time, but why not use canonicalize_file_name
> > on Windows hosts as well? My question is not necessarily to suggest
> > that we should be doing it, but rather whether you know of a reason
> > why we should not be doing it...
> 
> The code handling Windows host was added by your patch below
> 
>   [RFA/commit] Improve gdb_realpath for Windows hosts
>   https://sourceware.org/ml/gdb-patches/2011-12/msg00785.html
> 
> in order to handle a weird case that compiler produces some debug
> info where the path has double backslashes.  I am not sure gnulib
> realpath is able to handle such case, so I leave the code for Windows
> host there.

Arf! :-)

canonicalize_file_name does, as far as I can tell, collapse multiple
backslashes, but there is one critical difference between both
functions: GetFullPathName just performs the job mechanically
without checking that the path is at all valid, while
canonicalize_file_name requires the path to exist:

/* Return the canonical absolute name of file NAME.  A canonical name
   does not contain any ".", ".." components nor any repeated file name
   separators ('/') or symlinks.  All components must exist.
   The result is malloc'd.  */

Given how frequent it is for source files to be unavailable,
I would say that it would be useful to preserve the GetFullPathName
behavior for those files as well, and allow users to set breakpoints
without having to worry about how many backslashes GCC has been
using. Or said differently, I agree with your decision :-).

I am taking a note to:

  1. Add an extra comment to this part of the code once you've pushed
     your patch;

  2. Double-check what newer versions of GCC do. At some point, perhaps
     we can declare a minimum GCC version that we fully support and
     then mark this code area for possible removal if it ever starts
     causing extra work.
  
Yao Qi Nov. 28, 2014, 10:43 a.m. UTC | #4
Joel Brobecker <brobecker@adacore.com> writes:

> Given how frequent it is for source files to be unavailable,
> I would say that it would be useful to preserve the GetFullPathName
> behavior for those files as well, and allow users to set breakpoints
> without having to worry about how many backslashes GCC has been
> using. Or said differently, I agree with your decision :-).
>
> I am taking a note to:
>
>   1. Add an extra comment to this part of the code once you've pushed
>      your patch;
>
>   2. Double-check what newer versions of GCC do. At some point, perhaps
>      we can declare a minimum GCC version that we fully support and
>      then mark this code area for possible removal if it ever starts
>      causing extra work.

Great, I've pushed them in.
  

Patch

diff --git a/gdb/config.in b/gdb/config.in
index 8c5a710..fb3c315 100644
--- a/gdb/config.in
+++ b/gdb/config.in
@@ -75,9 +75,6 @@ 
 /* Define to 1 if you have the `btowc' function. */
 #undef HAVE_BTOWC
 
-/* Define to 1 if you have the `canonicalize_file_name' function. */
-#undef HAVE_CANONICALIZE_FILE_NAME
-
 /* Define to 1 if you have the <cursesX.h> header file. */
 #undef HAVE_CURSESX_H
 
@@ -327,9 +324,6 @@ 
 /* Define if Python interpreter is being linked in. */
 #undef HAVE_PYTHON
 
-/* Define to 1 if you have the `realpath' function. */
-#undef HAVE_REALPATH
-
 /* Define to 1 if you have the `resize_term' function. */
 #undef HAVE_RESIZE_TERM
 
diff --git a/gdb/configure b/gdb/configure
index 0c04eba..5f52e91 100755
--- a/gdb/configure
+++ b/gdb/configure
@@ -10478,7 +10478,7 @@  $as_echo "#define HAVE_WORKING_FORK 1" >>confdefs.h
 
 fi
 
-for ac_func in canonicalize_file_name realpath getrusage getuid getgid \
+for ac_func in getrusage getuid getgid \
 		pipe poll pread pread64 pwrite resize_term \
 		sbrk setpgid setpgrp setsid \
 		sigaction sigprocmask sigsetmask socketpair \
diff --git a/gdb/configure.ac b/gdb/configure.ac
index f8c32ad..84232f2 100644
--- a/gdb/configure.ac
+++ b/gdb/configure.ac
@@ -1304,7 +1304,7 @@  AC_C_BIGENDIAN
 
 AC_FUNC_MMAP
 AC_FUNC_VFORK
-AC_CHECK_FUNCS([canonicalize_file_name realpath getrusage getuid getgid \
+AC_CHECK_FUNCS([getrusage getuid getgid \
 		pipe poll pread pread64 pwrite resize_term \
 		sbrk setpgid setpgrp setsid \
 		sigaction sigprocmask sigsetmask socketpair \
diff --git a/gdb/utils.c b/gdb/utils.c
index ff5c00d..b3720f6 100644
--- a/gdb/utils.c
+++ b/gdb/utils.c
@@ -2868,67 +2868,6 @@  string_to_core_addr (const char *my_string)
 char *
 gdb_realpath (const char *filename)
 {
-  /* Method 1: The system has a compile time upper bound on a filename
-     path.  Use that and realpath() to canonicalize the name.  This is
-     the most common case.  Note that, if there isn't a compile time
-     upper bound, you want to avoid realpath() at all costs.  */
-#if defined (HAVE_REALPATH) && defined (PATH_MAX)
-  {
-    char buf[PATH_MAX];
-    const char *rp = realpath (filename, buf);
-
-    if (rp == NULL)
-      rp = filename;
-    return xstrdup (rp);
-  }
-#endif /* HAVE_REALPATH */
-
-  /* Method 2: The host system (i.e., GNU) has the function
-     canonicalize_file_name() which malloc's a chunk of memory and
-     returns that, use that.  */
-#if defined(HAVE_CANONICALIZE_FILE_NAME)
-  {
-    char *rp = canonicalize_file_name (filename);
-
-    if (rp == NULL)
-      return xstrdup (filename);
-    else
-      return rp;
-  }
-#endif
-
-  /* FIXME: cagney/2002-11-13:
-
-     Method 2a: Use realpath() with a NULL buffer.  Some systems, due
-     to the problems described in method 3, have modified their
-     realpath() implementation so that it will allocate a buffer when
-     NULL is passed in.  Before this can be used, though, some sort of
-     configure time test would need to be added.  Otherwize the code
-     will likely core dump.  */
-
-  /* Method 3: Now we're getting desperate!  The system doesn't have a
-     compile time buffer size and no alternative function.  Query the
-     OS, using pathconf(), for the buffer limit.  Care is needed
-     though, some systems do not limit PATH_MAX (return -1 for
-     pathconf()) making it impossible to pass a correctly sized buffer
-     to realpath() (it could always overflow).  On those systems, we
-     skip this.  */
-#if defined (HAVE_REALPATH) && defined (_PC_PATH_MAX)
-  {
-    /* Find out the max path size.  */
-    long path_max = pathconf ("/", _PC_PATH_MAX);
-
-    if (path_max > 0)
-      {
-	/* PATH_MAX is bounded.  */
-	char *buf = alloca (path_max);
-	char *rp = realpath (filename, buf);
-
-	return xstrdup (rp ? rp : filename);
-      }
-  }
-#endif
-
   /* The MS Windows method.  If we don't have realpath, we assume we
      don't have symlinks and just canonicalize to a Windows absolute
      path.  GetFullPath converts ../ and ./ in relative paths to
@@ -2946,6 +2885,13 @@  gdb_realpath (const char *filename)
     if (len > 0 && len < MAX_PATH)
       return xstrdup (buf);
   }
+#else
+  {
+    char *rp = canonicalize_file_name (filename);
+
+    if (rp != NULL)
+      return rp;
+  }
 #endif
 
   /* This system is a lost cause, just dup the buffer.  */