Re: [PATCH] fixincludes: fix fixincludes for MinGW

[Jeff Law]

On 09/30/2016 10:18 AM, Bruce Korb wrote:

Hi Tadek,

Looks good to me.  Thank you.
Clear to send (push).

Committed on behalf of Tadek.

Thanks,
jeff

Re: [PATCH] fixincludes: fix fixincludes for MinGW

[Bruce Korb]

Hi Tadek,

Looks good to me.  Thank you.
Clear to send (push).

Re: [PATCH] fixincludes: fix fixincludes for MinGW

[Tadek Kijkowski]

The fixincl executable uses system function to call applyfix or to
direcly patch a header file, with parameters enclosed in single
quotes. The problem is that MinGW system function just calls cmd.exe,
which doesn't strip quotes from parameters and completely ignores
quotes for embedding spaces in parameters. The MinGW system function
also doesn't allow embedding newlines in executed command parameters.
As a result fixincludes doesn't work at all when trying to build a cross
compiler with mingw as host.

On MinGW host, this patch adds system_with_shell function, which
transforms command passed to system function is following way:
* Enclose entire command in double quotes
* Prepend shell executable name, taken from environment variable SHELL
* Replace each occurence of newline with '$'\n'' sequence which is
understood by bash and ksh
* Escape special characters

Second attempt:

Moved system_with_shell to fixlib.c. Escape some special characters
located between single quotes as enclosing single-quoted string in double
quotes nullifies single quotes. Fixed passing file name to apply_fix.
Fixed "test" tests and "shell" fixes, also for DJGPP.

Convert line all endings to unix in check.sh to avoid false positives.

Followed Jeff's and Bruce's advices:
Fixed formatting.
Call system_with_shell explicitly. If it's not needed it just expands
to "system".

On MinGW (mingw64 actually) all fixinclude tests pass.
On DJGPP all but two tests pass - if you manage to get it to compile at all.

2016-09-30  Tadek Kijkowski  

* check.tpl: Convert line endings to unix on test outputs
* fixfixes.c: Fixed passing file name to apply_fix when
SEPARATE_FIX_PROC is defined
* fixincl.c: Use system_with_shell, fixes for MinGW and DJGPP
* fixlib.c, fixlib.h: Added system_with_shell and fix_path_separators

Index: fixincludes/check.tpl
===
--- fixincludes/check.tpl (revision 240643)
+++ fixincludes/check.tpl (working copy)
@@ -123,6 +123,11 @@
 exec < ${TESTDIR}/LIST
 while read f
 do
+  if [ -n "$MSYSTEM" -o -n "$DJGPP" ]
+  then
+# On MinGW and DJGPP convert line endings to avoid false positives
+mv $f $f.dos; tr -d '\r' < $f.dos > $f; rm $f.dos
+  fi
   if [ ! -f ${TESTBASE}/$f ]
   then
 echo "Newly fixed header:  $f" >&2
Index: fixincludes/fixfixes.c
===
--- fixincludes/fixfixes.c (revision 240643)
+++ fixincludes/fixfixes.c (working copy)
@@ -790,7 +790,8 @@
   return EXIT_FAILURE;
 }

-  apply_fix (pFix, argv[1]);
+  /* Second parameter of apply_fix is file name */
+  apply_fix (pFix, argv[2]);
   fclose (stdout);
   fclose (stdin);
   unlink (argv[4]);
Index: fixincludes/fixincl.c
===
--- fixincludes/fixincl.c (revision 240643)
+++ fixincludes/fixincl.c (working copy)
@@ -74,9 +74,12 @@
 #endif /* DO_STATS */

 const char incl_quote_pat[] = "^[ \t]*#[ \t]*include[ \t]*\"[^/]";
-tSCC z_fork_err[] = "Error %d (%s) starting filter process for %s\n";
 regex_t incl_quote_re;

+#ifndef SEPARATE_FIX_PROC
+tSCC z_fork_err[] = "Error %d (%s) starting filter process for %s\n";
+#endif
+
 static void do_version (void) ATTRIBUTE_NORETURN;
 char *load_file (const char *);
 void run_compiles (void);
@@ -188,7 +191,7 @@
   puts (zBuf + 5);
   exit (strcmp (run_shell (zBuf), program_id));
 #else
-  exit (system (zBuf));
+  exit (system_with_shell (zBuf));
 #endif
 }

@@ -275,6 +278,11 @@
   /* NULL as the first argument to `tempnam' causes it to DTRT
  wrt the temporary directory where the file will be created.  */
   pz_temp_file = tempnam( NULL, "fxinc" );
+
+#if defined(__MINGW32__)
+  fix_path_separators (pz_temp_file);
+#endif
+
 # endif

   signal (SIGQUIT, SIG_IGN);
@@ -566,7 +574,27 @@
   free ((void *) pz_res);
   return res;
 }
+#elif defined(__MINGW32__) || defined(__DJGPP__)
+static int
+test_test (tTestDesc* p_test, char* pz_test_file)
+{
+  tSCC cmd_fmt[] =
+#if defined(__DJGPP__)
+"file=%s; test %s >/dev/null 2>/dev/null";
 #else
+"file=%s; test %s > /dev/null 2>&1";
+#endif
+  int res;
+
+  char *cmd_buf = XNEWVEC (char, strlen(cmd_fmt) +
strlen(pz_test_file) + strlen(p_test->pz_test_text));
+
+  sprintf (cmd_buf, cmd_fmt, pz_test_file, p_test->pz_test_text);
+  res = system_with_shell (cmd_buf);
+
+  free (cmd_buf);
+  return res ? SKIP_FIX : APPLY_FIX;
+}
+#else
 /*
  *  IF we are in MS-DOS land, then whatever shell-type test is required
  *  will, by definition, fail
@@ -887,7 +915,7 @@
   else /* NOT an "internal" fix: */
 {
   size_t parg_size;
-#ifdef __MSDOS__
+#if defined(__MSDOS__) && !defined(__DJGPP__)
   /* Don't use the "src > dstX; rm -f dst; mv -f dstX dst" trick:
  dst is a temporary file anyway, so we know there's no other
  file by that name; and DOS's system(3) doesn't mind to
@@ -906,6 +934,8 @@
 

Re: [PATCH] fixincludes: fix fixincludes for MinGW

[Tadek Kijkowski]

Hold on. I have much improved version almost ready. It passes all the tests now.

2016-09-30 0:15 GMT+02:00 Bruce Korb :
> I usually try to catch emails with "fixincludes" in the title.
> Can I please get a copy of the original patch?  Thanks.
>
>
> On 09/29/16 11:44, Jeff Law wrote:
>>
>> On 09/22/2016 11:26 PM, Tadek Kijkowski wrote:
>>>
>>> The fixincl executable uses system function to call applyfix or to
>>> direcly patch a header file, with parameters enclosed in single
>>> quotes. This problem is that MinGW system function just calls cmd.exe,
>>> which doesn't strip quotes from parameters and completely ignores
>>> quotes for embedding spaces in parameters. The MinGW system function
>>> also doesn't allow for newlines in executed command parameters. As a
>>> result fixincludes doesn't wotk at on when trying to build a cross
>>> compiler with mingw as host.
>>>
>>> On MinGW host, this patch adds system_with_shell function, which
>>> transforms command passed to system function is following way:
>>> - Enclose entire command in double quotes
>>> - Prepend shell executable name, taken from environment variable SHELL
>>> - Replace each occurence of newline with '$'\n'' sequence which is
>>> understood by bash and ksh (it is assumed that all newlines are
>>> embedded in command parameters enclosed in single quotes)
>>>
>>> 2016-09-23 Tadek Kijkowski (tkijkow...@gmail.com)
>>>
>>> * fixincl.c: Added system_with_shell for MinGW host.
>>>
>>>
>>> Index: fixincludes/fixincl.c
>>> ===
>>> --- fixincludes/fixincl.c   (revision 240386)
>>> +++ fixincludes/fixincl.c   (working copy)
>>> @@ -170,7 +170,111 @@
>>>exit (EXIT_SUCCESS);
>>>  }
>>>
>>> +#ifdef __MINGW32__
>>>
>>> +/* Count number of \c needle character instances in string */
>>> +static int
>>> +count_chars ( char* str, char needle )
>>
>> Formatting it.  This should be:
>>
>> count_chars (char* str, char needle)
>>
>> Note the lack of horizontal whitespace after the open paren or before
>> the close paren.  Similarly for system_with_shell declaration below.
>>
>> Wouldn't this be better named "count_occurrences_of_char" or
>> "count_instances_of_char"?
>>
>>
>>
>>
>>> +
>>> +  /* Allocate enough memory to fit newly created command string */
>>> +  cmd_size = strlen (env_shell) + (sizeof (z_shell_start_args) - 1)
>>> +   + strlen (s) + newline_cnt * (sizeof(z_shell_newline) - 1
>>> - 1)
>>> +   + (sizeof (z_shell_end_args) - 1) + 1;
>>
>> Why use sizeof (foo) - 1 rather than just strlen (foo) here?  Note that
>> GCC can compute the string length as a compile time constant, so you're
>> not gaining any performance by using sizeof here and strlen seems clearer.
>>
>>
>>
>>> +
>>> +  long_cmd = XNEWVEC (char, cmd_size);
>>> +
>>> +  /* Start with ${SHELL} -c " */
>>> +  strcpy (long_cmd, env_shell);
>>> +  strcat (long_cmd, z_shell_start_args);
>>> +
>>> +  /* End pointer for appending pieces of command while replacing
>>> newlines */
>>> +  cmd_endp = long_cmd + strlen(long_cmd);
>>
>> Formatting nit.  Space between function name and its argument list.
>>
>>
>>> +  nl_scan = s;
>>> +  while (nl_scan != NULL)
>>> +{
>>> +  char* next_nl = strchr (nl_scan, '\n');
>>
>> Formatting nit.  char *next_nl.
>>
>>
>>> +  if (next_nl)
>>> +{
>>> +  /* Append part of command between newlines */
>>> +  size_t part_len = next_nl - nl_scan;
>>> +  memcpy(cmd_endp, nl_scan, part_len);
>>
>> Formatting nit, space between function name and its argument list.
>>
>>> +  cmd_endp += part_len;
>>> +
>>> +  /* Append newline replacement */
>>> +  memcpy(cmd_endp, z_shell_newline, sizeof(z_shell_newline));
>>
>> Smilarly, space between functio nname and its argument list.
>>
>>> +  cmd_endp += sizeof(z_shell_newline) - 1;
>>
>> Here too.
>>
>>> +
>>> +  free ( (void*) long_cmd);
>>
>> free (long_cmd);
>>
>> Can you fix those various (minor) issue and resubmit.  I think it'll be
>> OK for the trunk with those changes.
>>
>> jeff
>>
>>
>

Re: [PATCH] fixincludes: fix fixincludes for MinGW

[Bruce Korb]

OK, I found it.  Looks like my MUA is getting too aggressive with its filtering.
What Jeff said, plus I would prefer the tail end looking like:

+
+#else
+
+#define system_with_shell system // normal call
+
+#endif /* defined(__MINGW32__) */

and modifying the call to use "system_with_shell".
The point being that obvious clues are better than subtle switcheroos.

On Thu, Sep 29, 2016 at 11:44 AM, Jeff Law  wrote:

Re: [PATCH] fixincludes: fix fixincludes for MinGW

[Bruce Korb]

I usually try to catch emails with "fixincludes" in the title.
Can I please get a copy of the original patch?  Thanks.

On 09/29/16 11:44, Jeff Law wrote:

On 09/22/2016 11:26 PM, Tadek Kijkowski wrote:

The fixincl executable uses system function to call applyfix or to
direcly patch a header file, with parameters enclosed in single
quotes. This problem is that MinGW system function just calls cmd.exe,
which doesn't strip quotes from parameters and completely ignores
quotes for embedding spaces in parameters. The MinGW system function
also doesn't allow for newlines in executed command parameters. As a
result fixincludes doesn't wotk at on when trying to build a cross
compiler with mingw as host.

On MinGW host, this patch adds system_with_shell function, which
transforms command passed to system function is following way:
- Enclose entire command in double quotes
- Prepend shell executable name, taken from environment variable SHELL
- Replace each occurence of newline with '$'\n'' sequence which is
understood by bash and ksh (it is assumed that all newlines are
embedded in command parameters enclosed in single quotes)

2016-09-23 Tadek Kijkowski (tkijkow...@gmail.com)

* fixincl.c: Added system_with_shell for MinGW host.


Index: fixincludes/fixincl.c
===
--- fixincludes/fixincl.c   (revision 240386)
+++ fixincludes/fixincl.c   (working copy)
@@ -170,7 +170,111 @@
   exit (EXIT_SUCCESS);
 }

+#ifdef __MINGW32__

+/* Count number of \c needle character instances in string */
+static int
+count_chars ( char* str, char needle )

Formatting it.  This should be:

count_chars (char* str, char needle)

Note the lack of horizontal whitespace after the open paren or before
the close paren.  Similarly for system_with_shell declaration below.

Wouldn't this be better named "count_occurrences_of_char" or
"count_instances_of_char"?





+
+  /* Allocate enough memory to fit newly created command string */
+  cmd_size = strlen (env_shell) + (sizeof (z_shell_start_args) - 1)
+   + strlen (s) + newline_cnt * (sizeof(z_shell_newline) - 1
- 1)
+   + (sizeof (z_shell_end_args) - 1) + 1;

Why use sizeof (foo) - 1 rather than just strlen (foo) here?  Note that
GCC can compute the string length as a compile time constant, so you're
not gaining any performance by using sizeof here and strlen seems clearer.




+
+  long_cmd = XNEWVEC (char, cmd_size);
+
+  /* Start with ${SHELL} -c " */
+  strcpy (long_cmd, env_shell);
+  strcat (long_cmd, z_shell_start_args);
+
+  /* End pointer for appending pieces of command while replacing
newlines */
+  cmd_endp = long_cmd + strlen(long_cmd);

Formatting nit.  Space between function name and its argument list.



+  nl_scan = s;
+  while (nl_scan != NULL)
+{
+  char* next_nl = strchr (nl_scan, '\n');

Formatting nit.  char *next_nl.



+  if (next_nl)
+{
+  /* Append part of command between newlines */
+  size_t part_len = next_nl - nl_scan;
+  memcpy(cmd_endp, nl_scan, part_len);

Formatting nit, space between function name and its argument list.


+  cmd_endp += part_len;
+
+  /* Append newline replacement */
+  memcpy(cmd_endp, z_shell_newline, sizeof(z_shell_newline));

Smilarly, space between functio nname and its argument list.


+  cmd_endp += sizeof(z_shell_newline) - 1;

Here too.


+
+  free ( (void*) long_cmd);

free (long_cmd);

Can you fix those various (minor) issue and resubmit.  I think it'll be
OK for the trunk with those changes.

jeff

Re: [PATCH] fixincludes: fix fixincludes for MinGW

[Jeff Law]

On 09/22/2016 11:26 PM, Tadek Kijkowski wrote:

The fixincl executable uses system function to call applyfix or to
direcly patch a header file, with parameters enclosed in single
quotes. This problem is that MinGW system function just calls cmd.exe,
which doesn't strip quotes from parameters and completely ignores
quotes for embedding spaces in parameters. The MinGW system function
also doesn't allow for newlines in executed command parameters. As a
result fixincludes doesn't wotk at on when trying to build a cross
compiler with mingw as host.

On MinGW host, this patch adds system_with_shell function, which
transforms command passed to system function is following way:
- Enclose entire command in double quotes
- Prepend shell executable name, taken from environment variable SHELL
- Replace each occurence of newline with '$'\n'' sequence which is
understood by bash and ksh (it is assumed that all newlines are
embedded in command parameters enclosed in single quotes)

2016-09-23 Tadek Kijkowski (tkijkow...@gmail.com)

* fixincl.c: Added system_with_shell for MinGW host.


Index: fixincludes/fixincl.c
===
--- fixincludes/fixincl.c   (revision 240386)
+++ fixincludes/fixincl.c   (working copy)
@@ -170,7 +170,111 @@
   exit (EXIT_SUCCESS);
 }

+#ifdef __MINGW32__

+/* Count number of \c needle character instances in string */
+static int
+count_chars ( char* str, char needle )

Formatting it.  This should be:

count_chars (char* str, char needle)

Note the lack of horizontal whitespace after the open paren or before 
the close paren.  Similarly for system_with_shell declaration below.


Wouldn't this be better named "count_occurrences_of_char" or 
"count_instances_of_char"?






+
+  /* Allocate enough memory to fit newly created command string */
+  cmd_size = strlen (env_shell) + (sizeof (z_shell_start_args) - 1)
+   + strlen (s) + newline_cnt * (sizeof(z_shell_newline) - 1 - 1)
+   + (sizeof (z_shell_end_args) - 1) + 1;
Why use sizeof (foo) - 1 rather than just strlen (foo) here?  Note that 
GCC can compute the string length as a compile time constant, so you're 
not gaining any performance by using sizeof here and strlen seems clearer.





+
+  long_cmd = XNEWVEC (char, cmd_size);
+
+  /* Start with ${SHELL} -c " */
+  strcpy (long_cmd, env_shell);
+  strcat (long_cmd, z_shell_start_args);
+
+  /* End pointer for appending pieces of command while replacing newlines */
+  cmd_endp = long_cmd + strlen(long_cmd);

Formatting nit.  Space between function name and its argument list.



+  nl_scan = s;
+  while (nl_scan != NULL)
+{
+  char* next_nl = strchr (nl_scan, '\n');

Formatting nit.  char *next_nl.



+  if (next_nl)
+{
+  /* Append part of command between newlines */
+  size_t part_len = next_nl - nl_scan;
+  memcpy(cmd_endp, nl_scan, part_len);

Formatting nit, space between function name and its argument list.


+  cmd_endp += part_len;
+
+  /* Append newline replacement */
+  memcpy(cmd_endp, z_shell_newline, sizeof(z_shell_newline));

Smilarly, space between functio nname and its argument list.


+  cmd_endp += sizeof(z_shell_newline) - 1;

Here too.


+
+  free ( (void*) long_cmd);

free (long_cmd);

Can you fix those various (minor) issue and resubmit.  I think it'll be 
OK for the trunk with those changes.


jeff

[PATCH] fixincludes: fix fixincludes for MinGW

[Tadek Kijkowski]

The fixincl executable uses system function to call applyfix or to
direcly patch a header file, with parameters enclosed in single
quotes. This problem is that MinGW system function just calls cmd.exe,
which doesn't strip quotes from parameters and completely ignores
quotes for embedding spaces in parameters. The MinGW system function
also doesn't allow for newlines in executed command parameters. As a
result fixincludes doesn't wotk at on when trying to build a cross
compiler with mingw as host.

On MinGW host, this patch adds system_with_shell function, which
transforms command passed to system function is following way:
- Enclose entire command in double quotes
- Prepend shell executable name, taken from environment variable SHELL
- Replace each occurence of newline with '$'\n'' sequence which is
understood by bash and ksh (it is assumed that all newlines are
embedded in command parameters enclosed in single quotes)

2016-09-23 Tadek Kijkowski (tkijkow...@gmail.com)

* fixincl.c: Added system_with_shell for MinGW host.


Index: fixincludes/fixincl.c
===
--- fixincludes/fixincl.c   (revision 240386)
+++ fixincludes/fixincl.c   (working copy)
@@ -170,7 +170,111 @@
   exit (EXIT_SUCCESS);
 }

+#ifdef __MINGW32__

+/* Count number of \c needle character instances in string */
+static int
+count_chars ( char* str, char needle )
+{
+  int instances = 0;
+
+  while (str)
+{
+   str = strchr (str, needle);
+   if (str)
+ {
+   ++str;
+   ++instances;
+ }
+}
+
+  return instances;
+}
+
+/* On Mingw32 system(3) will just start cmd by default.
+   Try using unix style shell passed via SHELL env. variable.
+ */
+
+/* Call system(3) function, but prepend ${SHELL} -c to the command,
+   replace newlines with '$'\n'' and enclose command with double quotes.
+ */
+static int
+system_with_shell ( char* s )
+{
+  static const char z_shell_start_args[] = " -c \"";
+  static const char z_shell_end_args[] = "\"";
+  static const char z_shell_newline[] = "'$'\\n''";
+
+  char* env_shell;
+  char* long_cmd;
+  char* cmd_endp;
+  size_t cmd_size;
+  int sys_result;
+  int newline_cnt;
+  char* nl_scan;
+
+  /* If SHELL variable is not defined just call standard shell function */
+  env_shell = getenv ("SHELL");
+  if (env_shell == NULL)
+return system (s);
+
+  /* Count number of newlines in command */
+  newline_cnt = count_chars(s, '\n');
+
+  /* Allocate enough memory to fit newly created command string */
+  cmd_size = strlen (env_shell) + (sizeof (z_shell_start_args) - 1)
+   + strlen (s) + newline_cnt * (sizeof(z_shell_newline) - 1 - 1)
+   + (sizeof (z_shell_end_args) - 1) + 1;
+
+  long_cmd = XNEWVEC (char, cmd_size);
+
+  /* Start with ${SHELL} -c " */
+  strcpy (long_cmd, env_shell);
+  strcat (long_cmd, z_shell_start_args);
+
+  /* End pointer for appending pieces of command while replacing newlines */
+  cmd_endp = long_cmd + strlen(long_cmd);
+  nl_scan = s;
+  while (nl_scan != NULL)
+{
+  char* next_nl = strchr (nl_scan, '\n');
+  if (next_nl)
+{
+  /* Append part of command between newlines */
+  size_t part_len = next_nl - nl_scan;
+  memcpy(cmd_endp, nl_scan, part_len);
+  cmd_endp += part_len;
+
+  /* Append newline replacement */
+  memcpy(cmd_endp, z_shell_newline, sizeof(z_shell_newline));
+  cmd_endp += sizeof(z_shell_newline) - 1;
+
+  /* Skip newline in src */
+  ++next_nl;
+}
+  else
+{
+  /* Last portion */
+  strcpy (cmd_endp, nl_scan);
+}
+  nl_scan = next_nl;
+}
+
+  /* Closing quote */
+  strcat (long_cmd, z_shell_end_args);
+
+  sys_result = system (long_cmd);
+
+  free ( (void*) long_cmd);
+
+  return sys_result;
+}
+
+#define system system_with_shell
+
+#endif /* defined(__MINGW32__) */
+
+
 static void
 do_version (void)
 {