[RFC] Add support for locally modified environment variables for windows-nat.c

[RFC] Add support for locally modified environment variables for windows-nat.c

[Pierre Muller]

A while ago
Sebastian Puebla submitted a patch for 
support of locally modified environment variables.

http://sourceware.org/ml/gdb-patches/2010-05/msg00317.html
http://sourceware.org/ml/gdb-patches/2010-08/msg00026.html
http://sourceware.org/ml/gdb-patches/2010-11/msg00128.html

Nevertheless, I suspect that his patch would not have worked for Cygwin,
because Cygwin converts several environment variables
from windows style to POSIX style.

  Here is a patch that does support also Cygwin special variables.
This patch applies on top of my previously submitted patch
to separate out Cygwin/mingw and ANSI/Unicode specific code.
http://sourceware.org/ml/gdb-patches/2011-04/msg00328.html

  The patch allows to propagate locally modified
environment variables to inferior for both Cygwin and mingw
compilation hosts.

  There list of cygwin converted variables was
extracted from  from winsup/cygwin/environ.cc source,
from January 2011.

  Comments welcome,

Pierre Muller



2011-04-20  Pierre Muller  <muller@ics.u-strasbg.fr>

	Add support for locally modifed environment variables in windows
	native hosts.
	(win_env): New type.
	(conv_envvars): New array of type win_env.
	(cygwin_convert_envs, cywin_restore_envs): New functions using
	CONV_ENVVARS array to convert Cygwin environment variables
	back and forth to Windows style.
	(windows_create_inferior): Add support for setting 7th parameter
	of CreateProcess call containing possibly locally modified
	environment variables.

diff --git a/gdb/windows-nat.c b/gdb/windows-nat.c
index 9b49e6e..90e52af 100644
--- a/gdb/windows-nat.c
+++ b/gdb/windows-nat.c
@@ -1967,6 +1967,95 @@ windows_set_console_info (STARTUPINFO *si, DWORD
*flags)
   *flags |= CREATE_NEW_CONSOLE;
 }
 
+#ifdef __CYGWIN__
+/* List of names which are converted from dos to unix on the way in
+   and back again on the way out.
+   PATH needs to be here because CreateProcess uses it and gdb uses
+   CreateProcess.  HOME is here because most shells use it and would be
+   confused by Windows style path names.  */
+typedef struct struct_win_env {
+    char *name;
+    int in_index;
+    void *in_orig_val;
+    int is_list;
+} win_env;
+
+/* This list is extracted from cygwin/environ.cc source,
+   list from January 2011.  */
+static win_env conv_envvars[] =
+  {
+    {"PATH=", -1, NULL, 1},
+    {"HOME=", -1, NULL, 0},
+    {"LD_LIBRARY_PATH=", -1, NULL, 1},
+    {"TMPDIR=", -1, NULL, 0},
+    {"TMP=", -1, NULL, 0},
+    {"TEMP=", -1, NULL, 0},
+    {NULL, -1, NULL, 0}
+  };
+
+/* cgwin_convert_envs function uses conv_envvars array above to
+   convert Cygwin environment variables back to win32 format.  */
+
+static void
+cygwin_convert_envs (char **in_env)
+{
+  int i, j;
+
+  for (i = 0; in_env[i]; i++)
+    {
+      for (j = 0; conv_envvars[j].name; j++)
+	{
+	  char *name = conv_envvars[j].name;
+	  int nlen = strlen(name);
+
+	  if (strncmp (in_env[i], name, nlen) == 0)
+	    {
+	      char *conv;
+	      int len;
+
+	      /* We found this environment variable that we need to convert.
*/
+	      conv_envvars[j].in_index = i;
+	      conv_envvars[j].in_orig_val = in_env[i];
+	      if (conv_envvars[j].is_list)
+		{
+		  len = cygwin_conv_path_list (CCP_POSIX_TO_WIN_A,
+					       &in_env[i][nlen], NULL, 0);
+		  conv = (char *) alloca (len + nlen + 1);
+		  strcpy (conv, name);
+		  cygwin_conv_path_list (CCP_POSIX_TO_WIN_A,
+					 &in_env[i][nlen], &conv[nlen],
len);
+		}
+	      else
+		{
+		  conv = (char *) alloca (__PMAX + nlen);
+		  strcpy (conv, name);
+		  cygwin_conv_path (CCP_POSIX_TO_WIN_A,
+				    &in_env[i][nlen], &conv[nlen], __PMAX);
+		}
+	      in_env[i] = xstrdup (conv);
+	    }
+	}
+    }
+}
+
+/* cygwin_restore_envs function restores in_env elements to their
+   original value.  */
+
+static void
+cygwin_restore_envs (char **in_env)
+{
+  int i, j;
+
+  for (j = 0; conv_envvars[j].name; j++)
+    if (conv_envvars[j].in_index != -1)
+      {
+	xfree (in_env[conv_envvars[j].in_index]);
+        in_env[conv_envvars[j].in_index] = conv_envvars[j].in_orig_val;
+      }
+}
+
+#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.
@@ -1983,6 +2072,9 @@ windows_create_inferior (struct target_ops *ops, char
*exec_file,
   win_buf_t *toexec;
   win_buf_t *cygallargs;
   win_buf_t *args;
+  size_t env_size;
+  int i;
+  win_buf_t *out_env;
 #ifdef __USEWIDE
   size_t len;
 #endif
@@ -2010,6 +2102,7 @@ windows_create_inferior (struct target_ops *ops, char
*exec_file,
     windows_set_console_info (&si, &flags);
 
 #ifdef __CYGWIN__
+  cygwin_convert_envs (in_env);
   if (!useshell)
 #endif
     {
@@ -2071,15 +2164,43 @@ windows_create_inferior (struct target_ops *ops,
char *exec_file,
   wcscpy (args, toexec);
   wcscat (args, L" ");
   wcscat (args, cygallargs);
+  env_size = 1;
+  for (i = 0; in_env[i]; i++)
+    {
+      env_size += mbstowcs (NULL, in_env[i], 0) + 1;
+    }
+  out_env = (win_buf_t *) alloca (env_size * sizeof (win_buf_t *));
+  env_size = 0;
+  for (i = 0; in_env[i]; i++)
+    {
+      int len = mbstowcs (NULL, in_env[i], 0) + 1;
+      mbstowcs (&out_env[env_size], in_env[i], len);
+      env_size += len;
+    }
+  out_env[env_size] = L'\0';
+  flags |= CREATE_UNICODE_ENVIRONMENT;
 #else
   args = (win_buf_t *) alloca (strlen (toexec) + strlen (cygallargs) + 2);
   strcpy (args, toexec);
   strcat (args, " ");
   strcat (args, cygallargs);
+  env_size = 1;
+  for (i = 0; in_env[i]; i++)
+    {
+      env_size += strlen(in_env[i]) + 1;
+    }
+  out_env = (win_buf_t *) alloca (env_size * sizeof (win_buf_t *));
+  env_size = 0;
+  for (i = 0; in_env[i]; i++)
+    {
+      int len = strlen(in_env[i]) + 1;
+      strcpy (&out_env[env_size], in_env[i]);
+      env_size += len;
+    }
+  out_env[env_size] = '\0';
 #endif
 
 #ifdef __CYGWIN__
-  /* Prepare the environment vars for CreateProcess.  */
   cygwin_internal (CW_SYNC_WINENV);
 
   if (!inferior_io_terminal)
@@ -2137,11 +2258,13 @@ windows_create_inferior (struct target_ops *ops,
char *exec_file,
 		       NULL,	/* thread */
 		       TRUE,	/* inherit handles */
 		       flags,	/* start flags */
-		       NULL,	/* environment */
+		       out_env,	/* environment */
 		       NULL,	/* current directory */
 		       &si,
 		       &pi);
 #ifdef __CYGWIN__
+  cygwin_restore_envs (in_env);
+
   if (tty >= 0)
     {
       close (tty);

Re: [RFC] Add support for locally modified environment variables for windows-nat.c

[Corinna Vinschen]

On Apr 20 12:26, Pierre Muller wrote:
> A while ago
> Sebastian Puebla submitted a patch for 
> support of locally modified environment variables.
> 
> http://sourceware.org/ml/gdb-patches/2010-05/msg00317.html
> http://sourceware.org/ml/gdb-patches/2010-08/msg00026.html
> http://sourceware.org/ml/gdb-patches/2010-11/msg00128.html
> 
> Nevertheless, I suspect that his patch would not have worked for Cygwin,
> because Cygwin converts several environment variables
> from windows style to POSIX style.

Do you suspect or did you actually test it?  What you're trying to
accomplish is actually the task of the 

  cygwin_internal (CW_SYNC_WINENV);

call right before calling CreateProcess.  This allows to keep the actual
underlying details of the environment handling entirely in the hands of
Cygwin.  Especially copying an internal list of env vars from Cygwin
into GDB makes me very uncomfortable since this is bound to break as
soon as we decide to handle things a bit different in Cygwin.

Of course it's cgf's call, but I don't think this is the right thing to
do.  If this is actually broken, it's the cygwin_internal(CW_SYNC_WINENV)
implementation in Cygwin which should be fixed, rather than GDB going
out of its way to emulate what happens in Cygwin internally.


Corinna

-- 
Corinna Vinschen
Cygwin Project Co-Leader
Red Hat

RE: [RFC] Add support for locally modified environment variables for windows-nat.c

[Pierre Muller]

> -----Message d'origine-----
> De : gdb-patches-owner@sourceware.org [mailto:gdb-patches-
> owner@sourceware.org] De la part de Corinna Vinschen
> Envoyé : mercredi 20 avril 2011 13:55
> À : gdb-patches@sourceware.org
> Objet : Re: [RFC] Add support for locally modified environment variables for
> windows-nat.c
> 
> On Apr 20 12:26, Pierre Muller wrote:
> > A while ago
> > Sebastian Puebla submitted a patch for
> > support of locally modified environment variables.
> >
> > http://sourceware.org/ml/gdb-patches/2010-05/msg00317.html
> > http://sourceware.org/ml/gdb-patches/2010-08/msg00026.html
> > http://sourceware.org/ml/gdb-patches/2010-11/msg00128.html
> >
> > Nevertheless, I suspect that his patch would not have worked for Cygwin,
> > because Cygwin converts several environment variables
> > from windows style to POSIX style.
> 
> Do you suspect or did you actually test it?  What you're trying to
> accomplish is actually the task of the
> 
>   cygwin_internal (CW_SYNC_WINENV);
> 
> call right before calling CreateProcess.  This allows to keep the actual
> underlying details of the environment handling entirely in the hands of
> Cygwin.  Especially copying an internal list of env vars from Cygwin
> into GDB makes me very uncomfortable since this is bound to break as
> soon as we decide to handle things a bit different in Cygwin.

  The problem is that this does not handle
(gdb) set environment TEST=My dummy test
which is supposed to be passed to the inferior.
after the commands above the in_env char * vector
will have an additional entry "TEST=My dummy test"
but the internal environment variables of GDB itself
are not changed.
  I suspect that cygwin_internal (CW_SYNC_WINENV);
only acts on the environment variables of GDB itself.

Does that mean that we should instead copy this in_env
array using cygwin set_env function before calling cygwin_internal?
If this is true, do we need to restore the environment state of GDB
after?
 
> Of course it's cgf's call, but I don't think this is the right thing to
> do.  If this is actually broken, it's the cygwin_internal(CW_SYNC_WINENV)
> implementation in Cygwin which should be fixed, rather than GDB going
> out of its way to emulate what happens in Cygwin internally.

  Thanks you for explaining the cygwin_internal call.
Of course my current patch does invalidate this as out_env array is used 
instead of the current GDB environment variables.

  If you think that the proposal above is the right track for
Cygwin, I will try to implement this.

Pierre

Re: [RFC] Add support for locally modified environment variables for windows-nat.c

[Corinna Vinschen]

[Please do not CC me.  I'm subscribed to gdb-patches anyway.  Thanks.]


On Apr 20 14:05, Pierre Muller wrote:
> > owner@sourceware.org] De la part de Corinna Vinschen
> > On Apr 20 12:26, Pierre Muller wrote:
> > > Nevertheless, I suspect that his patch would not have worked for Cygwin,
> > > because Cygwin converts several environment variables
> > > from windows style to POSIX style.
> > 
> > Do you suspect or did you actually test it?  What you're trying to
> > accomplish is actually the task of the
> > 
> >   cygwin_internal (CW_SYNC_WINENV);
> > 
> > call right before calling CreateProcess.  This allows to keep the actual
> > underlying details of the environment handling entirely in the hands of
> > Cygwin.  Especially copying an internal list of env vars from Cygwin
> > into GDB makes me very uncomfortable since this is bound to break as
> > soon as we decide to handle things a bit different in Cygwin.
> 
>   The problem is that this does not handle
> (gdb) set environment TEST=My dummy test
> which is supposed to be passed to the inferior.
> after the commands above the in_env char * vector
> will have an additional entry "TEST=My dummy test"
> but the internal environment variables of GDB itself
> are not changed.
>   I suspect that cygwin_internal (CW_SYNC_WINENV);
> only acts on the environment variables of GDB itself.

Yes.

> Does that mean that we should instead copy this in_env
> array using cygwin set_env function before calling cygwin_internal?
> If this is true, do we need to restore the environment state of GDB
> after?

A pity that we still don't have the ptrace API in Cygwin.  But, anyway,
what about this:

  cygwin_internal (CW_SYNC_WINENV);
  for (all env vars set in GDB)
    SetEnvironmentVariableW (wide char representation of var);
  LPWCH out_env = GetEnvironmentStrings ();
  CreateProcess (..., CREATE_UNICODE_ENVIRONMENT, ..., out_env, ...);
  FreeEnvironmentStringsW ();

IMO the environment variables set by the user should *not* be tweaked
in any way.  It's entirely up to the user to set the correct value.
As a Cygwin application GDB doesn't care for its Windows environment
anyway, so there's no reason to revert the changes after the call to
CreateProcess.

>   If you think that the proposal above is the right track for
> Cygwin, I will try to implement this.

I guess we should wait for cgf to express his opinion first.


Corinna

-- 
Corinna Vinschen
Cygwin Project Co-Leader
Red Hat

Re: [RFC] Add support for locally modified environment variables for windows-nat.c

[Christopher Faylor]

There is no need to automatically convert an environment variable.  If
the user specifically sets it that's what the program should see.
Anything else would be very surprising.

cgf