[PATCH 1/2] Windows libibery: Don't quote args unnecessarily

[PATCH 1/2] Windows libibery: Don't quote args unnecessarily

[Ray Donnelly]

We only quote arguments that contain spaces, \n \t \v
or " characters to prevent wasting 2 characters per
argument of the CreateProcess() 32,768 limit.

libiberty/
	* pex-win32.c (argv_to_cmdline): Don't quote
	args unnecessarily
---
 libiberty/ChangeLog   |  5 +++++
 libiberty/pex-win32.c | 48 +++++++++++++++++++++++++++++++++++++++---------
 2 files changed, 44 insertions(+), 9 deletions(-)

diff --git a/libiberty/ChangeLog b/libiberty/ChangeLog
index d9a208b..f6a4f8f 100644
--- a/libiberty/ChangeLog
+++ b/libiberty/ChangeLog
@@ -1,3 +1,8 @@
+2014-04-14 Ray Donnelly <mingw.android@gmail.com>
+
+	* pex-win32.c (argv_to_cmdline): Don't quote
+	args unnecessarily.
+
 2014-04-17  Jakub Jelinek  <jakub@redhat.com>
 
 	PR sanitizer/56781
diff --git a/libiberty/pex-win32.c b/libiberty/pex-win32.c
index eae72c5..775b53c 100644
--- a/libiberty/pex-win32.c
+++ b/libiberty/pex-win32.c
@@ -340,17 +340,26 @@ argv_to_cmdline (char *const *argv)
   char *p;
   size_t cmdline_len;
   int i, j, k;
+  int needs_quotes;
 
   cmdline_len = 0;
   for (i = 0; argv[i]; i++)
     {
-      /* We quote every last argument.  This simplifies the problem;
-	 we need only escape embedded double-quotes and immediately
+      /* We only quote arguments that contain spaces, \n \t \v or " characters
+	 to prevent wasting 2 chars per argument of the CreateProcess 32k char
+	 limit We need only escape embedded double-quotes and immediately
 	 preceeding backslash characters.  A sequence of backslach characters
 	 that is not follwed by a double quote character will not be
 	 escaped.  */
+      needs_quotes = 0;
       for (j = 0; argv[i][j]; j++)
 	{
+	  if (argv[i][j] == ' '  || argv[i][j] == '\n' ||
+	      argv[i][j] == '\t' || argv[i][j] == '"' )
+	    {
+	      needs_quotes = 1;
+	    }
+
 	  if (argv[i][j] == '"')
 	    {
 	      /* Escape preceeding backslashes.  */
@@ -362,16 +371,34 @@ argv_to_cmdline (char *const *argv)
 	}
       /* Trailing backslashes also need to be escaped because they will be
          followed by the terminating quote.  */
-      for (k = j - 1; k >= 0 && argv[i][k] == '\\'; k--)
-	cmdline_len++;
+      if (needs_quotes)
+        {
+          for (k = j - 1; k >= 0 && argv[i][k] == '\\'; k--)
+            cmdline_len++;
+        }
       cmdline_len += j;
-      cmdline_len += 3;  /* for leading and trailing quotes and space */
+      /* for leading and trailing quotes and space */
+      cmdline_len += needs_quotes * 2 + 1;
     }
   cmdline = XNEWVEC (char, cmdline_len);
   p = cmdline;
   for (i = 0; argv[i]; i++)
     {
-      *p++ = '"';
+      needs_quotes = 0;
+      for (j = 0; argv[i][j]; j++)
+        {
+          if (argv[i][j] == ' '  || argv[i][j] == '\n' ||
+              argv[i][j] == '\t' || argv[i][j] == '"' )
+            {
+              needs_quotes = 1;
+              break;
+            }
+        }
+
+      if (needs_quotes)
+        {
+          *p++ = '"';
+        }
       for (j = 0; argv[i][j]; j++)
 	{
 	  if (argv[i][j] == '"')
@@ -382,9 +409,12 @@ argv_to_cmdline (char *const *argv)
 	    }
 	  *p++ = argv[i][j];
 	}
-      for (k = j - 1; k >= 0 && argv[i][k] == '\\'; k--)
-	*p++ = '\\';
-      *p++ = '"';
+      if (needs_quotes)
+        {
+          for (k = j - 1; k >= 0 && argv[i][k] == '\\'; k--)
+            *p++ = '\\';
+          *p++ = '"';
+        }
       *p++ = ' ';
     }
   p[-1] = '\0';
-- 
1.9.2

[PATCH 0/2] Windows: Two improvements.

[Ray Donnelly]

Patch 1 only quotes arguements where quotes are needed as otherwise the
32k limit can be hit sooner than otherwise. This patch should also be
applied to binutils.

Patch 2 makes path-exists semantics behave more like Posix in the face of
../ so that (for example) glibc can be cross compiled on Windows.

Ray Donnelly (2):
  Windows libibery: Don't quote args unnecessarily
  Windows libcpp: Make path-exists semantics more Posix-like

 libcpp/ChangeLog      |  5 +++
 libcpp/files.c        | 86 +++++++++++++++++++++++++++++++++++++++++++++++++++
 libiberty/ChangeLog   |  5 +++
 libiberty/pex-win32.c | 48 ++++++++++++++++++++++------
 4 files changed, 135 insertions(+), 9 deletions(-)

-- 
1.9.2

[PATCH 2/2] Windows libcpp: Make path-exists semantics more Posix-like

[Ray Donnelly]

Windows does a short-circuit lookup of paths containing
../ which means that:

exists/doesnotexist/../file

is considered to exist, while on Posix it is considered
not to. The Posix semantics are relied upon when building
glibc so any paths containing "../" are checked component
wise.

libcpp/
	* files.c (open_file): Implement Posix existence
	semantics for paths containing '../'
---
 libcpp/ChangeLog |  5 ++++
 libcpp/files.c   | 86 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 91 insertions(+)

diff --git a/libcpp/ChangeLog b/libcpp/ChangeLog
index 3a63434..ae1b62a 100644
--- a/libcpp/ChangeLog
+++ b/libcpp/ChangeLog
@@ -1,3 +1,8 @@
+2014-04-14  Ray Donnelly  <mingw.android@gmail.com>
+
+	* files.c (open_file): Implement Posix existence
+	semantics for paths containing '../'
+
 2014-02-24  Walter Lee  <walt@tilera.com>
 
 	* configure.ac: Change "tilepro" triplet to "tilepro*".
diff --git a/libcpp/files.c b/libcpp/files.c
index 7e88778..a9326bf 100644
--- a/libcpp/files.c
+++ b/libcpp/files.c
@@ -30,6 +30,13 @@ along with this program; see the file COPYING3.  If not see
 #include "md5.h"
 #include <dirent.h>
 
+/* Needed for stat_st_mode_symlink below */
+#if defined(_WIN32)
+#  include <windows.h>
+#  define S_IFLNK 0xF000
+#  define S_ISLNK(m) (((m) & S_IFMT) == S_IFLNK)
+#endif
+
 /* Variable length record files on VMS will have a stat size that includes
    record control characters that won't be included in the read size.  */
 #ifdef VMS
@@ -198,6 +205,49 @@ static int pchf_save_compare (const void *e1, const void *e2);
 static int pchf_compare (const void *d_p, const void *e_p);
 static bool check_file_against_entries (cpp_reader *, _cpp_file *, bool);
 
+#if defined(_WIN32)
+
+static int stat_st_mode_symlink (char const* path, struct stat* buf)
+{
+  WIN32_FILE_ATTRIBUTE_DATA attr;
+  memset(buf, 0, sizeof(*buf));
+  int err = GetFileAttributesExA (path, GetFileExInfoStandard, &attr) ? 0 : 1;
+  if (!err)
+    {
+      WIN32_FIND_DATAA finddata;
+      HANDLE h = FindFirstFileA (path, &finddata);
+      if (h != INVALID_HANDLE_VALUE)
+        {
+          FindClose (h);
+          if ((finddata.dwFileAttributes & FILE_ATTRIBUTE_REPARSE_POINT) &&
+              (finddata.dwReserved0 == IO_REPARSE_TAG_SYMLINK))
+              buf->st_mode = S_IFLNK;
+          else if (finddata.dwFileAttributes & FILE_ATTRIBUTE_DIRECTORY)
+              buf->st_mode = S_IFDIR;
+          else if (finddata.dwFileAttributes & FILE_ATTRIBUTE_ARCHIVE)
+              buf->st_mode = S_IFDIR;
+          else
+              buf->st_mode = S_IFREG;
+          buf->st_mode |= S_IREAD;
+          if (!(finddata.dwFileAttributes & FILE_ATTRIBUTE_READONLY))
+              buf->st_mode |= S_IWRITE;
+        }
+      else
+        {
+          buf->st_mode = S_IFDIR;
+        }
+      return 0;
+    }
+  return -1;
+}
+
+#else
+
+#define stat_st_mode_symlink (_name, _buf) stat ((_name), (_buf))
+
+#endif
+
+
 /* Given a filename in FILE->PATH, with the empty string interpreted
    as <stdin>, open it.
 
@@ -227,6 +277,42 @@ open_file (_cpp_file *file)
     }
   else
     file->fd = open (file->path, O_RDONLY | O_NOCTTY | O_BINARY, 0666);
+#if defined(_WIN32) || defined(__CYGWIN__)
+  /* Windows and Posix differ in the face of paths of the form:
+     nonexistantdir/.. in that Posix will return ENOENT whereas
+     Windows won't care that we stepped into a non-existant dir
+     Only do these slow checks if "../" appears in file->path.
+     Cygwin also suffers from the same problem (but doesn't need
+     a new stat function):
+     http://cygwin.com/ml/cygwin/2013-05/msg00222.html
+  */
+  if (file->fd > 0)
+    {
+      char filepath[MAX_PATH];
+      strncpy (filepath, file->path, MAX_PATH);
+      filepath[MAX_PATH-1] = (char) 0;
+      char *dirsep = &filepath[0];
+      while ( (dirsep = strchr (dirsep, '\\')) != NULL)
+        *dirsep = '/';
+      if (strstr(filepath, "../"))
+	{
+	  dirsep = &filepath[0];
+	  /* Check each directory in the chain. */
+	  while ( (dirsep = strchr (dirsep, '/')) != NULL)
+	    {
+	      *dirsep = (char) 0;
+	      if (stat_st_mode_symlink (filepath, &file->st) == -1)
+	        {
+	          *dirsep = '/';
+	          close (file->fd);
+	          file->fd = -1;
+	          return false;
+	        }
+	      *dirsep = '/';
+	    }
+	}
+    }
+#endif
 
   if (file->fd != -1)
     {
-- 
1.9.2

Re: [PATCH 1/2] Windows libibery: Don't quote args unnecessarily

[Kai Tietz]

Hello Ray,

Patches to libiberty need to be cross-posted to binutils, gdb, and gcc ML.  I did so for you now.

----- Original Message -----
> We only quote arguments that contain spaces, \n \t \v
> or " characters to prevent wasting 2 characters per
> argument of the CreateProcess() 32,768 limit.
> 
> libiberty/
> 	* pex-win32.c (argv_to_cmdline): Don't quote
> 	args unnecessarily

The changes to changelog shouldn't be part of the patch itself.  Just write into mail the changelog entry patch needs to have.  Eg as:

Changelog libiberty/
 	* pex-win32.c (argv_to_cmdline): Don't quote
 	args unnecessarily

> ---
>  libiberty/ChangeLog   |  5 +++++
>  libiberty/pex-win32.c | 48 +++++++++++++++++++++++++++++++++++++++---------
>  2 files changed, 44 insertions(+), 9 deletions(-)
> 
> diff --git a/libiberty/ChangeLog b/libiberty/ChangeLog
> index d9a208b..f6a4f8f 100644
> --- a/libiberty/ChangeLog
> +++ b/libiberty/ChangeLog
> @@ -1,3 +1,8 @@
> +2014-04-14 Ray Donnelly <mingw.android@gmail.com>
> +
> +	* pex-win32.c (argv_to_cmdline): Don't quote
> +	args unnecessarily.
> +
>  2014-04-17  Jakub Jelinek  <jakub@redhat.com>
>  
>  	PR sanitizer/56781
> diff --git a/libiberty/pex-win32.c b/libiberty/pex-win32.c
> index eae72c5..775b53c 100644
> --- a/libiberty/pex-win32.c
> +++ b/libiberty/pex-win32.c
> @@ -340,17 +340,26 @@ argv_to_cmdline (char *const *argv)
>    char *p;
>    size_t cmdline_len;
>    int i, j, k;
> +  int needs_quotes;
>  
>    cmdline_len = 0;
>    for (i = 0; argv[i]; i++)
>      {
> -      /* We quote every last argument.  This simplifies the problem;
> -	 we need only escape embedded double-quotes and immediately
> +      /* We only quote arguments that contain spaces, \n \t \v or "
> characters
> +	 to prevent wasting 2 chars per argument of the CreateProcess 32k char
> +	 limit We need only escape embedded double-quotes and immediately
>  	 preceeding backslash characters.  A sequence of backslach characters
>  	 that is not follwed by a double quote character will not be
>  	 escaped.  */
> +      needs_quotes = 0;
>        for (j = 0; argv[i][j]; j++)
>  	{
> +	  if (argv[i][j] == ' '  || argv[i][j] == '\n' ||
> +	      argv[i][j] == '\t' || argv[i][j] == '"' )
> +	    {
Here seems to be an intend issue.
> +	      needs_quotes = 1;
> +	    }
> +
>  	  if (argv[i][j] == '"')
>  	    {
>  	      /* Escape preceeding backslashes.  */
> @@ -362,16 +371,34 @@ argv_to_cmdline (char *const *argv)
>  	}
>        /* Trailing backslashes also need to be escaped because they will be
>           followed by the terminating quote.  */
> -      for (k = j - 1; k >= 0 && argv[i][k] == '\\'; k--)
> -	cmdline_len++;
> +      if (needs_quotes)
> +        {
> +          for (k = j - 1; k >= 0 && argv[i][k] == '\\'; k--)
> +            cmdline_len++;
> +        }
>        cmdline_len += j;
> -      cmdline_len += 3;  /* for leading and trailing quotes and space */
> +      /* for leading and trailing quotes and space */
> +      cmdline_len += needs_quotes * 2 + 1;
>      }
>    cmdline = XNEWVEC (char, cmdline_len);
>    p = cmdline;
>    for (i = 0; argv[i]; i++)
>      {
> -      *p++ = '"';
> +      needs_quotes = 0;
> +      for (j = 0; argv[i][j]; j++)
> +        {
> +          if (argv[i][j] == ' '  || argv[i][j] == '\n' ||
> +              argv[i][j] == '\t' || argv[i][j] == '"' )
> +            {
> +              needs_quotes = 1;
> +              break;
> +            }
> +        }
> +
> +      if (needs_quotes)
> +        {
> +          *p++ = '"';
> +        }
>        for (j = 0; argv[i][j]; j++)
>  	{
>  	  if (argv[i][j] == '"')
> @@ -382,9 +409,12 @@ argv_to_cmdline (char *const *argv)
>  	    }
>  	  *p++ = argv[i][j];
>  	}
> -      for (k = j - 1; k >= 0 && argv[i][k] == '\\'; k--)
> -	*p++ = '\\';
> -      *p++ = '"';
> +      if (needs_quotes)
> +        {
> +          for (k = j - 1; k >= 0 && argv[i][k] == '\\'; k--)
> +            *p++ = '\\';
> +          *p++ = '"';
> +        }
>        *p++ = ' ';
>      }
>    p[-1] = '\0';
> --
> 1.9.2

Patch itself makes sense.  Let see if there are additional comments.

Regards,
Kai

Re: [PATCH 2/2] Windows libcpp: Make path-exists semantics more Posix-like

[Kai Tietz]

Hello Ray,

----- Original Message -----
> Windows does a short-circuit lookup of paths containing
> ../ which means that:
> 
> exists/doesnotexist/../file
> 
> is considered to exist, while on Posix it is considered
> not to. The Posix semantics are relied upon when building
> glibc so any paths containing "../" are checked component
> wise.
> 
> libcpp/
> 	* files.c (open_file): Implement Posix existence
> 	semantics for paths containing '../'
> ---
>  libcpp/ChangeLog |  5 ++++
>  libcpp/files.c   | 86
>  ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 91 insertions(+)
> 
> diff --git a/libcpp/ChangeLog b/libcpp/ChangeLog
> index 3a63434..ae1b62a 100644
> --- a/libcpp/ChangeLog
> +++ b/libcpp/ChangeLog
> @@ -1,3 +1,8 @@
> +2014-04-14  Ray Donnelly  <mingw.android@gmail.com>
> +
> +	* files.c (open_file): Implement Posix existence
> +	semantics for paths containing '../'
> +
>  2014-02-24  Walter Lee  <walt@tilera.com>
>  
>  	* configure.ac: Change "tilepro" triplet to "tilepro*".

As for the other patch, please don't include ChangeLog modifications to the diff.  Instead just write them in mail.

> diff --git a/libcpp/files.c b/libcpp/files.c
> index 7e88778..a9326bf 100644
> --- a/libcpp/files.c
> +++ b/libcpp/files.c
> @@ -30,6 +30,13 @@ along with this program; see the file COPYING3.  If not
> see
>  #include "md5.h"
>  #include <dirent.h>
>  
> +/* Needed for stat_st_mode_symlink below */
> +#if defined(_WIN32)
> +#  include <windows.h>
> +#  define S_IFLNK 0xF000
> +#  define S_ISLNK(m) (((m) & S_IFMT) == S_IFLNK)
> +#endif
> +
>  /* Variable length record files on VMS will have a stat size that includes
>     record control characters that won't be included in the read size.  */
>  #ifdef VMS
> @@ -198,6 +205,49 @@ static int pchf_save_compare (const void *e1, const void
> *e2);
>  static int pchf_compare (const void *d_p, const void *e_p);
>  static bool check_file_against_entries (cpp_reader *, _cpp_file *, bool);
>  
> +#if defined(_WIN32)

This check isn't enough here.  you should check additionally that it isn't cygwin-host (__CYWIN__).

> +
> +static int stat_st_mode_symlink (char const* path, struct stat* buf)
Please break line after 'int' for coding-style.
> +{
> +  WIN32_FILE_ATTRIBUTE_DATA attr;
> +  memset(buf, 0, sizeof(*buf));
> +  int err = GetFileAttributesExA (path, GetFileExInfoStandard, &attr) ? 0 :
> 1;
> +  if (!err)
> +    {
> +      WIN32_FIND_DATAA finddata;
> +      HANDLE h = FindFirstFileA (path, &finddata);
Add an new line here.
> +      if (h != INVALID_HANDLE_VALUE)
> +        {
> +          FindClose (h);
> +          if ((finddata.dwFileAttributes & FILE_ATTRIBUTE_REPARSE_POINT) &&
> +              (finddata.dwReserved0 == IO_REPARSE_TAG_SYMLINK))
> +              buf->st_mode = S_IFLNK;
> +          else if (finddata.dwFileAttributes & FILE_ATTRIBUTE_DIRECTORY)
> +              buf->st_mode = S_IFDIR;
> +          else if (finddata.dwFileAttributes & FILE_ATTRIBUTE_ARCHIVE)
> +              buf->st_mode = S_IFDIR;
> +          else
> +              buf->st_mode = S_IFREG;
> +          buf->st_mode |= S_IREAD;
> +          if (!(finddata.dwFileAttributes & FILE_ATTRIBUTE_READONLY))
> +              buf->st_mode |= S_IWRITE;
> +        }
> +      else
> +        {
> +          buf->st_mode = S_IFDIR;
> +        }
> +      return 0;
> +    }
> +  return -1;
> +}
> +
> +#else
> +
> +#define stat_st_mode_symlink (_name, _buf) stat ((_name), (_buf))
> +
> +#endif

Isn't this function something better placed in libiberty?  Also this name looks a bit confusing.  Wouldn't be a an function calling for _WIN32 case also stat, and just overrides the st_mode member, if it is a link better.  So I would put this function to the file_... API of libiberty.
> +
> +
>  /* Given a filename in FILE->PATH, with the empty string interpreted
>     as <stdin>, open it.
>  
> @@ -227,6 +277,42 @@ open_file (_cpp_file *file)
>      }
>    else
>      file->fd = open (file->path, O_RDONLY | O_NOCTTY | O_BINARY, 0666);
> +#if defined(_WIN32) || defined(__CYGWIN__)
> +  /* Windows and Posix differ in the face of paths of the form:
> +     nonexistantdir/.. in that Posix will return ENOENT whereas
> +     Windows won't care that we stepped into a non-existant dir
> +     Only do these slow checks if "../" appears in file->path.
> +     Cygwin also suffers from the same problem (but doesn't need
> +     a new stat function):
> +     http://cygwin.com/ml/cygwin/2013-05/msg00222.html
> +  */
> +  if (file->fd > 0)
> +    {
> +      char filepath[MAX_PATH];
> +      strncpy (filepath, file->path, MAX_PATH);
> +      filepath[MAX_PATH-1] = (char) 0;
> +      char *dirsep = &filepath[0];
Please add here a new-line.
> +      while ( (dirsep = strchr (dirsep, '\\')) != NULL)
Space after '(' needs to be removed.
> +        *dirsep = '/';
> +      if (strstr(filepath, "../"))
> +	{
> +	  dirsep = &filepath[0];
> +	  /* Check each directory in the chain. */
Comments should end with two spaces.
> +	  while ( (dirsep = strchr (dirsep, '/')) != NULL)
Same as above.  No space after '(' here.
> +	    {
> +	      *dirsep = (char) 0;
this case here looks to me bogus, ... if you want to indicate it is a character then you might want to use here '\0' instead.
> +	      if (stat_st_mode_symlink (filepath, &file->st) == -1)
> +	        {
> +	          *dirsep = '/';
> +	          close (file->fd);
> +	          file->fd = -1;
> +	          return false;
> +	        }
Here should come a newline.
> +	      *dirsep = '/';
> +	    }
> +	}
> +    }
> +#endif
>  
>    if (file->fd != -1)
>      {
> --
> 1.9.2
> 
> 

The implemented logic of this patch makes sense and is IMO ok.  There are some minor coding-style issues you need to correct, and  I would recomment to split this patch into 2 parts (libiberty and libcpp).

Cheers,
Kai

Re: [PATCH 1/2] Windows libibery: Don't quote args unnecessarily

[Eli Zaretskii]

> Date: Sat, 19 Apr 2014 16:23:33 -0400 (EDT)
> From: Kai Tietz <ktietz@redhat.com>
> Cc: gcc-patches@gcc.gnu.org, ktietz70@gmail.com,        "binutils@sourceware.org Development" <binutils@sourceware.org>,        gdb-patches@sourceware.org
> 
> > +      /* We only quote arguments that contain spaces, \n \t \v or " characters
> > +	 to prevent wasting 2 chars per argument of the CreateProcess 32k char
> > +	 limit We need only escape embedded double-quotes and immediately
> >  	 preceeding backslash characters.  A sequence of backslach characters
> >  	 that is not follwed by a double quote character will not be
> >  	 escaped.  */
> > +      needs_quotes = 0;
> >        for (j = 0; argv[i][j]; j++)
> >  	{
> > +	  if (argv[i][j] == ' '  || argv[i][j] == '\n' ||
> > +	      argv[i][j] == '\t' || argv[i][j] == '"' )
> > +	    {
> Here seems to be an intend issue.
> > +	      needs_quotes = 1;
> > +	    }

I think you can omit the \n case, since command arguments on Windows
cannot possibly include newlines.  Also, the comment speaks about \v,
but I see no code to handle that (and am not sure you should bother in
that case as well).

> Patch itself makes sense.

Yes, I agree.

Re: [PATCH 1/2] Windows libibery: Don't quote args unnecessarily

[Joel Brobecker]

> Changelog libiberty/
>  	* pex-win32.c (argv_to_cmdline): Don't quote
>  	args unnecessarily

Some minor comments...

> > diff --git a/libiberty/pex-win32.c b/libiberty/pex-win32.c
> > index eae72c5..775b53c 100644
> > --- a/libiberty/pex-win32.c
> > +++ b/libiberty/pex-win32.c
> > @@ -340,17 +340,26 @@ argv_to_cmdline (char *const *argv)
> >    char *p;
> >    size_t cmdline_len;
> >    int i, j, k;
> > +  int needs_quotes;
> >  
> >    cmdline_len = 0;
> >    for (i = 0; argv[i]; i++)
> >      {
> > -      /* We quote every last argument.  This simplifies the problem;
> > -	 we need only escape embedded double-quotes and immediately
> > +      /* We only quote arguments that contain spaces, \n \t \v or "
> > characters
> > +	 to prevent wasting 2 chars per argument of the CreateProcess 32k char
> > +	 limit We need only escape embedded double-quotes and immediately

Period missing after "limit".  JIC, remember that we ask that periods be
followed by 2 spaces.

> >  	 preceeding backslash characters.  A sequence of backslach characters
> >  	 that is not follwed by a double quote character will not be
> >  	 escaped.  */
> > +      needs_quotes = 0;
> >        for (j = 0; argv[i][j]; j++)
> >  	{
> > +	  if (argv[i][j] == ' '  || argv[i][j] == '\n' ||
> > +	      argv[i][j] == '\t' || argv[i][j] == '"' )

GNU Coding Style requirement: Binary operators should be at the start
of the next line, not at the end. For instance:

	  if (argv[i][j] == ' '  || argv[i][j] == '\n'
              || argv[i][j] == '\t' || argv[i][j] == '"')

Also, watch out for the extra space before ')' - please remove it.


> > +	    {
> Here seems to be an intend issue.
> > +	      needs_quotes = 1;
> > +	    }
> > +
> >  	  if (argv[i][j] == '"')
> >  	    {
> >  	      /* Escape preceeding backslashes.  */
> > @@ -362,16 +371,34 @@ argv_to_cmdline (char *const *argv)
> >  	}
> >        /* Trailing backslashes also need to be escaped because they will be
> >           followed by the terminating quote.  */
> > -      for (k = j - 1; k >= 0 && argv[i][k] == '\\'; k--)
> > -	cmdline_len++;
> > +      if (needs_quotes)
> > +        {
> > +          for (k = j - 1; k >= 0 && argv[i][k] == '\\'; k--)
> > +            cmdline_len++;
> > +        }
> >        cmdline_len += j;
> > -      cmdline_len += 3;  /* for leading and trailing quotes and space */
> > +      /* for leading and trailing quotes and space */
> > +      cmdline_len += needs_quotes * 2 + 1;
> >      }
> >    cmdline = XNEWVEC (char, cmdline_len);
> >    p = cmdline;
> >    for (i = 0; argv[i]; i++)
> >      {
> > -      *p++ = '"';
> > +      needs_quotes = 0;
> > +      for (j = 0; argv[i][j]; j++)
> > +        {
> > +          if (argv[i][j] == ' '  || argv[i][j] == '\n' ||
> > +              argv[i][j] == '\t' || argv[i][j] == '"' )

Same as above.

> > +            {
> > +              needs_quotes = 1;
> > +              break;
> > +            }
> > +        }
> > +
> > +      if (needs_quotes)
> > +        {
> > +          *p++ = '"';
> > +        }
> >        for (j = 0; argv[i][j]; j++)
> >  	{
> >  	  if (argv[i][j] == '"')
> > @@ -382,9 +409,12 @@ argv_to_cmdline (char *const *argv)
> >  	    }
> >  	  *p++ = argv[i][j];
> >  	}
> > -      for (k = j - 1; k >= 0 && argv[i][k] == '\\'; k--)
> > -	*p++ = '\\';
> > -      *p++ = '"';
> > +      if (needs_quotes)
> > +        {
> > +          for (k = j - 1; k >= 0 && argv[i][k] == '\\'; k--)
> > +            *p++ = '\\';
> > +          *p++ = '"';
> > +        }
> >        *p++ = ' ';
> >      }
> >    p[-1] = '\0';
> > --
> > 1.9.2
> 
> Patch itself makes sense.  Let see if there are additional comments.
> 
> Regards,
> Kai

-- 
Joel

Re: [PATCH 2/2] Windows libcpp: Make path-exists semantics more Posix-like

[Pedro Alves]

On 04/19/2014 09:41 PM, Kai Tietz wrote:

> Isn't this function something better placed in libiberty?  Also this name looks a bit confusing.  Wouldn't be a an function calling for _WIN32 case also stat, and just overrides the st_mode member, if it is a link better.  So I would put this function to the file_... API of libiberty.

I'd even suspect that e.g., GNU Make / Makefiles would be likewise affected
by this.  A solution for this in gcc, or in a few selected programs
only, looks brittle to me.  Perhaps it should be mingw itself that provides
a _non-default_ replacement as option (similarly to __mingw_printf).

Can't glibc be changed to not rely on this?  /me hides.

-- 
Pedro Alves

Re: [PATCH 2/2] Windows libcpp: Make path-exists semantics more Posix-like

[Pedro Alves]

On 04/19/2014 08:40 PM, Ray Donnelly wrote:
> Windows does a short-circuit lookup of paths containing
> ../ which means that:
> 
> exists/doesnotexist/../file
> 
> is considered to exist, while on Posix it is considered
> not to. The Posix semantics are relied upon when building
> glibc so any paths containing "../" are checked component
> wise.

This looks like to me the sort of thing that potentially can
break Windows build systems that might rely on the Windows
semantics.

If one's running a native Windows program (such as a native
Windows GCC build), I'd expect that native Windows semantics are
followed (lest we end up reinventing Cygwin within GCC).
At least by default.  IMHO.

On 04/19/2014 08:40 PM, Ray Donnelly wrote:
...
> +     Only do these slow checks if "../" appears in file->path.
> +     Cygwin also suffers from the same problem (but doesn't need
> +     a new stat function):
> +     http://cygwin.com/ml/cygwin/2013-05/msg00222.html

That looks like a bug in Cygwin itself, and thus not appropriate to
fix or reference here.

> +  */
> +  if (file->fd > 0)
> +    {

-- 
Pedro Alves

Re: [PATCH 2/2] Windows libcpp: Make path-exists semantics more Posix-like

[Kai Tietz]

2014-04-25 18:53 GMT+02:00 Pedro Alves <palves@redhat.com>:
> On 04/19/2014 09:41 PM, Kai Tietz wrote:
>
>> Isn't this function something better placed in libiberty?  Also this name looks a bit confusing.  Wouldn't be a an function calling for _WIN32 case also stat, and just overrides the st_mode member, if it is a link better.  So I would put this function to the file_... API of libiberty.
>
> I'd even suspect that e.g., GNU Make / Makefiles would be likewise affected
> by this.  A solution for this in gcc, or in a few selected programs
> only, looks brittle to me.  Perhaps it should be mingw itself that provides
> a _non-default_ replacement as option (similarly to __mingw_printf).

Of course we could change default-behavior of stat-function within
mingw.  This would change documented and exprected behavior of
msvcrt's implementation.  And all this for an assumption made by some
ventures.
I think that libiberty is exactly present to unify functionality (and
API) for different operation systems.  Exactly for this libiberty was
made, isn't it?

I agree that there are other venture, which might be affected by same
problem.  So those venture could either use libiberty to solve this
problem too, or need to reimplement it as they do now.

> Can't glibc be changed to not rely on this?  /me hides.
>
> --
> Pedro Alves

Kai

Re: [PATCH 2/2] Windows libcpp: Make path-exists semantics more Posix-like

[Pedro Alves]

On 04/25/2014 08:05 PM, Kai Tietz wrote:
> 2014-04-25 18:53 GMT+02:00 Pedro Alves <palves@redhat.com>:
>> On 04/19/2014 09:41 PM, Kai Tietz wrote:
>>
>>> Isn't this function something better placed in libiberty?  Also this name looks a bit confusing.  Wouldn't be a an function calling for _WIN32 case also stat, and just overrides the st_mode member, if it is a link better.  So I would put this function to the file_... API of libiberty.
>>
>> I'd even suspect that e.g., GNU Make / Makefiles would be likewise affected
>> by this.  A solution for this in gcc, or in a few selected programs
>> only, looks brittle to me.  Perhaps it should be mingw itself that provides
>> a _non-default_ replacement as option (similarly to __mingw_printf).
> 
> Of course we could change default-behavior of stat-function within
> mingw.

Huh?  I said exactly the opposite.  To expose it as a __non-default__
replacement.  I pointed at __mingw_printf, so to suggest programs
would call it like __mingw_stat or something, or by defining
__USE_MINGW_POSIX_STAT or something, just like one can define
__USE_MINGW_ANSI_STDIO before including stdio.h.  I'll understand
if you wouldn't want to support that as an option, but I did _not_
suggest making it the default.

> This would change documented and exprected behavior of
> msvcrt's implementation.  And all this for an assumption made by some
> ventures.

"some ventures" here must be the whole toolchain.  Certainly the
whole toolchain needs to agree on path handling.  Makefiles were
an obvious example -- if the preprocessor needs needs to find the
right include files, so must Make need this to pick the right
dependencies, isn't it?

This sort of potencially-needing-to-be-fixed-in-many-places issue is
what makes me believe a much better solution would be to just not
rely on this semantics in the first place.

> I think that libiberty is exactly present to unify functionality (and
> API) for different operation systems.  Exactly for this libiberty was
> made, isn't it?

libiberty is actually a kitchen sink, and specific to gcc and src.
It does more than host abstraction.  Gnulib fills that role much better
nowdays.  I'd be nice if gcc used that instead for the host abstraction
parts (gdb does), but nobody's working on that afaik...

> I agree that there are other venture, which might be affected by same
> problem.  So those venture could either use libiberty to solve this
> problem too, or need to reimplement it as they do now.

And then we'll have reinvented Cygwin all over the map.  ;-)

>> Can't glibc be changed to not rely on this?  /me hides.

-- 
Pedro Alves

Re: [PATCH 2/2] Windows libcpp: Make path-exists semantics more Posix-like

[Kai Tietz]

2014-04-25 21:24 GMT+02:00 Pedro Alves <palves@redhat.com>:
> On 04/25/2014 08:05 PM, Kai Tietz wrote:
>> 2014-04-25 18:53 GMT+02:00 Pedro Alves <palves@redhat.com>:
>>> On 04/19/2014 09:41 PM, Kai Tietz wrote:
>>>
>>>> Isn't this function something better placed in libiberty?  Also this name looks a bit confusing.  Wouldn't be a an function calling for _WIN32 case also stat, and just overrides the st_mode member, if it is a link better.  So I would put this function to the file_... API of libiberty.
>>>
>>> I'd even suspect that e.g., GNU Make / Makefiles would be likewise affected
>>> by this.  A solution for this in gcc, or in a few selected programs
>>> only, looks brittle to me.  Perhaps it should be mingw itself that provides
>>> a _non-default_ replacement as option (similarly to __mingw_printf).
>>
>> Of course we could change default-behavior of stat-function within
>> mingw.
>
> Huh?  I said exactly the opposite.  To expose it as a __non-default__
> replacement.  I pointed at __mingw_printf, so to suggest programs
> would call it like __mingw_stat or something, or by defining
> __USE_MINGW_POSIX_STAT or something, just like one can define
> __USE_MINGW_ANSI_STDIO before including stdio.h.  I'll understand
> if you wouldn't want to support that as an option, but I did _not_
> suggest making it the default.

As none-default replacement it makes even less sense in mingw IMO.
the __mingw_printf (and related functions) are there for providing C99
functionality.  What standard shall __mingw_stat satisfy?

>> I think that libiberty is exactly present to unify functionality (and
>> API) for different operation systems.  Exactly for this libiberty was
>> made, isn't it?
>
> libiberty is actually a kitchen sink, and specific to gcc and src.
Well it is shared with binutils.  And in the past gdb used it too (I
think it still does in some way, as not everything is in glib.  I
might be wrong about this).

> It does more than host abstraction.  Gnulib fills that role much better
> nowdays.  I'd be nice if gcc used that instead for the host abstraction
> parts (gdb does), but nobody's working on that afaik...

That's true for gdb.  I don't see that binutils or gcc use glib.  So I
can only talk in this thread about what gcc/binutils uses right now.
Actually I am not really interested in what kind of compatibility
library is used.
Nevertheless to hi-jack this thread to start a discussion about
preferring glib over other (already existing) library in gcc/binutils
isn't ok.  Please start for such a discussion a separate RFC-thread.

>> I agree that there are other venture, which might be affected by same
>> problem.  So those venture could either use libiberty to solve this
>> problem too, or need to reimplement it as they do now.
>
> And then we'll have reinvented Cygwin all over the map.  ;-)

Huh?  mingw != cygwin.  and mingw's internal libraries aren't a
kitchen-sink too.

>>> Can't glibc be changed to not rely on this?  /me hides.
>
> --
> Pedro Alves

---
Kai Tietz

[PATCH] Windows libibery: Don't quote args unnecessarily

[Ray Donnelly]

We only quote arguments that contain spaces, \t or "
characters to prevent wasting 2 characters per
argument of the CreateProcess() 32,768 limit.
---
 libiberty/pex-win32.c | 46 +++++++++++++++++++++++++++++++++++++---------
 1 file changed, 37 insertions(+), 9 deletions(-)

diff --git a/libiberty/pex-win32.c b/libiberty/pex-win32.c
index eae72c5..8b9d4f0 100644
--- a/libiberty/pex-win32.c
+++ b/libiberty/pex-win32.c
@@ -340,17 +340,25 @@ argv_to_cmdline (char *const *argv)
   char *p;
   size_t cmdline_len;
   int i, j, k;
+  int needs_quotes;
 
   cmdline_len = 0;
   for (i = 0; argv[i]; i++)
     {
-      /* We quote every last argument.  This simplifies the problem;
-	 we need only escape embedded double-quotes and immediately
+      /* We only quote arguments that contain spaces, \t or " characters to
+	 prevent wasting 2 chars per argument of the CreateProcess 32k char
+	 limit.  We need only escape embedded double-quotes and immediately
 	 preceeding backslash characters.  A sequence of backslach characters
 	 that is not follwed by a double quote character will not be
 	 escaped.  */
+      needs_quotes = 0;
       for (j = 0; argv[i][j]; j++)
 	{
+	  if (argv[i][j] == ' ' || argv[i][j] == '\t' || argv[i][j] == '"')
+	    {
+	      needs_quotes = 1;
+	    }
+
 	  if (argv[i][j] == '"')
 	    {
 	      /* Escape preceeding backslashes.  */
@@ -362,16 +370,33 @@ argv_to_cmdline (char *const *argv)
 	}
       /* Trailing backslashes also need to be escaped because they will be
          followed by the terminating quote.  */
-      for (k = j - 1; k >= 0 && argv[i][k] == '\\'; k--)
-	cmdline_len++;
+      if (needs_quotes)
+        {
+          for (k = j - 1; k >= 0 && argv[i][k] == '\\'; k--)
+            cmdline_len++;
+        }
       cmdline_len += j;
-      cmdline_len += 3;  /* for leading and trailing quotes and space */
+      /* for leading and trailing quotes and space */
+      cmdline_len += needs_quotes * 2 + 1;
     }
   cmdline = XNEWVEC (char, cmdline_len);
   p = cmdline;
   for (i = 0; argv[i]; i++)
     {
-      *p++ = '"';
+      needs_quotes = 0;
+      for (j = 0; argv[i][j]; j++)
+        {
+          if (argv[i][j] == ' ' || argv[i][j] == '\t' || argv[i][j] == '"')
+            {
+              needs_quotes = 1;
+              break;
+            }
+        }
+
+      if (needs_quotes)
+        {
+          *p++ = '"';
+        }
       for (j = 0; argv[i][j]; j++)
 	{
 	  if (argv[i][j] == '"')
@@ -382,9 +407,12 @@ argv_to_cmdline (char *const *argv)
 	    }
 	  *p++ = argv[i][j];
 	}
-      for (k = j - 1; k >= 0 && argv[i][k] == '\\'; k--)
-	*p++ = '\\';
-      *p++ = '"';
+      if (needs_quotes)
+        {
+          for (k = j - 1; k >= 0 && argv[i][k] == '\\'; k--)
+            *p++ = '\\';
+          *p++ = '"';
+        }
       *p++ = ' ';
     }
   p[-1] = '\0';
-- 
1.9.2

[PATCH] Windows libiberty: Don't quote args unnecessarily (v2)

[Ray Donnelly]

We only quote arguments that contain spaces, \t or "
characters to prevent wasting 2 characters per
argument of the CreateProcess() 32,768 limit.

libiberty/
        * pex-win32.c (argv_to_cmdline): Don't quote
        args unnecessarily

Ray Donnelly (1):
  Windows libibery: Don't quote args unnecessarily

 libiberty/pex-win32.c | 46 +++++++++++++++++++++++++++++++++++++---------
 1 file changed, 37 insertions(+), 9 deletions(-)

-- 
1.9.2

Re: [PATCH] Windows libiberty: Don't quote args unnecessarily (v2)

[Kai Tietz]

2014-05-07 8:55 GMT+02:00 Ray Donnelly <mingw.android@gmail.com>:
> We only quote arguments that contain spaces, \t or "
> characters to prevent wasting 2 characters per
> argument of the CreateProcess() 32,768 limit.
>
> libiberty/
>         * pex-win32.c (argv_to_cmdline): Don't quote
>         args unnecessarily
>
> Ray Donnelly (1):
>   Windows libibery: Don't quote args unnecessarily
>
>  libiberty/pex-win32.c | 46 +++++++++++++++++++++++++++++++++++++---------
>  1 file changed, 37 insertions(+), 9 deletions(-)
>
> --
> 1.9.2
>

Did you missed to attach patch?  Or is that my mail-client?

Kai

Re: [PATCH] Windows libibery: Don't quote args unnecessarily

[Kai Tietz]

Ah, that was my mail-client.

Patch is ok IMO.  I add Ian CC for a second look at it.

Cheers,
Kai

Re: [PATCH] Windows libibery: Don't quote args unnecessarily

[Ian Lance Taylor]

On Tue, May 6, 2014 at 11:56 PM, Ray Donnelly <mingw.android@gmail.com> wrote:
> We only quote arguments that contain spaces, \t or "
> characters to prevent wasting 2 characters per
> argument of the CreateProcess() 32,768 limit.

This is OK.

Thanks.

Ian


>  libiberty/pex-win32.c | 46 +++++++++++++++++++++++++++++++++++++---------
>  1 file changed, 37 insertions(+), 9 deletions(-)
>
> diff --git a/libiberty/pex-win32.c b/libiberty/pex-win32.c
> index eae72c5..8b9d4f0 100644
> --- a/libiberty/pex-win32.c
> +++ b/libiberty/pex-win32.c
> @@ -340,17 +340,25 @@ argv_to_cmdline (char *const *argv)
>    char *p;
>    size_t cmdline_len;
>    int i, j, k;
> +  int needs_quotes;
>
>    cmdline_len = 0;
>    for (i = 0; argv[i]; i++)
>      {
> -      /* We quote every last argument.  This simplifies the problem;
> -        we need only escape embedded double-quotes and immediately
> +      /* We only quote arguments that contain spaces, \t or " characters to
> +        prevent wasting 2 chars per argument of the CreateProcess 32k char
> +        limit.  We need only escape embedded double-quotes and immediately
>          preceeding backslash characters.  A sequence of backslach characters
>          that is not follwed by a double quote character will not be
>          escaped.  */
> +      needs_quotes = 0;
>        for (j = 0; argv[i][j]; j++)
>         {
> +         if (argv[i][j] == ' ' || argv[i][j] == '\t' || argv[i][j] == '"')
> +           {
> +             needs_quotes = 1;
> +           }
> +
>           if (argv[i][j] == '"')
>             {
>               /* Escape preceeding backslashes.  */
> @@ -362,16 +370,33 @@ argv_to_cmdline (char *const *argv)
>         }
>        /* Trailing backslashes also need to be escaped because they will be
>           followed by the terminating quote.  */
> -      for (k = j - 1; k >= 0 && argv[i][k] == '\\'; k--)
> -       cmdline_len++;
> +      if (needs_quotes)
> +        {
> +          for (k = j - 1; k >= 0 && argv[i][k] == '\\'; k--)
> +            cmdline_len++;
> +        }
>        cmdline_len += j;
> -      cmdline_len += 3;  /* for leading and trailing quotes and space */
> +      /* for leading and trailing quotes and space */
> +      cmdline_len += needs_quotes * 2 + 1;
>      }
>    cmdline = XNEWVEC (char, cmdline_len);
>    p = cmdline;
>    for (i = 0; argv[i]; i++)
>      {
> -      *p++ = '"';
> +      needs_quotes = 0;
> +      for (j = 0; argv[i][j]; j++)
> +        {
> +          if (argv[i][j] == ' ' || argv[i][j] == '\t' || argv[i][j] == '"')
> +            {
> +              needs_quotes = 1;
> +              break;
> +            }
> +        }
> +
> +      if (needs_quotes)
> +        {
> +          *p++ = '"';
> +        }
>        for (j = 0; argv[i][j]; j++)
>         {
>           if (argv[i][j] == '"')
> @@ -382,9 +407,12 @@ argv_to_cmdline (char *const *argv)
>             }
>           *p++ = argv[i][j];
>         }
> -      for (k = j - 1; k >= 0 && argv[i][k] == '\\'; k--)
> -       *p++ = '\\';
> -      *p++ = '"';
> +      if (needs_quotes)
> +        {
> +          for (k = j - 1; k >= 0 && argv[i][k] == '\\'; k--)
> +            *p++ = '\\';
> +          *p++ = '"';
> +        }
>        *p++ = ' ';
>      }
>    p[-1] = '\0';
> --
> 1.9.2
>

Re: [PATCH 2/2] Windows libcpp: Make path-exists semantics more Posix-like

[Ray Donnelly]

On Fri, Apr 25, 2014 at 9:33 PM, Kai Tietz <ktietz70@googlemail.com> wrote:
> 2014-04-25 21:24 GMT+02:00 Pedro Alves <palves@redhat.com>:
>> On 04/25/2014 08:05 PM, Kai Tietz wrote:
>>> 2014-04-25 18:53 GMT+02:00 Pedro Alves <palves@redhat.com>:
>>>> On 04/19/2014 09:41 PM, Kai Tietz wrote:
>>>>
>>>>> Isn't this function something better placed in libiberty?  Also this name looks a bit confusing.  Wouldn't be a an function calling for _WIN32 case also stat, and just overrides the st_mode member, if it is a link better.  So I would put this function to the file_... API of libiberty.
>>>>
>>>> I'd even suspect that e.g., GNU Make / Makefiles would be likewise affected
>>>> by this.  A solution for this in gcc, or in a few selected programs
>>>> only, looks brittle to me.  Perhaps it should be mingw itself that provides
>>>> a _non-default_ replacement as option (similarly to __mingw_printf).
>>>
>>> Of course we could change default-behavior of stat-function within
>>> mingw.
>>
>> Huh?  I said exactly the opposite.  To expose it as a __non-default__
>> replacement.  I pointed at __mingw_printf, so to suggest programs
>> would call it like __mingw_stat or something, or by defining
>> __USE_MINGW_POSIX_STAT or something, just like one can define
>> __USE_MINGW_ANSI_STDIO before including stdio.h.  I'll understand
>> if you wouldn't want to support that as an option, but I did _not_
>> suggest making it the default.
>
> As none-default replacement it makes even less sense in mingw IMO.
> the __mingw_printf (and related functions) are there for providing C99
> functionality.  What standard shall __mingw_stat satisfy?
>
>>> I think that libiberty is exactly present to unify functionality (and
>>> API) for different operation systems.  Exactly for this libiberty was
>>> made, isn't it?
>>
>> libiberty is actually a kitchen sink, and specific to gcc and src.
> Well it is shared with binutils.  And in the past gdb used it too (I
> think it still does in some way, as not everything is in glib.  I
> might be wrong about this).
>
>> It does more than host abstraction.  Gnulib fills that role much better
>> nowdays.  I'd be nice if gcc used that instead for the host abstraction
>> parts (gdb does), but nobody's working on that afaik...
>
> That's true for gdb.  I don't see that binutils or gcc use glib.  So I
> can only talk in this thread about what gcc/binutils uses right now.
> Actually I am not really interested in what kind of compatibility
> library is used.
> Nevertheless to hi-jack this thread to start a discussion about
> preferring glib over other (already existing) library in gcc/binutils
> isn't ok.  Please start for such a discussion a separate RFC-thread.
>
>>> I agree that there are other venture, which might be affected by same
>>> problem.  So those venture could either use libiberty to solve this
>>> problem too, or need to reimplement it as they do now.
>>
>> And then we'll have reinvented Cygwin all over the map.  ;-)
>
> Huh?  mingw != cygwin.  and mingw's internal libraries aren't a
> kitchen-sink too.
>
>>>> Can't glibc be changed to not rely on this?  /me hides.

Yes mingw != cygwin, but we'd still like to be able to cross compile
glibc on Windows targeting GNU/Linux and this patch is necessary for
that to work. I see benefits in normalizing the behavior over three
build machine OSes (GNU/Linux, OS X and WIndows) and for all projects
being built (rather than just changing glibc). If I hear any positive
noises, I'll send a re-based version of the patch.

--
Ray

>>
>> --
>> Pedro Alves
>
> ---
> Kai Tietz