[RFA] testsuite: Add a test for passing of environment variables to inferior

[RFA] testsuite: Add a test for passing of environment variables to inferior

[Pierre Muller]

  Following Eli's comment that it was not clear
what I wanted to achieve, I wrote a new test.

  Manually checking CVS GDB for mingw, Eli's patch
gives the correct output.
  But Stock Cygwin GDB currently passes none of these
TEST_GDB_XXX variables to inferior.
  Even the patch that I sent earlier is not correct:
it still fails for the last test,
once TEST_GDB_VAR1 has been set into GDB environment list,
it doesn't get removed on the last start of the inferior...

  Corinna, I think this is the reason why I wanted to restore
the original environment layout (to avoid leaving unset
variables around.)

  Anyhow, one more test in the testsuite to test
an unchecked feature is always a good thing, no?

  
Pierre

testsuite/ChangeLog entry:

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

	Add tests for passing of environment variables to inferior.
	* gdb.base/testenv.c: New test source.
	* gdb.base/testenv.exp: New expect test.



Index: gdb.base/testenv.c
===================================================================
RCS file: gdb.base/testenv.c
diff -N gdb.base/testenv.c
--- /dev/null	1 Jan 1970 00:00:00 -0000
+++ gdb.base/testenv.c	4 Oct 2011 12:27:05 -0000
@@ -0,0 +1,53 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2011 Free Software Foundation, Inc.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.
*/
+
+/*
+    This source is used to check that GDb correctly
+    passes on environment variables down to inferior.
+    One of the tests checks that 'unset' variables also are removed from
+    inferior environment list.  */
+
+#include <stdio.h>
+
+#ifdef PROTOTYPES
+int main (int argc, char **argv, char **envp)
+#else
+main (argc, argv, envp)
+     int argc;
+     char **argv;
+     char **envp;
+#endif
+{
+    int i, j;
+#ifdef usestubs
+    set_debug_traps();
+    breakpoint();
+#endif
+
+    j = 0;
+    for (i = 0; envp[i]; i++)
+      {
+	if (strncmp ("TEST_GDB", envp[i], 8) == 0)
+	  {
+	    printf ("%s\n", envp[i]);
+	    j++;
+	  }
+      }
+    printf ("Program found %d variables starting with TEST_GDB\n", j);
+    return 0; /* set breakpoint here.  */
+}
+
Index: gdb.base/testenv.exp
===================================================================
RCS file: gdb.base/testenv.exp
diff -N gdb.base/testenv.exp
--- /dev/null	1 Jan 1970 00:00:00 -0000
+++ gdb.base/testenv.exp	4 Oct 2011 12:27:05 -0000
@@ -0,0 +1,94 @@
+# Copyright 2011 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+# This file was written by Pierre Muller <muller@ics.u-strasbg.fr>
+#
+# Check if environment variables are correctly passed to inferiors
+#
+
+if $tracelevel then {
+	strace $tracelevel
+}
+
+
+set testfile "testenv"
+set srcfile ${testfile}.c
+set binfile ${objdir}/${subdir}/${testfile}
+if { [gdb_compile ${srcdir}/${subdir}/${srcfile} ${binfile} executable
{debug}] != "" } {
+     untested testenv.exp
+     return -1
+}
+
+# Start with a fresh gdb
+
+gdb_exit
+gdb_start
+gdb_reinitialize_dir $srcdir/$subdir
+gdb_load ${binfile}
+
+# make sure $pc is sane, in case we're talking to a board.
+if { ![runto_main] } {
+    gdb_suppress_tests;
+}
+
+set bp_line [gdb_get_line_number "set breakpoint here"]
+gdb_breakpoint  $bp_line
+
+#
+# Test gdb set/unset environment commands.
+# Executable lists and counts all environment variables
+# starting with TEST_GDB.
+
+
+# First test with no TEST_GDB_VAR
+gdb_test "continue" \
+  ".*Program found 0 variables starting with TEST_GDB.*" \
+  "Test no TEST_GDB var"
+
+gdb_test_no_output "set env TEST_GDB_VAR1 test1" \
+  "Set TEST_GDB_VAR1"
+
+runto main
+gdb_breakpoint  $bp_line
+
+# Second test with one TEST_GDB_VAR
+gdb_test "continue" \
+  ".*Program found 1 variables starting with TEST_GDB.*" \
+  "Test with one TEST_GDB var"
+
+gdb_test_no_output "set env TEST_GDB_VAR2 test2" \
+  "Set TEST_GDB_VAR2"
+
+runto main
+gdb_breakpoint  $bp_line
+
+# Third test with two TEST_GDB_VAR
+gdb_test "continue"  \
+  ".*Program found 2 variables starting with TEST_GDB.*" \
+  "Test with two TEST_GDB var"
+
+gdb_test_no_output "unset env TEST_GDB_VAR1" \
+  "Unset TEST_GDB_VAR1"
+
+runto main
+gdb_breakpoint  $bp_line
+
+# Fourth test with one TEST_GDB_VAR left, after one was removed
+# with unset command.
+gdb_test "continue"  \
+  ".*Program found 1 variables starting with TEST_GDB.*" \
+  "Test with one TEST_GDB var, after unset"
+
+gdb_exit

Re: [RFA] testsuite: Add a test for passing of environment variables to inferior

[Corinna Vinschen]

On Oct  4 14:36, Pierre Muller wrote:
>   Following Eli's comment that it was not clear
> what I wanted to achieve, I wrote a new test.
> 
>   Manually checking CVS GDB for mingw, Eli's patch
> gives the correct output.
>   But Stock Cygwin GDB currently passes none of these
> TEST_GDB_XXX variables to inferior.
>   Even the patch that I sent earlier is not correct:
> it still fails for the last test,
> once TEST_GDB_VAR1 has been set into GDB environment list,
> it doesn't get removed on the last start of the inferior...
> 
>   Corinna, I think this is the reason why I wanted to restore
> the original environment layout (to avoid leaving unset
> variables around.)

Yes, that would be necessary.  I'm wondering if we can't just utilze the
global environ variable for this and spare us all the hassle.  Something
along these lines:

  char **old_env = environ;
  environ = in_env;
  cygwin_internal (CW_SYNC_WINENV);
  CreateProcessW (NULL environment pointer);
  environ = old_env;

>   Anyhow, one more test in the testsuite to test
> an unchecked feature is always a good thing, no?

No doubt.


Corinna

-- 
Corinna Vinschen
Cygwin Project Co-Leader
Red Hat

Re: [RFA] testsuite: Add a test for passing of environment variables to inferior

[Eli Zaretskii]

> Date: Tue, 4 Oct 2011 15:45:06 +0200
> From: Corinna Vinschen <vinschen@redhat.com>
> 
> Yes, that would be necessary.  I'm wondering if we can't just utilze the
> global environ variable for this and spare us all the hassle.  Something
> along these lines:
> 
>   char **old_env = environ;
>   environ = in_env;
>   cygwin_internal (CW_SYNC_WINENV);
>   CreateProcessW (NULL environment pointer);
>   environ = old_env;

If my reading of sync_winenv is correct, you'd need one more call to
cygwin_internal after restoring `environ'.  Otherwise, the Windows
environment of GDB will be left at the value passed to the inferior,
which could have all kinds of weird unexpected effects elsewhere in
GDB.

RE: [RFA] testsuite: Add a test for passing of environment variables to inferior

[Pierre Muller]

> -----Message d'origine-----
> De : gdb-patches-owner@sourceware.org [mailto:gdb-patches-
> owner@sourceware.org] De la part de Eli Zaretskii
> Envoyé : mardi 4 octobre 2011 16:43
> À : gdb-patches@sourceware.org
> Objet : Re: [RFA] testsuite: Add a test for passing of environment
variables
> to inferior
> 
> > Date: Tue, 4 Oct 2011 15:45:06 +0200
> > From: Corinna Vinschen <vinschen@redhat.com>
> >
> > Yes, that would be necessary.  I'm wondering if we can't just utilze the
> > global environ variable for this and spare us all the hassle.  Something
> > along these lines:
> >
> >   char **old_env = environ;
> >   environ = in_env;
> >   cygwin_internal (CW_SYNC_WINENV);
> >   CreateProcessW (NULL environment pointer);
> >   environ = old_env;
> 
> If my reading of sync_winenv is correct, you'd need one more call to
> cygwin_internal after restoring `environ'.  Otherwise, the Windows
> environment of GDB will be left at the value passed to the inferior,
> which could have all kinds of weird unexpected effects elsewhere in
> GDB.
No, this is not enough, see my next email...

Pierre

RE: [RFA] testsuite: Add a test for passing of environment variables to inferior

[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 15:45
> À : gdb-patches@sourceware.org
> Objet : Re: [RFA] testsuite: Add a test for passing of environment variables
> to inferior
> 
> On Oct  4 14:36, Pierre Muller wrote:
> >   Following Eli's comment that it was not clear
> > what I wanted to achieve, I wrote a new test.
> >
> >   Manually checking CVS GDB for mingw, Eli's patch
> > gives the correct output.
> >   But Stock Cygwin GDB currently passes none of these
> > TEST_GDB_XXX variables to inferior.
> >   Even the patch that I sent earlier is not correct:
> > it still fails for the last test,
> > once TEST_GDB_VAR1 has been set into GDB environment list,
> > it doesn't get removed on the last start of the inferior...
> >
> >   Corinna, I think this is the reason why I wanted to restore
> > the original environment layout (to avoid leaving unset
> > variables around.)
> 
> Yes, that would be necessary.  I'm wondering if we can't just utilze the
> global environ variable for this and spare us all the hassle.  Something
> along these lines:
> 
>   char **old_env = environ;
>   environ = in_env;
>   cygwin_internal (CW_SYNC_WINENV);
>   CreateProcessW (NULL environment pointer);
>   environ = old_env;

  I checked it out, but it still
fails for the last test, the missing TEST_GDB_VAR1
variable doesn't get removed from GDB environment list
apparently (This is probably related to the internals
of cygwin_internal (CW_SYNC_WINENV), no?

  Manually resetting all variables should help...
The patch below does not generate any failure on my test,
but I am still not sure what happens if you try to
remove an environment variable that was set when GDB started...
  Following Elis's comment, I also checked what happens
if I only call cygwin_internal (CW_SYNC_WINENV)
after resetting environ to the GDB value, but
the TEST_GDB_VAR1 doesn't get removed and the last test
fails.

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.

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 14:40:59 -0000
@@ -1980,8 +1980,9 @@ windows_create_inferior (struct target_o
   cygwin_buf_t *toexec;
   cygwin_buf_t *cygallargs;
   cygwin_buf_t *args;
+  char **old_env;
   size_t len;
-  int tty;
+  int tty, i;
   int ostdin, ostdout, ostderr;
 #else
   char real_path[__PMAX];
@@ -2066,6 +2067,8 @@ windows_create_inferior (struct target_o
   strcat (args, cygallargs);
 #endif

+  old_env = environ;
+  environ = in_env;
   /* Prepare the environment vars for CreateProcess.  */
   cygwin_internal (CW_SYNC_WINENV);

@@ -2101,6 +2104,21 @@ windows_create_inferior (struct target_o
                       NULL,    /* current directory */
                       &si,
                       &pi);
+
+  /* Reset all environment variables to avoid leftover on next run. */
+  for (i = 0; in_env[i] && *in_env[i]; i++)
+    {
+      char *equalpos = strchr (in_env[i], '=');
+      if (equalpos)
+       *equalpos = '\0';
+      SetEnvironmentVariableA (in_env[i], NULL);
+      if (equalpos)
+       *equalpos = '=';
+    }
+  /* Restore normal GDB environment variables.  */
+  environ = old_env;
+  cygwin_internal (CW_SYNC_WINENV);
+
   if (tty >= 0)
     {
       close (tty);

Re: [RFA] testsuite: Add a test for passing of environment variables to inferior

[Corinna Vinschen]

On Oct  4 16:49, Pierre Muller wrote:
> > De : gdb-patches-owner@sourceware.org [mailto:gdb-patches-
> > owner@sourceware.org] De la part de Corinna Vinschen
> > Yes, that would be necessary.  I'm wondering if we can't just utilze the
> > global environ variable for this and spare us all the hassle.  Something
> > along these lines:
> > 
> >   char **old_env = environ;
> >   environ = in_env;
> >   cygwin_internal (CW_SYNC_WINENV);
> >   CreateProcessW (NULL environment pointer);
> >   environ = old_env;
> 
>   I checked it out, but it still
> fails for the last test, the missing TEST_GDB_VAR1
> variable doesn't get removed from GDB environment list
> apparently (This is probably related to the internals
> of cygwin_internal (CW_SYNC_WINENV), no?

Right.  The problem is that there's no simple Win32 call to set (or
clear) the Windows environment in one go.  So CW_SYNC_WINENV only sets
but never unsets because that would require to check each Windows
variable if it exists in the POSIX env or not.  That was more effort
than seemed to make sense at the time.

>   Manually resetting all variables should help...

Yes, but that's far from elegant.  I don't mean your patch, but the fact
that it seems to be necessary at all.  Maybe we should introduce some
new cygwin_internal call, which produces a Win32 environment from a
POSIX environment without affecting the Win32 environment of the calling
process:

  LPWCH out_env = (LPWCH) cygwin_internal (CW_CREATE_WIN32_ENV, in_env);
  CreateProcess (..., CREATE_UNICODE_ENVIRONMENT, ..., out_env, ...);
  free (new_env);

But that would only work for newer Cygwin releases, so that's just
a future option.

> The patch below does not generate any failure on my test,
> but I am still not sure what happens if you try to
> remove an environment variable that was set when GDB started...

GDB should not be affected.  Its POSIX environment is still intact and
that's all what's used when interacting with the underlying Cygwin POSIX
calls.  The only variable which should have an effect is $PATH, and that's
restored by the final cygwin_internal(CW_SYNC_WINENV) call.  So, AFAICS,
your patch should be fine.


Corinna

-- 
Corinna Vinschen
Cygwin Project Co-Leader
Red Hat

[RFA-v2] testsuite: Add a test for passing of environment variables to inferior

[Pierre Muller]

  Here is an updated version that also
checks that removing of an environment variable that was set
already on GDB startup works correctly.
  This last added test currently fails for the cygwin patch I submitted earlier...

  So the only solution seems to unset all environment
variables both in environ list before calling
CreateProcess and in in_env after...




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

	Add tests for passing of environment variables to inferior.
	* gdb.base/testenv.c: New test source.
	* gdb.base/testenv.exp: New expect test.



Index: testsuite/gdb.base/testenv.c
===================================================================
RCS file: testsuite/gdb.base/testenv.c
diff -N testsuite/gdb.base/testenv.c
--- /dev/null	1 Jan 1970 00:00:00 -0000
+++ testsuite/gdb.base/testenv.c	4 Oct 2011 16:04:12 -0000
@@ -0,0 +1,53 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2011 Free Software Foundation, Inc.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+/*
+    This source is used to check that GDb correctly
+    passes on environment variables down to inferior.
+    One of the tests checks that 'unset' variables also are removed from
+    inferior environment list.  */
+
+#include <stdio.h>
+
+#ifdef PROTOTYPES
+int main (int argc, char **argv, char **envp)
+#else
+main (argc, argv, envp)
+     int argc;
+     char **argv;
+     char **envp;
+#endif
+{
+    int i, j;
+#ifdef usestubs
+    set_debug_traps();
+    breakpoint();
+#endif
+
+    j = 0;
+    for (i = 0; envp[i]; i++)
+      {
+	if (strncmp ("TEST_GDB", envp[i], 8) == 0)
+	  {
+	    printf ("%s\n", envp[i]);
+	    j++;
+	  }
+      }
+    printf ("Program found %d variables starting with TEST_GDB\n", j);
+    return 0; /* set breakpoint here.  */
+}
+
Index: testsuite/gdb.base/testenv.exp
===================================================================
RCS file: testsuite/gdb.base/testenv.exp
diff -N testsuite/gdb.base/testenv.exp
--- /dev/null	1 Jan 1970 00:00:00 -0000
+++ testsuite/gdb.base/testenv.exp	4 Oct 2011 16:04:12 -0000
@@ -0,0 +1,127 @@
+# Copyright 2011 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+# This file was written by Pierre Muller <muller@ics.u-strasbg.fr>
+#
+# Check if environment variables are correctly passed to inferiors
+#
+
+if $tracelevel then {
+	strace $tracelevel
+}
+
+
+set testfile "testenv"
+set srcfile ${testfile}.c
+set binfile ${objdir}/${subdir}/${testfile}
+if { [gdb_compile ${srcdir}/${subdir}/${srcfile} ${binfile} executable {debug}] != "" } {
+     untested testenv.exp
+     return -1
+}
+
+# Start with a fresh gdb
+
+gdb_exit
+gdb_start
+gdb_reinitialize_dir $srcdir/$subdir
+gdb_load ${binfile}
+
+# make sure $pc is sane, in case we're talking to a board.
+if { ![runto_main] } {
+    gdb_suppress_tests;
+}
+
+set bp_line [gdb_get_line_number "set breakpoint here"]
+gdb_breakpoint  $bp_line
+
+#
+# Test gdb set/unset environment commands.
+# Executable lists and counts all environment variables
+# starting with TEST_GDB.
+
+
+# First test with no TEST_GDB_VAR
+gdb_test "continue" \
+  ".*Program found 0 variables starting with TEST_GDB.*" \
+  "Test no TEST_GDB var"
+
+gdb_test_no_output "set env TEST_GDB_VAR1 test1" \
+  "Set TEST_GDB_VAR1"
+
+runto main
+gdb_breakpoint  $bp_line
+
+# Second test with one TEST_GDB_VAR
+gdb_test "continue" \
+  ".*Program found 1 variables starting with TEST_GDB.*" \
+  "Test with one TEST_GDB var"
+
+gdb_test_no_output "set env TEST_GDB_VAR2 test2" \
+  "Set TEST_GDB_VAR2"
+
+runto main
+gdb_breakpoint  $bp_line
+
+# Third test with two TEST_GDB_VAR
+gdb_test "continue"  \
+  ".*Program found 2 variables starting with TEST_GDB.*" \
+  "Test with two TEST_GDB var"
+
+gdb_test_no_output "unset env TEST_GDB_VAR1" \
+  "Unset TEST_GDB_VAR1"
+
+runto main
+gdb_breakpoint  $bp_line
+
+# Fourth test with one TEST_GDB_VAR left, after one was removed
+# with unset command.
+gdb_test "continue"  \
+  ".*Program found 1 variables starting with TEST_GDB.*" \
+  "Test with one TEST_GDB var, after unset"
+
+gdb_exit
+
+set env(TEST_GDB_GLOBAL) "Global environment value"
+
+gdb_start
+gdb_reinitialize_dir $srcdir/$subdir
+gdb_load ${binfile}
+
+# make sure $pc is sane, in case we're talking to a board.
+if { ![runto_main] } {
+    gdb_suppress_tests;
+}
+
+set bp_line [gdb_get_line_number "set breakpoint here"]
+gdb_breakpoint  $bp_line
+
+gdb_test "show env" ".*TEST_GDB_GLOBAL=.*" "Test passing TEST_GDB_GLOBAL to GDB"
+# First test with only inherited TEST_GDB_GLOBAL
+gdb_test "continue" \
+  ".*TEST_GDB_GLOBAL=Global environment value.*Program found 1 variables starting with TEST_GDB.*" \
+  "Test with TEST_GDB_GLOBAL"
+
+gdb_test_no_output "unset env TEST_GDB_GLOBAL" \
+  "Unset TEST_GDB_GLOBAL"
+
+runto main
+gdb_breakpoint  $bp_line
+
+# Second test with one TEST_GDB_VAR
+gdb_test "continue" \
+  ".*Program found 0 variables starting with TEST_GDB.*" \
+  "Test with TEST_GDB_GLOBAL unset"
+
+gdb_exit

RE: [RFA] testsuite: Add a test for passing of environment variables to inferior

[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 17:13
> À : gdb-patches@sourceware.org
> Objet : Re: [RFA] testsuite: Add a test for passing of environment variables
> to inferior
> 
> On Oct  4 16:49, Pierre Muller wrote:
> > > De : gdb-patches-owner@sourceware.org [mailto:gdb-patches-
> > > owner@sourceware.org] De la part de Corinna Vinschen
> > > Yes, that would be necessary.  I'm wondering if we can't just utilze the
> > > global environ variable for this and spare us all the hassle.  Something
> > > along these lines:
> > >
> > >   char **old_env = environ;
> > >   environ = in_env;
> > >   cygwin_internal (CW_SYNC_WINENV);
> > >   CreateProcessW (NULL environment pointer);
> > >   environ = old_env;
> >
> >   I checked it out, but it still
> > fails for the last test, the missing TEST_GDB_VAR1
> > variable doesn't get removed from GDB environment list
> > apparently (This is probably related to the internals
> > of cygwin_internal (CW_SYNC_WINENV), no?
> 
> Right.  The problem is that there's no simple Win32 call to set (or
> clear) the Windows environment in one go.  So CW_SYNC_WINENV only sets
> but never unsets because that would require to check each Windows
> variable if it exists in the POSIX env or not.  That was more effort
> than seemed to make sense at the time.
> 
> >   Manually resetting all variables should help...
> 
> Yes, but that's far from elegant.  I don't mean your patch, but the fact
> that it seems to be necessary at all.  Maybe we should introduce some
> new cygwin_internal call, which produces a Win32 environment from a
> POSIX environment without affecting the Win32 environment of the calling
> process:
> 
>   LPWCH out_env = (LPWCH) cygwin_internal (CW_CREATE_WIN32_ENV, in_env);
>   CreateProcess (..., CREATE_UNICODE_ENVIRONMENT, ..., out_env, ...);
>   free (new_env);
> 
> But that would only work for newer Cygwin releases, so that's just
> a future option.
> 
> > The patch below does not generate any failure on my test,
> > but I am still not sure what happens if you try to
> > remove an environment variable that was set when GDB started...
> 
> GDB should not be affected.  Its POSIX environment is still intact and
> that's all what's used when interacting with the underlying Cygwin POSIX
> calls.  The only variable which should have an effect is $PATH, and that's
> restored by the final cygwin_internal(CW_SYNC_WINENV) call.  So, AFAICS,
> your patch should be fine.

  Nor completely yet,
here is an update that unsets all environment
variables both in environ list before calling
CreateProcess and in in_env list after...

  This passes the new updated test I just sent.



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

	* windows-nat.c (windows_create_inferior): Handle in_env
	array for Cygwin GDB.
	* testsuite/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 16:05:36 -0000
@@ -1980,8 +1980,9 @@ windows_create_inferior (struct target_o
   cygwin_buf_t *toexec;
   cygwin_buf_t *cygallargs;
   cygwin_buf_t *args;
+  char **old_env;
   size_t len;
-  int tty;
+  int tty, i;
   int ostdin, ostdout, ostderr;
 #else
   char real_path[__PMAX];
@@ -2066,6 +2067,19 @@ windows_create_inferior (struct target_o
   strcat (args, cygallargs);
 #endif
 
+  /* Reset all environment variables to avoid leftover on next run. */
+  for (i = 0; environ[i] && *environ[i]; i++)
+    {
+      char *equalpos;
+      char *copy = alloca (strlen(environ[i]) + 1);
+      strcpy (copy, environ[i]);
+      equalpos = strchr (copy, '=');
+      if (equalpos)
+	*equalpos = '\0';
+      SetEnvironmentVariableA (copy, NULL);
+    }
+  old_env = environ;
+  environ = in_env;
   /* Prepare the environment vars for CreateProcess.  */
   cygwin_internal (CW_SYNC_WINENV);
 
@@ -2101,6 +2115,20 @@ windows_create_inferior (struct target_o
 		       NULL,	/* current directory */
 		       &si,
 		       &pi);
+  /* Reset all environment variables to avoid leftover on next run. */
+  for (i = 0; in_env[i] && *in_env[i]; i++)
+    {
+      char *equalpos = strchr (in_env[i], '=');
+      if (equalpos)
+	*equalpos = '\0';
+      SetEnvironmentVariableA (in_env[i], NULL);
+      if (equalpos)
+	*equalpos = '=';
+    }
+  /* Restore normal GDB environment variables.  */
+  environ = old_env;
+  cygwin_internal (CW_SYNC_WINENV);
+
   if (tty >= 0)
     {
       close (tty);

Re: [RFA-v2] testsuite: Add a test for passing of environment variables to inferior

[Tom Tromey]

>>>>> "Pierre" == Pierre Muller <pierre.muller@ics-cnrs.unistra.fr> writes:

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

Pierre> 	Add tests for passing of environment variables to inferior.
Pierre> 	* gdb.base/testenv.c: New test source.
Pierre> 	* gdb.base/testenv.exp: New expect test.

Thanks for doing this.

Pierre> +#ifdef PROTOTYPES
Pierre> +int main (int argc, char **argv, char **envp)

You don't need to handle the no PROTOTYPES case any more.

Pierre> +if { [gdb_compile ${srcdir}/${subdir}/${srcfile} ${binfile} executable {debug}] != "" } {
Pierre> +     untested testenv.exp
Pierre> +     return -1
Pierre> +}
Pierre> +
Pierre> +# Start with a fresh gdb
Pierre> +
Pierre> +gdb_exit
Pierre> +gdb_start
Pierre> +gdb_reinitialize_dir $srcdir/$subdir
Pierre> +gdb_load ${binfile}

You can use prepare_for_testing instead.

Pierre> +runto main

There's a special runto_main proc for this.

Pierre> +gdb_exit
Pierre> +
Pierre> +set env(TEST_GDB_GLOBAL) "Global environment value"
Pierre> +
Pierre> +gdb_start
Pierre> +gdb_reinitialize_dir $srcdir/$subdir
Pierre> +gdb_load ${binfile}

You can use clean_restart here.

Tom

[RFA-v3] testsuite: Add a test for passing of environment variables to inferior

[Pierre Muller]

> -----Message d'origine-----
> De : gdb-patches-owner@sourceware.org [mailto:gdb-patches-
> owner@sourceware.org] De la part de Tom Tromey
> Envoyé : mardi 4 octobre 2011 19:27
> À : Pierre Muller
> Cc : gdb-patches@sourceware.org
> Objet : Re: [RFA-v2] testsuite: Add a test for passing of environment
> variables to inferior
> 
> >>>>> "Pierre" == Pierre Muller <pierre.muller@ics-cnrs.unistra.fr>
writes:
> 
> Pierre> 2011-10-04  Pierre Muller  <muller@ics.u-strasbg.fr>
> 
> Pierre> 	Add tests for passing of environment variables to inferior.
> Pierre> 	* gdb.base/testenv.c: New test source.
> Pierre> 	* gdb.base/testenv.exp: New expect test.
> 
> Thanks for doing this.
> 
> Pierre> +#ifdef PROTOTYPES
> Pierre> +int main (int argc, char **argv, char **envp)
> 
> You don't need to handle the no PROTOTYPES case any more.
Removed 
> Pierre> +if { [gdb_compile ${srcdir}/${subdir}/${srcfile} ${binfile}
> executable {debug}] != "" } {
> Pierre> +     untested testenv.exp
> Pierre> +     return -1
> Pierre> +}
> Pierre> +
> Pierre> +# Start with a fresh gdb
> Pierre> +
> Pierre> +gdb_exit
> Pierre> +gdb_start
> Pierre> +gdb_reinitialize_dir $srcdir/$subdir
> Pierre> +gdb_load ${binfile}
> 
> You can use prepare_for_testing instead.
Used instead. 
> Pierre> +runto main
> 
> There's a special runto_main proc for this.
> 
> Pierre> +gdb_exit
> Pierre> +
> Pierre> +set env(TEST_GDB_GLOBAL) "Global environment value"
> Pierre> +
> Pierre> +gdb_start
> Pierre> +gdb_reinitialize_dir $srcdir/$subdir
> Pierre> +gdb_load ${binfile}
> 
> You can use clean_restart here.
Also changed.

  Thanks for all your comments,
here is an updated patch
that takes all your suggestions into account.


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

	Add tests for passing of environment variables to inferior.
	* gdb.base/testenv.c: New test source.
	* gdb.base/testenv.exp: New expect test.



Index: gdb.base/testenv.c
===================================================================
RCS file: gdb.base/testenv.c
diff -N gdb.base/testenv.c
--- /dev/null	1 Jan 1970 00:00:00 -0000
+++ gdb.base/testenv.c	4 Oct 2011 21:32:05 -0000
@@ -0,0 +1,47 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2011 Free Software Foundation, Inc.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.
*/
+
+/*
+    This source is used to check that GDB correctly
+    passes on environment variables down to inferior.
+    One of the tests checks that 'unset' variables also are removed from
+    inferior environment list.  */
+
+#include <stdio.h>
+
+int main (int argc, char **argv, char **envp)
+
+{
+    int i, j;
+#ifdef usestubs
+    set_debug_traps();
+    breakpoint();
+#endif
+
+    j = 0;
+    for (i = 0; envp[i]; i++)
+      {
+	if (strncmp ("TEST_GDB", envp[i], 8) == 0)
+	  {
+	    printf ("%s\n", envp[i]);
+	    j++;
+	  }
+      }
+    printf ("Program found %d variables starting with TEST_GDB\n", j);
+    return 0; /* set breakpoint here.  */
+}
+
Index: gdb.base/testenv.exp
===================================================================
RCS file: gdb.base/testenv.exp
diff -N gdb.base/testenv.exp
--- /dev/null	1 Jan 1970 00:00:00 -0000
+++ gdb.base/testenv.exp	4 Oct 2011 21:32:05 -0000
@@ -0,0 +1,121 @@
+# Copyright 2011 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+# This file was written by Pierre Muller <muller@ics.u-strasbg.fr>
+#
+# Check if environment variables are correctly passed to inferiors
+#
+
+if $tracelevel then {
+	strace $tracelevel
+}
+
+
+set testfile "testenv"
+set srcfile ${testfile}.c
+set binfile ${testfile}
+
+# Compile binary
+# and start with a fresh gdb
+
+if { [prepare_for_testing ${testfile}.exp ${binfile} ${srcfile}] } {
+     return -1
+}
+
+# make sure $pc is sane, in case we're talking to a board.
+if { ![runto_main] } {
+    gdb_suppress_tests;
+}
+
+set bp_line [gdb_get_line_number "set breakpoint here"]
+gdb_breakpoint  $bp_line
+
+#
+# Test gdb set/unset environment commands.
+# Executable lists and counts all environment variables
+# starting with TEST_GDB.
+
+
+# First test with no TEST_GDB_VAR
+gdb_test "continue" \
+  ".*Program found 0 variables starting with TEST_GDB.*" \
+  "Test no TEST_GDB var"
+
+gdb_test_no_output "set env TEST_GDB_VAR1 test1" \
+  "Set TEST_GDB_VAR1"
+
+runto_main
+gdb_breakpoint  $bp_line
+
+# Second test with one TEST_GDB_VAR
+gdb_test "continue" \
+  ".*Program found 1 variables starting with TEST_GDB.*" \
+  "Test with one TEST_GDB var"
+
+gdb_test_no_output "set env TEST_GDB_VAR2 test2" \
+  "Set TEST_GDB_VAR2"
+
+runto_main
+gdb_breakpoint  $bp_line
+
+# Third test with two TEST_GDB_VAR
+gdb_test "continue"  \
+  ".*Program found 2 variables starting with TEST_GDB.*" \
+  "Test with two TEST_GDB var"
+
+gdb_test_no_output "unset env TEST_GDB_VAR1" \
+  "Unset TEST_GDB_VAR1"
+
+runto_main
+gdb_breakpoint  $bp_line
+
+# Fourth test with one TEST_GDB_VAR left, after one was removed
+# with unset command.
+gdb_test "continue"  \
+  ".*Program found 1 variables starting with TEST_GDB.*" \
+  "Test with one TEST_GDB var, after unset"
+
+gdb_exit
+
+set env(TEST_GDB_GLOBAL) "Global environment value"
+
+clean_restart $binfile
+
+# make sure $pc is sane, in case we're talking to a board.
+if { ![runto_main] } {
+    gdb_suppress_tests;
+}
+
+set bp_line [gdb_get_line_number "set breakpoint here"]
+gdb_breakpoint  $bp_line
+
+gdb_test "show env" ".*TEST_GDB_GLOBAL=.*" "Test passing TEST_GDB_GLOBAL to
GDB"
+# First test with only inherited TEST_GDB_GLOBAL
+gdb_test "continue" \
+  ".*TEST_GDB_GLOBAL=Global environment value.*Program found 1 variables
starting with TEST_GDB.*" \
+  "Test with TEST_GDB_GLOBAL"
+
+gdb_test_no_output "unset env TEST_GDB_GLOBAL" \
+  "Unset TEST_GDB_GLOBAL"
+
+runto_main
+gdb_breakpoint  $bp_line
+
+# Second test with one TEST_GDB_VAR
+gdb_test "continue" \
+  ".*Program found 0 variables starting with TEST_GDB.*" \
+  "Test with TEST_GDB_GLOBAL unset"
+
+gdb_exit

Re: [RFA] testsuite: Add a test for passing of environment variables to inferior

[Corinna Vinschen]

On Oct  4 18:11, Pierre Muller wrote:
> here is an update that unsets all environment
> variables both in environ list before calling
> CreateProcess and in in_env list after...
> 
>   This passes the new updated test I just sent.

Oh boy, I guess there's no way around that for now.  I will create
a new cygwin_internal call for just such a scenario, as proposed in
http://sourceware.org/ml/gdb-patches/2011-10/msg00079.html and add a
patch to GDB to use it if it's available.  That way, we can limit this
complicated method to current and older versions of Cygwin.

> +  /* Reset all environment variables to avoid leftover on next run. */
> +  for (i = 0; environ[i] && *environ[i]; i++)
> +    {
> +      char *equalpos;
> +      char *copy = alloca (strlen(environ[i]) + 1);

If the environment is very large, using alloca here in a loop might
result in a stack overflow.  Pretty unlikely, I assume, but possible.

> +      strcpy (copy, environ[i]);
> +      equalpos = strchr (copy, '=');
> +      if (equalpos)
> +	*equalpos = '\0';
> +      SetEnvironmentVariableA (copy, NULL);

Since Cygwin 1.7, the Cygwin multibyte charset does not correspond with
the current Windows ANSI codepage.  Cygwin's default multibyte charset
is UTF-8, for instance, while the ANSI Windows functions will use some
arbitrary Windows codepage depending on system and user language.  So we
should convert the variables to UNICODE and use the corresponding Win32
function.

Here's my proposal, based on your patch.  I'll work on using the
yet-to-be-created new cygwin_internal call after I implemented it in
Cygwin.

	* windows-nat.c: Include wchar.h to avoid compiler warnings.
	(clear_win32_environment): New function for Cygwin to clear out
	Win32 environment.
	(windows_create_inferior): Prepare new environment from in_env
	for Cygwin, too.

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	5 Oct 2011 11:36:10 -0000
@@ -40,6 +40,7 @@
 #include <imagehlp.h>
 #include <psapi.h>
 #ifdef __CYGWIN__
+#include <wchar.h>
 #include <sys/cygwin.h>
 #endif
 #include <signal.h>
@@ -1963,6 +1964,28 @@ envvar_cmp (const void *a, const void *b
 }
 #endif
 
+#ifdef __CYGWIN__
+static void
+clear_win32_environment (char **env)
+{
+  int i;
+  size_t len;
+  wchar_t *copy = NULL, *equalpos;
+
+  for (i = 0; env[i] && *env[i]; i++)
+    {
+      len = mbstowcs (NULL, env[i], 0) + 1;
+      copy = (wchar_t *) xrealloc (copy, len * sizeof (wchar_t));
+      mbstowcs (copy, env[i], len);
+      equalpos = wcschr (copy, L'=');
+      if (equalpos)
+        *equalpos = L'\0';
+      SetEnvironmentVariableW (copy, NULL);
+    }
+  xfree (copy);
+}
+#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.
@@ -1980,6 +2003,7 @@ windows_create_inferior (struct target_o
   cygwin_buf_t *toexec;
   cygwin_buf_t *cygallargs;
   cygwin_buf_t *args;
+  char **old_env;
   size_t len;
   int tty;
   int ostdin, ostdout, ostderr;
@@ -2066,7 +2090,11 @@ windows_create_inferior (struct target_o
   strcat (args, cygallargs);
 #endif
 
+  /* Reset all Win32 environment variables to avoid leftover on next run. */
+  clear_win32_environment (environ);
   /* Prepare the environment vars for CreateProcess.  */
+  old_env = environ;
+  environ = in_env;
   cygwin_internal (CW_SYNC_WINENV);
 
   if (!inferior_io_terminal)
@@ -2101,6 +2129,12 @@ windows_create_inferior (struct target_o
 		       NULL,	/* current directory */
 		       &si,
 		       &pi);
+  /* Reset all environment variables to avoid leftover on next run. */
+  clear_win32_environment (in_env);
+  /* Restore normal GDB environment variables.  */
+  environ = old_env;
+  cygwin_internal (CW_SYNC_WINENV);
+
   if (tty >= 0)
     {
       close (tty);


Corinna

-- 
Corinna Vinschen
Cygwin Project Co-Leader
Red Hat

RE: [RFA] testsuite: Add a test for passing of environment variables to inferior

[Pierre Muller]

  Hi Corinna,
 I didn't know about the charset problem...

> Here's my proposal, based on your patch.  I'll work on using the
> yet-to-be-created new cygwin_internal call after I implemented it in
> Cygwin.

  I think that your approach is OK,
I checked that it does also pass all
the tests in my gdb.base/testenv.exp
(11 passes instead of 7 passes/4 failures for current Cygwin GDB 7.3.50 20110821cvs).

Your patch proposal thus supersedes mine.

 We just need an approval from Christopher Faylor...


Pierre

Re: [RFA] testsuite: Add a test for passing of environment variables to inferior

[Corinna Vinschen]

On Oct  5 14:08, Pierre Muller wrote:
>   Hi Corinna,
>  I didn't know about the charset problem...
> 
> > Here's my proposal, based on your patch.  I'll work on using the
> > yet-to-be-created new cygwin_internal call after I implemented it in
> > Cygwin.
> 
>   I think that your approach is OK,
> I checked that it does also pass all
> the tests in my gdb.base/testenv.exp
> (11 passes instead of 7 passes/4 failures for current Cygwin GDB 7.3.50 20110821cvs).
> 
> Your patch proposal thus supersedes mine.
> 
>  We just need an approval from Christopher Faylor...

Right.  Here's a new version of the patch which uses the new
cygwin_internal(CW_CVT_ENV_TO_WINENV) which will be available starting
with the next Cygwin 1.7.10.  The patch allows to build GDB under older
versions of Cygwin and it will fallback to the CW_SYNC_WINENV method if
the cygwin_internal (CW_CVT_ENV_TO_WINENV) call returns with an error.
That allows to run a GDB built under 1.7.10 to run under older Cygwin
versions (provided all other DLL dependencies still work).  Tested under
Cygwin 1.7.9 and current CVS.


Corinna


        * windows-nat.c: Include wchar.h to avoid compiler warnings.
	Include cygwin/version.h to get version information.
        (clear_win32_environment): New function for Cygwin to clear out
        Win32 environment.
        (windows_create_inferior): Prepare new environment from in_env
        for Cygwin, too.  Use cygwin_internal (CW_CVT_ENV_TO_WINENV) call
	to get a Win32 copy of the desired POSIX environment if available,
	fall back to changing the GDB environment otherwise.

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	5 Oct 2011 12:47:40 -0000
@@ -40,7 +40,9 @@
 #include <imagehlp.h>
 #include <psapi.h>
 #ifdef __CYGWIN__
+#include <wchar.h>
 #include <sys/cygwin.h>
+#include <cygwin/version.h>
 #endif
 #include <signal.h>
 
@@ -1963,6 +1965,28 @@ envvar_cmp (const void *a, const void *b
 }
 #endif
 
+#ifdef __CYGWIN__
+static void
+clear_win32_environment (char **env)
+{
+  int i;
+  size_t len;
+  wchar_t *copy = NULL, *equalpos;
+
+  for (i = 0; env[i] && *env[i]; i++)
+    {
+      len = mbstowcs (NULL, env[i], 0) + 1;
+      copy = (wchar_t *) xrealloc (copy, len * sizeof (wchar_t));
+      mbstowcs (copy, env[i], len);
+      equalpos = wcschr (copy, L'=');
+      if (equalpos)
+        *equalpos = L'\0';
+      SetEnvironmentVariableW (copy, NULL);
+    }
+  xfree (copy);
+}
+#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.
@@ -1980,6 +2004,8 @@ windows_create_inferior (struct target_o
   cygwin_buf_t *toexec;
   cygwin_buf_t *cygallargs;
   cygwin_buf_t *args;
+  char **old_env;
+  PWCHAR w32_env;
   size_t len;
   int tty;
   int ostdin, ostdout, ostderr;
@@ -2066,8 +2092,23 @@ windows_create_inferior (struct target_o
   strcat (args, cygallargs);
 #endif
 
-  /* Prepare the environment vars for CreateProcess.  */
-  cygwin_internal (CW_SYNC_WINENV);
+#if CYGWIN_VERSION_API_MAJOR > 0 || CYGWIN_VERSION_API_MINOR >= 252
+  /* First try to create a direct Win32 copy of the POSIX environment. */
+  w32_env = (PWCHAR) cygwin_internal (CW_CVT_ENV_TO_WINENV, in_env);
+  if (w32_env != (PWCHAR) -1)
+    flags |= CREATE_UNICODE_ENVIRONMENT;
+  else
+    /* If that fails, fall back to old method tweaking GDB's environment. */
+#endif
+    {
+      /* Reset all Win32 environment variables to avoid leftover on next run. */
+      clear_win32_environment (environ);
+      /* Prepare the environment vars for CreateProcess.  */
+      old_env = environ;
+      environ = in_env;
+      cygwin_internal (CW_SYNC_WINENV);
+      w32_env = NULL;
+    }
 
   if (!inferior_io_terminal)
     tty = ostdin = ostdout = ostderr = -1;
@@ -2097,10 +2138,22 @@ windows_create_inferior (struct target_o
 		       NULL,	/* thread */
 		       TRUE,	/* inherit handles */
 		       flags,	/* start flags */
-		       NULL,	/* environment */
+		       w32_env,	/* environment */
 		       NULL,	/* current directory */
 		       &si,
 		       &pi);
+  if (w32_env)
+    /* Just free the Win32 environment, if it could be created. */
+    free (w32_env);
+  else
+    {
+      /* Reset all environment variables to avoid leftover on next run. */
+      clear_win32_environment (in_env);
+      /* Restore normal GDB environment variables.  */
+      environ = old_env;
+      cygwin_internal (CW_SYNC_WINENV);
+    }
+
   if (tty >= 0)
     {
       close (tty);

-- 
Corinna Vinschen
Cygwin Project Co-Leader
Red Hat

Re: [RFA-v3] testsuite: Add a test for passing of environment variables to inferior

[Tom Tromey]

>>>>> "Pierre" == Pierre Muller <pierre.muller@ics-cnrs.unistra.fr> writes:

Pierre> 2011-10-04  Pierre Muller  <muller@ics.u-strasbg.fr>
Pierre> 	Add tests for passing of environment variables to inferior.
Pierre> 	* gdb.base/testenv.c: New test source.
Pierre> 	* gdb.base/testenv.exp: New expect test.

Ok.

Tom

RE: [RFA-v3] testsuite: Add a test for passing of environment variables to inferior

[Pierre Muller]

> -----Message d'origine-----
> De : gdb-patches-owner@sourceware.org [mailto:gdb-patches-
> owner@sourceware.org] De la part de Tom Tromey
> Envoyé : mercredi 5 octobre 2011 15:53
> À : Pierre Muller
> Cc : gdb-patches@sourceware.org
> Objet : Re: [RFA-v3] testsuite: Add a test for passing of environment
> variables to inferior
> 
> >>>>> "Pierre" == Pierre Muller <pierre.muller@ics-cnrs.unistra.fr>
writes:
> 
> Pierre> 2011-10-04  Pierre Muller  <muller@ics.u-strasbg.fr>
> Pierre> 	Add tests for passing of environment variables to inferior.
> Pierre> 	* gdb.base/testenv.c: New test source.
> Pierre> 	* gdb.base/testenv.exp: New expect test.
> 
> Ok.


  Thanks, patch committed in:
http://sourceware.org/ml/gdb-cvs/2011-10/msg00028.html

Pierre

Re: [RFA] testsuite: Add a test for passing of environment variables to inferior

[Christopher Faylor]

On Wed, Oct 05, 2011 at 02:51:57PM +0200, Corinna Vinschen wrote:
>On Oct  5 14:08, Pierre Muller wrote:
>>   Hi Corinna,
>>  I didn't know about the charset problem...
>> 
>> > Here's my proposal, based on your patch.  I'll work on using the
>> > yet-to-be-created new cygwin_internal call after I implemented it in
>> > Cygwin.
>> 
>>   I think that your approach is OK,
>> I checked that it does also pass all
>> the tests in my gdb.base/testenv.exp
>> (11 passes instead of 7 passes/4 failures for current Cygwin GDB 7.3.50 20110821cvs).
>> 
>> Your patch proposal thus supersedes mine.
>> 
>>  We just need an approval from Christopher Faylor...
>
>Right.  Here's a new version of the patch which uses the new
>cygwin_internal(CW_CVT_ENV_TO_WINENV) which will be available starting
>with the next Cygwin 1.7.10.  The patch allows to build GDB under older
>versions of Cygwin and it will fallback to the CW_SYNC_WINENV method if
>the cygwin_internal (CW_CVT_ENV_TO_WINENV) call returns with an error.
>That allows to run a GDB built under 1.7.10 to run under older Cygwin
>versions (provided all other DLL dependencies still work).  Tested under
>Cygwin 1.7.9 and current CVS.
>
>
>Corinna
>
>
>        * windows-nat.c: Include wchar.h to avoid compiler warnings.
>	Include cygwin/version.h to get version information.
>        (clear_win32_environment): New function for Cygwin to clear out
>        Win32 environment.
>        (windows_create_inferior): Prepare new environment from in_env
>        for Cygwin, too.  Use cygwin_internal (CW_CVT_ENV_TO_WINENV) call
>	to get a Win32 copy of the desired POSIX environment if available,
>	fall back to changing the GDB environment otherwise.
>
>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	5 Oct 2011 12:47:40 -0000
>@@ -40,7 +40,9 @@
> #include <imagehlp.h>
> #include <psapi.h>
> #ifdef __CYGWIN__
>+#include <wchar.h>
> #include <sys/cygwin.h>
>+#include <cygwin/version.h>
> #endif
> #include <signal.h>
> 
>@@ -1963,6 +1965,28 @@ envvar_cmp (const void *a, const void *b
> }
> #endif
> 
>+#ifdef __CYGWIN__
>+static void
>+clear_win32_environment (char **env)
>+{
>+  int i;
>+  size_t len;
>+  wchar_t *copy = NULL, *equalpos;
>+
>+  for (i = 0; env[i] && *env[i]; i++)
>+    {
>+      len = mbstowcs (NULL, env[i], 0) + 1;
>+      copy = (wchar_t *) xrealloc (copy, len * sizeof (wchar_t));
>+      mbstowcs (copy, env[i], len);
>+      equalpos = wcschr (copy, L'=');
>+      if (equalpos)
>+        *equalpos = L'\0';
>+      SetEnvironmentVariableW (copy, NULL);
>+    }
>+  xfree (copy);
>+}
>+#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.
>@@ -1980,6 +2004,8 @@ windows_create_inferior (struct target_o
>   cygwin_buf_t *toexec;
>   cygwin_buf_t *cygallargs;
>   cygwin_buf_t *args;
>+  char **old_env;
>+  PWCHAR w32_env;
>   size_t len;
>   int tty;
>   int ostdin, ostdout, ostderr;
>@@ -2066,8 +2092,23 @@ windows_create_inferior (struct target_o
>   strcat (args, cygallargs);
> #endif
> 
>-  /* Prepare the environment vars for CreateProcess.  */
>-  cygwin_internal (CW_SYNC_WINENV);
>+#if CYGWIN_VERSION_API_MAJOR > 0 || CYGWIN_VERSION_API_MINOR >= 252

Why not just check if CW_CVT_ENV_TO_WINENV is defined rather than checking
specifically for a version number?  Checking arbitrary versions like this
should be a last resort.

Otherwise, the patch is fine.

cgf

Re: [RFA] testsuite: Add a test for passing of environment variables to inferior

[Corinna Vinschen]

On Oct  6 10:51, Christopher Faylor wrote:
> On Wed, Oct 05, 2011 at 02:51:57PM +0200, Corinna Vinschen wrote:
> >On Oct  5 14:08, Pierre Muller wrote:
> >>   Hi Corinna,
> >>  I didn't know about the charset problem...
> >> 
> >> > Here's my proposal, based on your patch.  I'll work on using the
> >> > yet-to-be-created new cygwin_internal call after I implemented it in
> >> > Cygwin.
> >> 
> >>   I think that your approach is OK,
> >> I checked that it does also pass all
> >> the tests in my gdb.base/testenv.exp
> >> (11 passes instead of 7 passes/4 failures for current Cygwin GDB 7.3.50 20110821cvs).
> >> 
> >> Your patch proposal thus supersedes mine.
> >> 
> >>  We just need an approval from Christopher Faylor...
> >
> >Right.  Here's a new version of the patch which uses the new
> >cygwin_internal(CW_CVT_ENV_TO_WINENV) which will be available starting
> >with the next Cygwin 1.7.10.  The patch allows to build GDB under older
> >versions of Cygwin and it will fallback to the CW_SYNC_WINENV method if
> >the cygwin_internal (CW_CVT_ENV_TO_WINENV) call returns with an error.
> >That allows to run a GDB built under 1.7.10 to run under older Cygwin
> >versions (provided all other DLL dependencies still work).  Tested under
> >Cygwin 1.7.9 and current CVS.
> >
> >
> >Corinna
> >
> >
> >        * windows-nat.c: Include wchar.h to avoid compiler warnings.
> >	Include cygwin/version.h to get version information.
> >        (clear_win32_environment): New function for Cygwin to clear out
> >        Win32 environment.
> >        (windows_create_inferior): Prepare new environment from in_env
> >        for Cygwin, too.  Use cygwin_internal (CW_CVT_ENV_TO_WINENV) call
> >	to get a Win32 copy of the desired POSIX environment if available,
> >	fall back to changing the GDB environment otherwise.
> >
> >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	5 Oct 2011 12:47:40 -0000
> >@@ -40,7 +40,9 @@
> > #include <imagehlp.h>
> > #include <psapi.h>
> > #ifdef __CYGWIN__
> >+#include <wchar.h>
> > #include <sys/cygwin.h>
> >+#include <cygwin/version.h>
> > #endif
> > #include <signal.h>
> > 
> >@@ -1963,6 +1965,28 @@ envvar_cmp (const void *a, const void *b
> > }
> > #endif
> > 
> >+#ifdef __CYGWIN__
> >+static void
> >+clear_win32_environment (char **env)
> >+{
> >+  int i;
> >+  size_t len;
> >+  wchar_t *copy = NULL, *equalpos;
> >+
> >+  for (i = 0; env[i] && *env[i]; i++)
> >+    {
> >+      len = mbstowcs (NULL, env[i], 0) + 1;
> >+      copy = (wchar_t *) xrealloc (copy, len * sizeof (wchar_t));
> >+      mbstowcs (copy, env[i], len);
> >+      equalpos = wcschr (copy, L'=');
> >+      if (equalpos)
> >+        *equalpos = L'\0';
> >+      SetEnvironmentVariableW (copy, NULL);
> >+    }
> >+  xfree (copy);
> >+}
> >+#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.
> >@@ -1980,6 +2004,8 @@ windows_create_inferior (struct target_o
> >   cygwin_buf_t *toexec;
> >   cygwin_buf_t *cygallargs;
> >   cygwin_buf_t *args;
> >+  char **old_env;
> >+  PWCHAR w32_env;
> >   size_t len;
> >   int tty;
> >   int ostdin, ostdout, ostderr;
> >@@ -2066,8 +2092,23 @@ windows_create_inferior (struct target_o
> >   strcat (args, cygallargs);
> > #endif
> > 
> >-  /* Prepare the environment vars for CreateProcess.  */
> >-  cygwin_internal (CW_SYNC_WINENV);
> >+#if CYGWIN_VERSION_API_MAJOR > 0 || CYGWIN_VERSION_API_MINOR >= 252
> 
> Why not just check if CW_CVT_ENV_TO_WINENV is defined rather than checking
> specifically for a version number?  Checking arbitrary versions like this
> should be a last resort.

The CW_foo values are not macros, but enum values.  You can't check
them for being defined.

Possible workaround is to define them twice, once as enum and once
as macro, just as Linux does or just as some Cygwin headers do,
for instance <cygwin/in.h>:

  enum
  {
    IPPROTO_IP = 0,               /* Dummy protocol for TCP               */
    IPPROTO_HOPOPTS = 0,          /* IPv6 Hop-by-Hop options              */
    [...]
  };

  #define IPPROTO_IP IPPROTO_IP
  #define IPPROTO_HOPOPTS IPPROTO_HOPOPTS
  [...]

If we do that in <sys/cygwin.h> as well, I can change the test to
#ifdef CW_CVT_ENV_TO_WINENV.  What do you think?


Corinna

-- 
Corinna Vinschen
Cygwin Project Co-Leader
Red Hat

Re: [RFA] testsuite: Add a test for passing of environment variables to inferior

[Christopher Faylor]

On Thu, Oct 06, 2011 at 05:02:56PM +0200, Corinna Vinschen wrote:
>On Oct  6 10:51, Christopher Faylor wrote:
>> Why not just check if CW_CVT_ENV_TO_WINENV is defined rather than checking
>> specifically for a version number?  Checking arbitrary versions like this
>> should be a last resort.
>
>The CW_foo values are not macros, but enum values.  You can't check
>them for being defined.
>
>Possible workaround is to define them twice, once as enum and once
>as macro, just as Linux does or just as some Cygwin headers do,
>for instance <cygwin/in.h>:
>
>  enum
>  {
>    IPPROTO_IP = 0,               /* Dummy protocol for TCP               */
>    IPPROTO_HOPOPTS = 0,          /* IPv6 Hop-by-Hop options              */
>    [...]
>  };
>
>  #define IPPROTO_IP IPPROTO_IP
>  #define IPPROTO_HOPOPTS IPPROTO_HOPOPTS
>  [...]
>
>If we do that in <sys/cygwin.h> as well, I can change the test to
>#ifdef CW_CVT_ENV_TO_WINENV.  What do you think?

Ok.

cgf

Re: [RFA] testsuite: Add a test for passing of environment variables to inferior

[Corinna Vinschen]

On Oct  7 08:25, Christopher Faylor wrote:
> On Thu, Oct 06, 2011 at 05:02:56PM +0200, Corinna Vinschen wrote:
> >On Oct  6 10:51, Christopher Faylor wrote:
> >> Why not just check if CW_CVT_ENV_TO_WINENV is defined rather than checking
> >> specifically for a version number?  Checking arbitrary versions like this
> >> should be a last resort.
> >
> >The CW_foo values are not macros, but enum values.  You can't check
> >them for being defined.
> >
> >Possible workaround is to define them twice, once as enum and once
> >as macro, just as Linux does or just as some Cygwin headers do,
> >for instance <cygwin/in.h>:
> >
> >  enum
> >  {
> >    IPPROTO_IP = 0,               /* Dummy protocol for TCP               */
> >    IPPROTO_HOPOPTS = 0,          /* IPv6 Hop-by-Hop options              */
> >    [...]
> >  };
> >
> >  #define IPPROTO_IP IPPROTO_IP
> >  #define IPPROTO_HOPOPTS IPPROTO_HOPOPTS
> >  [...]
> >
> >If we do that in <sys/cygwin.h> as well, I can change the test to
> >#ifdef CW_CVT_ENV_TO_WINENV.  What do you think?
> 
> Ok.

Thanks, patch with this change applied.


Corinna

-- 
Corinna Vinschen
Cygwin Project Co-Leader
Red Hat