Re: [PATCH 1/2] Windows libibery: Don't quote args unnecessarily
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 1/2] Windows libibery: Don't quote args unnecessarily
Date: Sat, 19 Apr 2014 16:23:33 -0400 (EDT) From: Kai Tietz kti...@redhat.com Cc: gcc-patches@gcc.gnu.org, ktiet...@gmail.com, binut...@sourceware.org Development binut...@sourceware.org, gdb-patc...@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.
[PATCH 1/2] Windows libibery: Don't quote args unnecessarily
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.andr...@gmail.com + + * pex-win32.c (argv_to_cmdline): Don't quote + args unnecessarily. + 2014-04-17 Jakub Jelinek ja...@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
Re: [PATCH 1/2] Windows libibery: Don't quote args unnecessarily
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.andr...@gmail.com + + * pex-win32.c (argv_to_cmdline): Don't quote + args unnecessarily. + 2014-04-17 Jakub Jelinek ja...@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