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

Pierre Muller
2011-10-04T09:56:56Z

> -----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"