[RFA] Environment variables passed to inferior by MinGW build (PR 10989)

[RFA] Environment variables passed to inferior by MinGW build (PR 10989)

[Eli Zaretskii]

There's some history to this issue.  It was discussed at least twice
before:

   http://sourceware.org/ml/gdb-patches/2010-05/msg00317.html
   http://sourceware.org/ml/gdb-patches/2011-04/msg00347.html

The first discussion simply died because the OP never responded to
Joel's request to clean up the patch from weird characters.

As for the second discussion, upon re-reading it I can only assume
that there was a misunderstanding of some kind, perhaps Pierre was
talking about both the Cygwin and the native Windows builds, while
Chris and Corinna replied only about the latter.

In any case, the original problem is still there in the MinGW build of
GDB 7.3, and it just bit me hard enough to sit down and solve it.

The code in the patch below is simply taken from what was there before
it was deleted by Corinna back in 2006, after removing from it
everything that was Cygwin-specific.  So I don't even think I can take
credit for this code ;-)

With that patch, I can add environment variables with "set
environment", delete them with "unset environment", and the inferior
gets the environment I meant it to have.

OK to commit?

2011-09-11  Eli Zaretskii  <eliz@gnu.org>

	* windows-nat.c (env_sort) [!__CYGWIN__]: Function restored from
	before the change on 2006-12-09.
	(windows_create_inferior) [!__CYGWIN__]: Restore code that
	generates the environment block for CreateProcessA, modulo the
	Cygwin-specific parts that are not needed here.


=== modified file 'gdb/windows-nat.c'
--- gdb/windows-nat.c	2011-05-09 14:25:35 +0000
+++ gdb/windows-nat.c	2011-09-11 12:26:41 +0000
@@ -1951,6 +1951,17 @@
   *flags |= CREATE_NEW_CONSOLE;
 }
 
+#ifndef __CYGWIN__
+/* Function called by qsort to sort environment strings.  */
+static int
+env_sort (const void *a, const void *b)
+{
+  const char **p = (const char **) a;
+  const char **q = (const char **) b;
+  return strcasecmp (*p, *q);
+}
+#endif
+
 /* Start an inferior windows child process and sets inferior_ptid to its pid.
    EXEC_FILE is the file to run.
    ALLARGS is a string containing the arguments to the program.
@@ -1977,6 +1988,12 @@
   char *toexec;
   char *args;
   HANDLE tty;
+  char *w32env;
+  char *temp;
+  size_t envlen;
+  int i;
+  size_t envsize;
+  char **env;
 #endif
   PROCESS_INFORMATION pi;
   BOOL ret;
@@ -2124,6 +2141,31 @@
 	}
     }
 
+  /* CreateProcess takes the environment list as a null terminated set of
+     strings (i.e. two nulls terminate the list).  */
+
+  /* Get total size for env strings.  */
+  for (envlen = 0, i = 0; in_env[i] && *in_env[i]; i++)
+    envlen += strlen (in_env[i]) + 1;
+
+  envsize = sizeof (in_env[0]) * (i + 1);
+  env = (char **) alloca (envsize);
+  memcpy (env, in_env, envsize);
+  /* Windows programs expect the environment block to be sorted.  */
+  qsort (env, i, sizeof (char *), env_sort);
+
+  w32env = alloca (envlen + 1);
+
+  /* Copy env strings into new buffer.  */
+  for (temp = w32env, i = 0; env[i] && *env[i]; i++)
+    {
+      strcpy (temp, env[i]);
+      temp += strlen (temp) + 1;
+    }
+
+  /* Final nil string to terminate new env.  */
+  *temp = 0;
+
   windows_init_thread_list ();
   ret = CreateProcessA (0,
 			args,	/* command line */
@@ -2131,7 +2173,7 @@
 			NULL,	/* thread */
 			TRUE,	/* inherit handles */
 			flags,	/* start flags */
-			NULL,	/* environment */
+			w32env,	/* environment */
 			NULL,	/* current directory */
 			&si,
 			&pi);

Re: [RFA] Environment variables passed to inferior by MinGW build (PR 10989)

[asmwarrior]

On 2011-9-11 20:33, Eli Zaretskii wrote:
> There's some history to this issue.  It was discussed at least twice
> before:
>
>     http://sourceware.org/ml/gdb-patches/2010-05/msg00317.html
>     http://sourceware.org/ml/gdb-patches/2011-04/msg00347.html
>
> The first discussion simply died because the OP never responded to
> Joel's request to clean up the patch from weird characters.
>
> As for the second discussion, upon re-reading it I can only assume
> that there was a misunderstanding of some kind, perhaps Pierre was
> talking about both the Cygwin and the native Windows builds, while
> Chris and Corinna replied only about the latter.
>
> In any case, the original problem is still there in the MinGW build of
> GDB 7.3, and it just bit me hard enough to sit down and solve it.
>
> The code in the patch below is simply taken from what was there before
> it was deleted by Corinna back in 2006, after removing from it
> everything that was Cygwin-specific.  So I don't even think I can take
> credit for this code ;-)
>
> With that patch, I can add environment variables with "set
> environment", delete them with "unset environment", and the inferior
> gets the environment I meant it to have.
>
> OK to commit?
>
> 2011-09-11  Eli Zaretskii<eliz@gnu.org>
>
> 	* windows-nat.c (env_sort) [!__CYGWIN__]: Function restored from
> 	before the change on 2006-12-09.
> 	(windows_create_inferior) [!__CYGWIN__]: Restore code that
> 	generates the environment block for CreateProcessA, modulo the
> 	Cygwin-specific parts that are not needed here.
>
>
> === modified file 'gdb/windows-nat.c'
> --- gdb/windows-nat.c	2011-05-09 14:25:35 +0000
> +++ gdb/windows-nat.c	2011-09-11 12:26:41 +0000
> @@ -1951,6 +1951,17 @@
>     *flags |= CREATE_NEW_CONSOLE;
>   }
>
> +#ifndef __CYGWIN__
> +/* Function called by qsort to sort environment strings.  */
> +static int
> +env_sort (const void *a, const void *b)
> +{
> +  const char **p = (const char **) a;
> +  const char **q = (const char **) b;
> +  return strcasecmp (*p, *q);
> +}
> +#endif
> +
>   /* Start an inferior windows child process and sets inferior_ptid to its pid.
>      EXEC_FILE is the file to run.
>      ALLARGS is a string containing the arguments to the program.
> @@ -1977,6 +1988,12 @@
>     char *toexec;
>     char *args;
>     HANDLE tty;
> +  char *w32env;
> +  char *temp;
> +  size_t envlen;
> +  int i;
> +  size_t envsize;
> +  char **env;
>   #endif
>     PROCESS_INFORMATION pi;
>     BOOL ret;
> @@ -2124,6 +2141,31 @@
>   	}
>       }
>
> +  /* CreateProcess takes the environment list as a null terminated set of
> +     strings (i.e. two nulls terminate the list).  */
> +
> +  /* Get total size for env strings.  */
> +  for (envlen = 0, i = 0; in_env[i]&&  *in_env[i]; i++)
> +    envlen += strlen (in_env[i]) + 1;
> +
> +  envsize = sizeof (in_env[0]) * (i + 1);
> +  env = (char **) alloca (envsize);
> +  memcpy (env, in_env, envsize);
> +  /* Windows programs expect the environment block to be sorted.  */
> +  qsort (env, i, sizeof (char *), env_sort);
> +
> +  w32env = alloca (envlen + 1);
> +
> +  /* Copy env strings into new buffer.  */
> +  for (temp = w32env, i = 0; env[i]&&  *env[i]; i++)
> +    {
> +      strcpy (temp, env[i]);
> +      temp += strlen (temp) + 1;
> +    }
> +
> +  /* Final nil string to terminate new env.  */
> +  *temp = 0;
> +
>     windows_init_thread_list ();
>     ret = CreateProcessA (0,
>   			args,	/* command line */
> @@ -2131,7 +2173,7 @@
>   			NULL,	/* thread */
>   			TRUE,	/* inherit handles */
>   			flags,	/* start flags */
> -			NULL,	/* environment */
> +			w32env,	/* environment */
>   			NULL,	/* current directory */
>   			&si,
>   			&pi);
>
>

I just apply this patch, and build a native gdb under MinGW.
I use the test code in:

http://sourceware.org/ml/gdb-patches/2010-05/msg00317.html

----------------------------------------------------------
/* env-bug.c */

#include <stdio.h>
#include <stdlib.h>

int main(void)
{
char *env_name= "PATH";
char *env_value = getenv(env_name);

if(env_name)
printf("env-bug: %s=%s\n", env_name, env_value);
else
printf("env-bug: environment variable %s not found.\n", env_name);

env_name= "FOO";
env_value = getenv(env_name);

if(env_name)
printf("env-bug: %s=%s\n", env_name, env_value);
else
printf("env-bug: environment variable %s not found.\n", env_name);

return 0;
}
----------------------------------------------------------
(gdb) set environment FOO=bar

It works fine!

So, I think this patch should be put in trunk.

Thank you!

asmwarrior
ollydbg from codeblocks' forum

Re: [RFA] Environment variables passed to inferior by MinGW build (PR 10989)

[Eli Zaretskii]

Ping!

> Date: Sun, 11 Sep 2011 08:33:25 -0400
> From: Eli Zaretskii <eliz@gnu.org>
> CC: Corinna Vinschen <vinschen@redhat.com>
> 
> There's some history to this issue.  It was discussed at least twice
> before:
> 
>    http://sourceware.org/ml/gdb-patches/2010-05/msg00317.html
>    http://sourceware.org/ml/gdb-patches/2011-04/msg00347.html
> 
> The first discussion simply died because the OP never responded to
> Joel's request to clean up the patch from weird characters.
> 
> As for the second discussion, upon re-reading it I can only assume
> that there was a misunderstanding of some kind, perhaps Pierre was
> talking about both the Cygwin and the native Windows builds, while
> Chris and Corinna replied only about the latter.
> 
> In any case, the original problem is still there in the MinGW build of
> GDB 7.3, and it just bit me hard enough to sit down and solve it.
> 
> The code in the patch below is simply taken from what was there before
> it was deleted by Corinna back in 2006, after removing from it
> everything that was Cygwin-specific.  So I don't even think I can take
> credit for this code ;-)
> 
> With that patch, I can add environment variables with "set
> environment", delete them with "unset environment", and the inferior
> gets the environment I meant it to have.
> 
> OK to commit?
> 
> 2011-09-11  Eli Zaretskii  <eliz@gnu.org>
> 
> 	* windows-nat.c (env_sort) [!__CYGWIN__]: Function restored from
> 	before the change on 2006-12-09.
> 	(windows_create_inferior) [!__CYGWIN__]: Restore code that
> 	generates the environment block for CreateProcessA, modulo the
> 	Cygwin-specific parts that are not needed here.
> 
> 
> === modified file 'gdb/windows-nat.c'
> --- gdb/windows-nat.c	2011-05-09 14:25:35 +0000
> +++ gdb/windows-nat.c	2011-09-11 12:26:41 +0000
> @@ -1951,6 +1951,17 @@
>    *flags |= CREATE_NEW_CONSOLE;
>  }
>  
> +#ifndef __CYGWIN__
> +/* Function called by qsort to sort environment strings.  */
> +static int
> +env_sort (const void *a, const void *b)
> +{
> +  const char **p = (const char **) a;
> +  const char **q = (const char **) b;
> +  return strcasecmp (*p, *q);
> +}
> +#endif
> +
>  /* Start an inferior windows child process and sets inferior_ptid to its pid.
>     EXEC_FILE is the file to run.
>     ALLARGS is a string containing the arguments to the program.
> @@ -1977,6 +1988,12 @@
>    char *toexec;
>    char *args;
>    HANDLE tty;
> +  char *w32env;
> +  char *temp;
> +  size_t envlen;
> +  int i;
> +  size_t envsize;
> +  char **env;
>  #endif
>    PROCESS_INFORMATION pi;
>    BOOL ret;
> @@ -2124,6 +2141,31 @@
>  	}
>      }
>  
> +  /* CreateProcess takes the environment list as a null terminated set of
> +     strings (i.e. two nulls terminate the list).  */
> +
> +  /* Get total size for env strings.  */
> +  for (envlen = 0, i = 0; in_env[i] && *in_env[i]; i++)
> +    envlen += strlen (in_env[i]) + 1;
> +
> +  envsize = sizeof (in_env[0]) * (i + 1);
> +  env = (char **) alloca (envsize);
> +  memcpy (env, in_env, envsize);
> +  /* Windows programs expect the environment block to be sorted.  */
> +  qsort (env, i, sizeof (char *), env_sort);
> +
> +  w32env = alloca (envlen + 1);
> +
> +  /* Copy env strings into new buffer.  */
> +  for (temp = w32env, i = 0; env[i] && *env[i]; i++)
> +    {
> +      strcpy (temp, env[i]);
> +      temp += strlen (temp) + 1;
> +    }
> +
> +  /* Final nil string to terminate new env.  */
> +  *temp = 0;
> +
>    windows_init_thread_list ();
>    ret = CreateProcessA (0,
>  			args,	/* command line */
> @@ -2131,7 +2173,7 @@
>  			NULL,	/* thread */
>  			TRUE,	/* inherit handles */
>  			flags,	/* start flags */
> -			NULL,	/* environment */
> +			w32env,	/* environment */
>  			NULL,	/* current directory */
>  			&si,
>  			&pi);
> 

Re: [RFA] Environment variables passed to inferior by MinGW build (PR 10989)

[Eli Zaretskii]

Ping!  Ping!  (2 weeks)

> Date: Mon, 19 Sep 2011 03:37:06 -0400
> From: Eli Zaretskii <eliz@gnu.org>
> Reply-to: Eli Zaretskii <eliz@gnu.org>
> 
> Ping!
> 
> > Date: Sun, 11 Sep 2011 08:33:25 -0400
> > From: Eli Zaretskii <eliz@gnu.org>
> > CC: Corinna Vinschen <vinschen@redhat.com>
> > 
> > There's some history to this issue.  It was discussed at least twice
> > before:
> > 
> >    http://sourceware.org/ml/gdb-patches/2010-05/msg00317.html
> >    http://sourceware.org/ml/gdb-patches/2011-04/msg00347.html
> > 
> > The first discussion simply died because the OP never responded to
> > Joel's request to clean up the patch from weird characters.
> > 
> > As for the second discussion, upon re-reading it I can only assume
> > that there was a misunderstanding of some kind, perhaps Pierre was
> > talking about both the Cygwin and the native Windows builds, while
> > Chris and Corinna replied only about the latter.
> > 
> > In any case, the original problem is still there in the MinGW build of
> > GDB 7.3, and it just bit me hard enough to sit down and solve it.
> > 
> > The code in the patch below is simply taken from what was there before
> > it was deleted by Corinna back in 2006, after removing from it
> > everything that was Cygwin-specific.  So I don't even think I can take
> > credit for this code ;-)
> > 
> > With that patch, I can add environment variables with "set
> > environment", delete them with "unset environment", and the inferior
> > gets the environment I meant it to have.
> > 
> > OK to commit?
> > 
> > 2011-09-11  Eli Zaretskii  <eliz@gnu.org>
> > 
> > 	* windows-nat.c (env_sort) [!__CYGWIN__]: Function restored from
> > 	before the change on 2006-12-09.
> > 	(windows_create_inferior) [!__CYGWIN__]: Restore code that
> > 	generates the environment block for CreateProcessA, modulo the
> > 	Cygwin-specific parts that are not needed here.
> > 
> > 
> > === modified file 'gdb/windows-nat.c'
> > --- gdb/windows-nat.c	2011-05-09 14:25:35 +0000
> > +++ gdb/windows-nat.c	2011-09-11 12:26:41 +0000
> > @@ -1951,6 +1951,17 @@
> >    *flags |= CREATE_NEW_CONSOLE;
> >  }
> >  
> > +#ifndef __CYGWIN__
> > +/* Function called by qsort to sort environment strings.  */
> > +static int
> > +env_sort (const void *a, const void *b)
> > +{
> > +  const char **p = (const char **) a;
> > +  const char **q = (const char **) b;
> > +  return strcasecmp (*p, *q);
> > +}
> > +#endif
> > +
> >  /* Start an inferior windows child process and sets inferior_ptid to its pid.
> >     EXEC_FILE is the file to run.
> >     ALLARGS is a string containing the arguments to the program.
> > @@ -1977,6 +1988,12 @@
> >    char *toexec;
> >    char *args;
> >    HANDLE tty;
> > +  char *w32env;
> > +  char *temp;
> > +  size_t envlen;
> > +  int i;
> > +  size_t envsize;
> > +  char **env;
> >  #endif
> >    PROCESS_INFORMATION pi;
> >    BOOL ret;
> > @@ -2124,6 +2141,31 @@
> >  	}
> >      }
> >  
> > +  /* CreateProcess takes the environment list as a null terminated set of
> > +     strings (i.e. two nulls terminate the list).  */
> > +
> > +  /* Get total size for env strings.  */
> > +  for (envlen = 0, i = 0; in_env[i] && *in_env[i]; i++)
> > +    envlen += strlen (in_env[i]) + 1;
> > +
> > +  envsize = sizeof (in_env[0]) * (i + 1);
> > +  env = (char **) alloca (envsize);
> > +  memcpy (env, in_env, envsize);
> > +  /* Windows programs expect the environment block to be sorted.  */
> > +  qsort (env, i, sizeof (char *), env_sort);
> > +
> > +  w32env = alloca (envlen + 1);
> > +
> > +  /* Copy env strings into new buffer.  */
> > +  for (temp = w32env, i = 0; env[i] && *env[i]; i++)
> > +    {
> > +      strcpy (temp, env[i]);
> > +      temp += strlen (temp) + 1;
> > +    }
> > +
> > +  /* Final nil string to terminate new env.  */
> > +  *temp = 0;
> > +
> >    windows_init_thread_list ();
> >    ret = CreateProcessA (0,
> >  			args,	/* command line */
> > @@ -2131,7 +2173,7 @@
> >  			NULL,	/* thread */
> >  			TRUE,	/* inherit handles */
> >  			flags,	/* start flags */
> > -			NULL,	/* environment */
> > +			w32env,	/* environment */
> >  			NULL,	/* current directory */
> >  			&si,
> >  			&pi);
> > 
> 

Re: [RFA] Environment variables passed to inferior by MinGW build (PR 10989)

[Stan Shebs]

On 9/25/11 3:47 AM, Eli Zaretskii wrote:
> Ping!  Ping!  (2 weeks)

Seems uncontroversial to me (the previous discussion being about 
entirely different issues).  Let's put it in!

Stan
stan@codesourcery.com


>
>> Date: Mon, 19 Sep 2011 03:37:06 -0400
>> From: Eli Zaretskii<eliz@gnu.org>
>> Reply-to: Eli Zaretskii<eliz@gnu.org>
>>
>> Ping!
>>
>>> Date: Sun, 11 Sep 2011 08:33:25 -0400
>>> From: Eli Zaretskii<eliz@gnu.org>
>>> CC: Corinna Vinschen<vinschen@redhat.com>
>>>
>>> There's some history to this issue.  It was discussed at least twice
>>> before:
>>>
>>>     http://sourceware.org/ml/gdb-patches/2010-05/msg00317.html
>>>     http://sourceware.org/ml/gdb-patches/2011-04/msg00347.html
>>>
>>> The first discussion simply died because the OP never responded to
>>> Joel's request to clean up the patch from weird characters.
>>>
>>> As for the second discussion, upon re-reading it I can only assume
>>> that there was a misunderstanding of some kind, perhaps Pierre was
>>> talking about both the Cygwin and the native Windows builds, while
>>> Chris and Corinna replied only about the latter.
>>>
>>> In any case, the original problem is still there in the MinGW build of
>>> GDB 7.3, and it just bit me hard enough to sit down and solve it.
>>>
>>> The code in the patch below is simply taken from what was there before
>>> it was deleted by Corinna back in 2006, after removing from it
>>> everything that was Cygwin-specific.  So I don't even think I can take
>>> credit for this code ;-)
>>>
>>> With that patch, I can add environment variables with "set
>>> environment", delete them with "unset environment", and the inferior
>>> gets the environment I meant it to have.
>>>
>>> OK to commit?
>>>
>>> 2011-09-11  Eli Zaretskii<eliz@gnu.org>
>>>
>>> 	* windows-nat.c (env_sort) [!__CYGWIN__]: Function restored from
>>> 	before the change on 2006-12-09.
>>> 	(windows_create_inferior) [!__CYGWIN__]: Restore code that
>>> 	generates the environment block for CreateProcessA, modulo the
>>> 	Cygwin-specific parts that are not needed here.
>>>
>>>
>>> === modified file 'gdb/windows-nat.c'
>>> --- gdb/windows-nat.c	2011-05-09 14:25:35 +0000
>>> +++ gdb/windows-nat.c	2011-09-11 12:26:41 +0000
>>> @@ -1951,6 +1951,17 @@
>>>     *flags |= CREATE_NEW_CONSOLE;
>>>   }
>>>
>>> +#ifndef __CYGWIN__
>>> +/* Function called by qsort to sort environment strings.  */
>>> +static int
>>> +env_sort (const void *a, const void *b)
>>> +{
>>> +  const char **p = (const char **) a;
>>> +  const char **q = (const char **) b;
>>> +  return strcasecmp (*p, *q);
>>> +}
>>> +#endif
>>> +
>>>   /* Start an inferior windows child process and sets inferior_ptid to its pid.
>>>      EXEC_FILE is the file to run.
>>>      ALLARGS is a string containing the arguments to the program.
>>> @@ -1977,6 +1988,12 @@
>>>     char *toexec;
>>>     char *args;
>>>     HANDLE tty;
>>> +  char *w32env;
>>> +  char *temp;
>>> +  size_t envlen;
>>> +  int i;
>>> +  size_t envsize;
>>> +  char **env;
>>>   #endif
>>>     PROCESS_INFORMATION pi;
>>>     BOOL ret;
>>> @@ -2124,6 +2141,31 @@
>>>   	}
>>>       }
>>>
>>> +  /* CreateProcess takes the environment list as a null terminated set of
>>> +     strings (i.e. two nulls terminate the list).  */
>>> +
>>> +  /* Get total size for env strings.  */
>>> +  for (envlen = 0, i = 0; in_env[i]&&  *in_env[i]; i++)
>>> +    envlen += strlen (in_env[i]) + 1;
>>> +
>>> +  envsize = sizeof (in_env[0]) * (i + 1);
>>> +  env = (char **) alloca (envsize);
>>> +  memcpy (env, in_env, envsize);
>>> +  /* Windows programs expect the environment block to be sorted.  */
>>> +  qsort (env, i, sizeof (char *), env_sort);
>>> +
>>> +  w32env = alloca (envlen + 1);
>>> +
>>> +  /* Copy env strings into new buffer.  */
>>> +  for (temp = w32env, i = 0; env[i]&&  *env[i]; i++)
>>> +    {
>>> +      strcpy (temp, env[i]);
>>> +      temp += strlen (temp) + 1;
>>> +    }
>>> +
>>> +  /* Final nil string to terminate new env.  */
>>> +  *temp = 0;
>>> +
>>>     windows_init_thread_list ();
>>>     ret = CreateProcessA (0,
>>>   			args,	/* command line */
>>> @@ -2131,7 +2173,7 @@
>>>   			NULL,	/* thread */
>>>   			TRUE,	/* inherit handles */
>>>   			flags,	/* start flags */
>>> -			NULL,	/* environment */
>>> +			w32env,	/* environment */
>>>   			NULL,	/* current directory */
>>>   			&si,
>>>   			&pi);
>>>

Re: [RFA] Environment variables passed to inferior by MinGW build (PR 10989)

[Joel Brobecker]

> >Ping!  Ping!  (2 weeks)
> 
> Seems uncontroversial to me (the previous discussion being about
> entirely different issues).  Let's put it in!

And I think that 2 weeks without comment from the area maintainer
is reasonable delay for a Global Maintainer to approve or
self-approve... If it turns out the area maintainer was away
during that time, and finds some problems, we can always fix or
revert.

> >>>2011-09-11  Eli Zaretskii<eliz@gnu.org>
> >>>
> >>>	* windows-nat.c (env_sort) [!__CYGWIN__]: Function restored from
> >>>	before the change on 2006-12-09.
> >>>	(windows_create_inferior) [!__CYGWIN__]: Restore code that
> >>>	generates the environment block for CreateProcessA, modulo the
> >>>	Cygwin-specific parts that are not needed here.

Just a few comments:

> >>>+#ifndef __CYGWIN__
> >>>+/* Function called by qsort to sort environment strings.  */
> >>>+static int

Empty line after the function description (and before the declaration).

> >>>+env_sort (const void *a, const void *b)

Can we rename it to something like `env_cmp'. The use of `sort' makes
me think that the contents of `a' and `b' are going to be swapped if
b < a...

(this is a bit of a nitpick, so feel free to ignore)

> >>>+  for (envlen = 0, i = 0; in_env[i]&&  *in_env[i]; i++)
                                        ^^^ formatting of the `&&' ?

-- 
Joel

Re: [RFA] Environment variables passed to inferior by MinGW build (PR 10989)

[Eli Zaretskii]

> Date: Mon, 26 Sep 2011 10:15:11 -0700
> From: Joel Brobecker <brobecker@adacore.com>
> Cc: gdb-patches@sourceware.org
> 
> Just a few comments:

Thanks, I will fix all of these.  (They came from the original code.)

Re: [RFA] Environment variables passed to inferior by MinGW build (PR 10989)

[Eli Zaretskii]

> Date: Mon, 26 Sep 2011 20:21:24 +0300
> From: Eli Zaretskii <eliz@gnu.org>
> Cc: gdb-patches@sourceware.org
> 
> > Date: Mon, 26 Sep 2011 10:15:11 -0700
> > From: Joel Brobecker <brobecker@adacore.com>
> > Cc: gdb-patches@sourceware.org
> > 
> > Just a few comments:
> 
> Thanks, I will fix all of these.  (They came from the original code.)

Done.  The actual patch I committed is below.

I replaced env_sort with envvar_cmp.

The last comment, viz.:

> > >>>+  for (envlen = 0, i = 0; in_env[i]&&  *in_env[i]; i++)
>                                         ^^^ formatting of the `&&' ?
> 

didn't need any change, because there's no such problem in the patch I
sent.  Joel, you were replying to Stan's mail, so I'm guessing that
Stan's mail software tried to be overly smart about the formatting of
the patch.

Thanks for the review and the approval.

Could someone with the appropriate authority please close bug #10989,
or instruct me how to do that?

Index: gdb/ChangeLog
===================================================================
RCS file: /cvs/src/src/gdb/ChangeLog,v
retrieving revision 1.13362
retrieving revision 1.13363
diff -u -r1.13362 -r1.13363
--- gdb/ChangeLog	27 Sep 2011 15:30:12 -0000	1.13362
+++ gdb/ChangeLog	28 Sep 2011 09:07:54 -0000	1.13363
@@ -1,3 +1,11 @@
+2011-09-28  Eli Zaretskii  <eliz@gnu.org>
+
+	* windows-nat.c (env_sort) [!__CYGWIN__]: Function restored from
+	before the change on 2006-12-09.
+	(windows_create_inferior) [!__CYGWIN__]: Restore code that
+	generates the environment block for CreateProcessA, modulo the
+	Cygwin-specific parts that are not needed here.
+
 2011-09-27  Tristan Gingold  <gingold@adacore.com>
 
 	* target.h (enum target_object): Add TARGET_OBJECT_DARWIN_DYLD_INFO.


Index: gdb/windows-nat.c
===================================================================
RCS file: /cvs/src/src/gdb/windows-nat.c,v
retrieving revision 1.218
retrieving revision 1.219
diff -u -p -r1.218 -r1.219
--- gdb/windows-nat.c	9 May 2011 14:25:37 -0000	1.218
+++ gdb/windows-nat.c	28 Sep 2011 09:07:54 -0000	1.219
@@ -1951,6 +1951,18 @@ windows_set_console_info (STARTUPINFO *s
   *flags |= CREATE_NEW_CONSOLE;
 }
 
+#ifndef __CYGWIN__
+/* Function called by qsort to sort environment strings.  */
+
+static int
+envvar_cmp (const void *a, const void *b)
+{
+  const char **p = (const char **) a;
+  const char **q = (const char **) b;
+  return strcasecmp (*p, *q);
+}
+#endif
+
 /* Start an inferior windows child process and sets inferior_ptid to its pid.
    EXEC_FILE is the file to run.
    ALLARGS is a string containing the arguments to the program.
@@ -1977,6 +1989,12 @@ windows_create_inferior (struct target_o
   char *toexec;
   char *args;
   HANDLE tty;
+  char *w32env;
+  char *temp;
+  size_t envlen;
+  int i;
+  size_t envsize;
+  char **env;
 #endif
   PROCESS_INFORMATION pi;
   BOOL ret;
@@ -2124,6 +2142,31 @@ windows_create_inferior (struct target_o
 	}
     }
 
+  /* CreateProcess takes the environment list as a null terminated set of
+     strings (i.e. two nulls terminate the list).  */
+
+  /* Get total size for env strings.  */
+  for (envlen = 0, i = 0; in_env[i] && *in_env[i]; i++)
+    envlen += strlen (in_env[i]) + 1;
+
+  envsize = sizeof (in_env[0]) * (i + 1);
+  env = (char **) alloca (envsize);
+  memcpy (env, in_env, envsize);
+  /* Windows programs expect the environment block to be sorted.  */
+  qsort (env, i, sizeof (char *), envvar_cmp);
+
+  w32env = alloca (envlen + 1);
+
+  /* Copy env strings into new buffer.  */
+  for (temp = w32env, i = 0; env[i] && *env[i]; i++)
+    {
+      strcpy (temp, env[i]);
+      temp += strlen (temp) + 1;
+    }
+
+  /* Final nil string to terminate new env.  */
+  *temp = 0;
+
   windows_init_thread_list ();
   ret = CreateProcessA (0,
 			args,	/* command line */
@@ -2131,7 +2174,7 @@ windows_create_inferior (struct target_o
 			NULL,	/* thread */
 			TRUE,	/* inherit handles */
 			flags,	/* start flags */
-			NULL,	/* environment */
+			w32env,	/* environment */
 			NULL,	/* current directory */
 			&si,
 			&pi);

Re: [RFA] Environment variables passed to inferior by MinGW build (PR 10989)

[Tom Tromey]

>>>>> "Eli" == Eli Zaretskii <eliz@gnu.org> writes:

Eli> Could someone with the appropriate authority please close bug #10989,
Eli> or instruct me how to do that?

For future reference, if a patch fixes a PR, you can mention the PR in
the ChangeLog and the commit message; this causes the commit message to
be appended to the PR, which is handy for future archaeology.

I closed the PR.

I think you can close them by logging in to bugzilla using your
sourceware.org address.  Those addresses, IIUC, are automatically given
write permissions.

Tom

Re: [RFA] Environment variables passed to inferior by MinGW build (PR 10989)

[Eli Zaretskii]

> From: Tom Tromey <tromey@redhat.com>
> Cc: brobecker@adacore.com, gdb-patches@sourceware.org
> Date: Mon, 03 Oct 2011 12:17:25 -0600
> 
> For future reference, if a patch fixes a PR, you can mention the PR in
> the ChangeLog and the commit message; this causes the commit message to
> be appended to the PR, which is handy for future archaeology.

Will do.  (I actually remembered about this, but it was too late...)(

> I think you can close them by logging in to bugzilla using your
> sourceware.org address.  Those addresses, IIUC, are automatically given
> write permissions.

Thanks, will try that.

Re: [RFA] Environment variables passed to inferior by MinGW build (PR 10989)

[Corinna Vinschen]

On Sep 26 10:15, Joel Brobecker wrote:
> > >Ping!  Ping!  (2 weeks)
> > 
> > Seems uncontroversial to me (the previous discussion being about
> > entirely different issues).  Let's put it in!
> 
> And I think that 2 weeks without comment from the area maintainer
> is reasonable delay for a Global Maintainer to approve or
> self-approve... If it turns out the area maintainer was away
> during that time, and finds some problems, we can always fix or
> revert.

I was on vacation and I'm also not the area maintainer for windows-nat.c.
I commented to http://sourceware.org/ml/gdb-patches/2011-04/msg00347.html
because what Pierre was doing looked wrong to me.

Eli's patch is ok, IMHO, but it's a pity that Pierre never followed up
to http://sourceware.org/ml/gdb-patches/2011-04/msg00351.html since that
means that now setting environment variables works in Mingw, but not in
Cygwin.  So, as far as I can see, PR #10989 should not have been closed
yet.


Corinna

-- 
Corinna Vinschen
Cygwin Project Co-Leader
Red Hat

Re: [RFA] Environment variables passed to inferior by MinGW build (PR 10989)

[Eli Zaretskii]

> Date: Tue, 4 Oct 2011 09:53:26 +0200
> From: Corinna Vinschen <vinschen@redhat.com>
> Reply-To: gdb-patches@sourceware.org
> 
> I was on vacation and I'm also not the area maintainer for windows-nat.c.

No worries, I hope you had a good time ;-)
I CC'ed you because you made the change that I partially undid, and
also participated in some of the discussions I cited.

> Eli's patch is ok, IMHO, but it's a pity that Pierre never followed up
> to http://sourceware.org/ml/gdb-patches/2011-04/msg00351.html since that
> means that now setting environment variables works in Mingw, but not in
> Cygwin.

Could you elaborate why the behavior on Cygwin is incorrect?  AFAICT,
Pierre's changes didn't get into GDB, so GDB still works as it did
before, and your changes that introduced the call to cygwin_internal
were supposed to fix the issue of "set environment" and "unset
environment", right?  Not that I understand exactly what
cygwin_internal does in that case and how it works.

> So, as far as I can see, PR #10989 should not have been closed
> yet.

But that bug report is for the MinGW host and target only, so if
Cygwin has a similar problem, it's a different issue and probably also
a different solution.

OTOH, if the same code I re-introduced will work for Cygwin, it should
be a simple matter to remove the #if conditionals.  Am I missing
something?

Re: [RFA] Environment variables passed to inferior by MinGW build (PR 10989)

[Corinna Vinschen]

On Oct  4 05:29, Eli Zaretskii wrote:
> > Date: Tue, 4 Oct 2011 09:53:26 +0200
> > From: Corinna Vinschen <vinschen@redhat.com>
> > Reply-To: gdb-patches@sourceware.org
> > 
> > I was on vacation and I'm also not the area maintainer for windows-nat.c.
> 
> No worries, I hope you had a good time ;-)
> I CC'ed you because you made the change that I partially undid, and
> also participated in some of the discussions I cited.
> 
> > Eli's patch is ok, IMHO, but it's a pity that Pierre never followed up
> > to http://sourceware.org/ml/gdb-patches/2011-04/msg00351.html since that
> > means that now setting environment variables works in Mingw, but not in
> > Cygwin.
> 
> Could you elaborate why the behavior on Cygwin is incorrect?  AFAICT,
> Pierre's changes didn't get into GDB, so GDB still works as it did
> before, and your changes that introduced the call to cygwin_internal
> were supposed to fix the issue of "set environment" and "unset
> environment", right?

No, my changes from way back didn't handle `set env' and `unset env',
they actually introduced the problem.  The  cygwin_internal call was
necessary to sync Windows and Cygwin environment when calling
CreateProcess.  At the time I neglected the internally set environment
vars and nobody noticed the mistake, apparently.

So, your patch is fine, I was just commenting on the fact that it would
have been nice if the patch cared for all code paths.

> > So, as far as I can see, PR #10989 should not have been closed
> > yet.
> 
> But that bug report is for the MinGW host and target only, so if
> Cygwin has a similar problem, it's a different issue and probably also
> a different solution.
> 
> OTOH, if the same code I re-introduced will work for Cygwin, it should
> be a simple matter to remove the #if conditionals.  Am I missing
> something?

I'm quite busy catching up after vacaction right now.  Off the top of
my head it might be the right thing to do, with a single exception:
It would be most helpful if the Windows environment is created as
UNICODE environment and CreateProcess uses the CREATE_UNICODE_ENVIRONMENT
flag.  This should work pretty much identically for both code paths.
Using the Unicode environment has some advantages:

- The ANSI environment block is restricted to 64K in size, the UNICODE
  environment block is not restricted at all.

- Using UNICODE makes the environment codepage agnostic.

- A single static function could be used to allocate and create the
  environment for both code paths.


Corinna

-- 
Corinna Vinschen
Cygwin Project Co-Leader
Red Hat

[RFC] Environment variables passed to inferior for Cygwin build ( follow up on mingw fix for PR 10989)

[Pierre Muller]

> -----Message d'origine-----
> De : gdb-patches-owner@sourceware.org [mailto:gdb-patches-
> owner@sourceware.org] De la part de Corinna Vinschen
> Envoyé : mardi 4 octobre 2011 09:53
> À : gdb-patches@sourceware.org
> Objet : Re: [RFA] Environment variables passed to inferior by MinGW build
> (PR 10989)
> 
> On Sep 26 10:15, Joel Brobecker wrote:
> > > >Ping!  Ping!  (2 weeks)
> > >
> > > Seems uncontroversial to me (the previous discussion being about
> > > entirely different issues).  Let's put it in!
> >
> > And I think that 2 weeks without comment from the area maintainer
> > is reasonable delay for a Global Maintainer to approve or
> > self-approve... If it turns out the area maintainer was away
> > during that time, and finds some problems, we can always fix or
> > revert.
> 
> I was on vacation and I'm also not the area maintainer for windows-nat.c.
> I commented to http://sourceware.org/ml/gdb-patches/2011-04/msg00347.html
> because what Pierre was doing looked wrong to me.

  
 
> Eli's patch is ok, IMHO, but it's a pity that Pierre never followed up
> to http://sourceware.org/ml/gdb-patches/2011-04/msg00351.html since that
> means that now setting environment variables works in Mingw, but not in
> Cygwin.  So, as far as I can see, PR #10989 should not have been closed
> yet.

  If you want, I can resubmit a patch for that specific
part, as mingw part is now handled by Eli's patch anyway.
  But, after looking into it, I suspect that your proposal
was about setting only environment variables that have
been explicitly modified within GDB... but the environment list is a 
simple array of 'char *'.
  Furthermore, there is no recording inside GDB code of
environment variables that were explicitly modified.
  in_env contains all environment variables coming from GDB startup.

  Thus, if I use your suggestion,
call   cygwin_internal (CW_SYNC_WINENV); first
and set environment variables after,
PATH variable stays in cygwin form, which leads to a failure within
CreateProcess call because some DLLs are not found.

  Using   cygwin_internal (CW_SYNC_WINENV);
after settings individual environment variables
using SetEnvironmentVariable call
work, but I am not sure this is what you wanted.

  I am still sending here the patch, so you can test it, if you want.
In fact, I suppose that the out_env part is not really needed:
if no environment pointer is given, CreateProcess takes the environment of
the parent process (which is GDB!), but we just modified it
so that probably, leaving that part out would give the same result.
  I also added a testsuite check for unsetting in gdb.base/setshow.exp
(there is a test called environ.exp, but it is limited to hpux 
for a reason that isn't clear to me...)


Pierre

ChangeLog entry:
2011-10-04  Pierre Muller  <muller@ics.u-strasbg.fr>

	* windows-nat.c (windows_create_inferior): Handle in_env
	array for Cygwin GDB.


testsuite/ChangeLog entry:
2011-10-04  Pierre Muller  <muller@ics.u-strasbg.fr>

	* gdb.base/setshow.exp (gdb_test): Add test to check
	that 'unset environment' works.

Index: windows-nat.c
===================================================================
RCS file: /cvs/src/src/gdb/windows-nat.c,v
retrieving revision 1.219
diff -u -p -r1.219 windows-nat.c
--- windows-nat.c	28 Sep 2011 09:07:54 -0000	1.219
+++ windows-nat.c	4 Oct 2011 09:49:26 -0000
@@ -1980,8 +1980,9 @@ windows_create_inferior (struct target_o
   cygwin_buf_t *toexec;
   cygwin_buf_t *cygallargs;
   cygwin_buf_t *args;
+  LPWCH out_env;
   size_t len;
-  int tty;
+  int tty, i;
   int ostdin, ostdout, ostderr;
 #else
   char real_path[__PMAX];
@@ -2066,9 +2067,36 @@ windows_create_inferior (struct target_o
   strcat (args, cygallargs);
 #endif
 
+  /* Set environment strings.  */
+  for (i = 0; in_env[i] && *in_env[i]; i++)
+    {
+      LPWCH wenv;
+      len = mbstowcs (NULL, in_env[i], 0) + 1;
+      if (len == (size_t) -1)
+	error (_("Error converting %s to UniCode %d"), in_env[i], errno);
+      else
+	{
+	  LPWCH envval;
+	  wenv = (LPWCH) alloca (len * sizeof (wchar_t));
+          mbstowcs (wenv, in_env[i], len);
+	  envval = (LPWCH) wcschr (wenv, L'=');
+	  /* If not found, it would mean unset
+	     which is done by setting second parameter of
+	     SetEnvironmentVaraibleW to NULL. */
+	  if (envval)
+	    {
+	      *envval = L'\0';
+	      envval++;
+	      if (*envval == '\0')
+		envval = NULL;
+	    }
+	  SetEnvironmentVariableW (wenv, envval);
+	}
+    }
   /* Prepare the environment vars for CreateProcess.  */
   cygwin_internal (CW_SYNC_WINENV);
-
+  out_env = GetEnvironmentStringsW ();
+  flags |= CREATE_UNICODE_ENVIRONMENT;
   if (!inferior_io_terminal)
     tty = ostdin = ostdout = ostderr = -1;
   else
@@ -2097,10 +2125,11 @@ windows_create_inferior (struct target_o
 		       NULL,	/* thread */
 		       TRUE,	/* inherit handles */
 		       flags,	/* start flags */
-		       NULL,	/* environment */
+		       out_env,	/* environment */
 		       NULL,	/* current directory */
 		       &si,
 		       &pi);
+  FreeEnvironmentStringsW (out_env);
   if (tty >= 0)
     {
       close (tty);
Index: testsuite/gdb.base/setshow.exp
===================================================================
RCS file: /cvs/src/src/gdb/testsuite/gdb.base/setshow.exp,v
retrieving revision 1.21
diff -u -p -r1.21 setshow.exp
--- testsuite/gdb.base/setshow.exp	20 Apr 2011 14:56:49 -0000	1.21
+++ testsuite/gdb.base/setshow.exp	4 Oct 2011 09:49:26 -0000
@@ -155,8 +155,13 @@ gdb_test "show editing" "Editing of comm
 gdb_test_no_output "set environment FOOBARBAZ = grbxgrbxgrbx" \
     "set environment FOOBARBAZ" 
 #test show environment FOOBARBAZ
-gdb_test "show environment FOOBARBAZ" "FOOBARBAZ = grbxgrbxgrbx.*"  "show environment FOOBARBAZ" 
-#test set height 100
+gdb_test "show environment FOOBARBAZ" "FOOBARBAZ = grbxgrbxgrbx.*"  "show environment FOOBARBAZ"
+#test unset environment FOOBARBAZ
+gdb_test_no_output "unset environment FOOBARBAZ" \
+    "unset environment FOOBARBAZ"
+#test show environment FOOBARBAZ
+gdb_test "show environment FOOBARBAZ" "Environment variable \"FOOBARBAZ\" not defined.*"  "show unset environment FOOBARBAZ"
+##test set height 100
 gdb_test_no_output "set height 100" "set height 100" 
 #test show height 100
 gdb_test "show height" "Number of lines gdb thinks are in a page is 100..*" "show height"

Re: [RFC] Environment variables passed to inferior for Cygwin build ( follow up on mingw fix for PR 10989)

[Eli Zaretskii]

> From: "Pierre Muller" <pierre.muller@ics-cnrs.unistra.fr>
> Date: Tue, 4 Oct 2011 11:56:35 +0200
> 
>   But, after looking into it, I suspect that your proposal
> was about setting only environment variables that have
> been explicitly modified within GDB... but the environment list is a 
> simple array of 'char *'.
>   Furthermore, there is no recording inside GDB code of
> environment variables that were explicitly modified.
>   in_env contains all environment variables coming from GDB startup.
> 
>   Thus, if I use your suggestion,
> call   cygwin_internal (CW_SYNC_WINENV); first
> and set environment variables after,
> PATH variable stays in cygwin form, which leads to a failure within
> CreateProcess call because some DLLs are not found.
> 
>   Using   cygwin_internal (CW_SYNC_WINENV);
> after settings individual environment variables
> using SetEnvironmentVariable call
> work, but I am not sure this is what you wanted.
> 
>   I am still sending here the patch, so you can test it, if you want.
> In fact, I suppose that the out_env part is not really needed:
> if no environment pointer is given, CreateProcess takes the environment of
> the parent process (which is GDB!), but we just modified it
> so that probably, leaving that part out would give the same result.

I'm confused: what problem(s) are you trying to fix in your patch?
Can you please enumerate the problems, and why do you think they
exist?  I think there's a misunderstanding here (I thought that about
the previous discussion in April, and I fear we will just repeat the
same misunderstanding now).

To comment on the part of what you say that I do understand: using
SetEnvironmentVariable in this case is not TRT, because that changes
the environment of GDB itself.  By contrast, "set environment" and
"unset environment" commands are supposed to affect only the
environment of the inferior, without any effect on GDB itself.  That
is why I didn't use SetEnvironmentVariable in my patch.

Re: [RFC] Environment variables passed to inferior for Cygwin build ( follow up on mingw fix for PR 10989)

[Corinna Vinschen]

On Oct  4 06:40, Eli Zaretskii wrote:
> To comment on the part of what you say that I do understand: using
> SetEnvironmentVariable in this case is not TRT, because that changes
> the environment of GDB itself.  By contrast, "set environment" and
> "unset environment" commands are supposed to affect only the
> environment of the inferior, without any effect on GDB itself.  That
> is why I didn't use SetEnvironmentVariable in my patch.

Duh, you're right.  Maybe my proposal to create the UNICODE environment
in a static function and use that in both code paths as in
http://sourceware.org/ml/gdb-patches/2011-10/msg00071.html
is acceptable?


Corinna

-- 
Corinna Vinschen
Cygwin Project Co-Leader
Red Hat