[RFA] win32-nat.c: Simplify generation of Windows environment

[RFA] win32-nat.c: Simplify generation of Windows environment

[Corinna Vinschen]

Hi,

the below patch simplifies the code which translates the Cygwin
environment into the native Windows environment.  So far this is
done in GDB manually.  However, there's a Cygwin specific function
call which does the same for the calling process.  Using this call
has three advantages.

- We can drop a rather big chunk of code from GDB which should be the
  task of Cygwin anyway.
- By using the Cygwin method of converting the environment, we take
  care of all environment variables which have to be converted in
  some way; not only the PATH variable, but all variables which are
  also translated by Cygwin, thus making this process more transparent.
- Subsequent changes in Cygwin don't require to change GDB.


Ok to apply?


Corinna


	* win32-nat.c (env_sort): Remove.
	(win32_create_inferior): Remove code which creates a Windows
	environment.  Use Cygwin function call instead.  Propagate
	current environment to inferior process.


Index: win32-nat.c
===================================================================
RCS file: /cvs/src/src/gdb/win32-nat.c,v
retrieving revision 1.123
diff -u -p -r1.123 win32-nat.c
--- win32-nat.c	21 May 2006 23:04:39 -0000	1.123
+++ win32-nat.c	7 Dec 2006 09:56:15 -0000
@@ -1842,15 +1842,6 @@ win32_open (char *arg, int from_tty)
   error (_("Use the \"run\" command to start a Unix child process."));
 }
 
-/* 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);
-}
-
 /* Start an inferior win32 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.
@@ -1918,83 +1909,7 @@ win32_create_inferior (char *exec_file, 
   strcat (args, allargs);
 
   /* Prepare the environment vars for CreateProcess.  */
-  {
-    /* This code used to assume all env vars were file names and would
-       translate them all to win32 style.  That obviously doesn't work in the
-       general case.  The current rule is that we only translate PATH.
-       We need to handle PATH because we're about to call CreateProcess and
-       it uses PATH to find DLL's.  Fortunately PATH has a well-defined value
-       in both posix and win32 environments.  cygwin.dll will change it back
-       to posix style if necessary.  */
-
-    static const char *conv_path_names[] =
-    {
-      "PATH=",
-      0
-    };
-
-    /* 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++)
-      {
-	int j, len;
-
-	for (j = 0; conv_path_names[j]; j++)
-	  {
-	    len = strlen (conv_path_names[j]);
-	    if (strncmp (conv_path_names[j], in_env[i], len) == 0)
-	      {
-		if (cygwin_posix_path_list_p (in_env[i] + len))
-		  envlen += len
-		    + cygwin_posix_to_win32_path_list_buf_size (in_env[i] + len);
-		else
-		  envlen += strlen (in_env[i]) + 1;
-		break;
-	      }
-	  }
-	if (conv_path_names[j] == NULL)
-	  envlen += strlen (in_env[i]) + 1;
-      }
-
-    size_t envsize = sizeof (in_env[0]) * (i + 1);
-    char **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);
-
-    winenv = alloca (envlen + 1);
-
-    /* Copy env strings into new buffer.  */
-    for (temp = winenv, i = 0; env[i] && *env[i]; i++)
-      {
-	int j, len;
-
-	for (j = 0; conv_path_names[j]; j++)
-	  {
-	    len = strlen (conv_path_names[j]);
-	    if (strncmp (conv_path_names[j], env[i], len) == 0)
-	      {
-		if (cygwin_posix_path_list_p (env[i] + len))
-		  {
-		    memcpy (temp, env[i], len);
-		    cygwin_posix_to_win32_path_list (env[i] + len, temp + len);
-		  }
-		else
-		  strcpy (temp, env[i]);
-		break;
-	      }
-	  }
-	if (conv_path_names[j] == NULL)
-	  strcpy (temp, env[i]);
-
-	temp += strlen (temp) + 1;
-      }
-
-    /* Final nil string to terminate new env.  */
-    *temp = 0;
-  }
+  cygwin_internal (CW_SYNC_WINENV);
 
   if (!inferior_io_terminal)
     tty = ostdin = ostdout = ostderr = -1;
@@ -2024,7 +1939,7 @@ win32_create_inferior (char *exec_file, 
 		       NULL,	/* thread */
 		       TRUE,	/* inherit handles */
 		       flags,	/* start flags */
-		       winenv,
+		       NULL,	/* environment */
 		       NULL,	/* current directory */
 		       &si,
 		       &pi);

-- 
Corinna Vinschen
Cygwin Project Co-Leader
Red Hat

Re: [RFA] win32-nat.c: Simplify generation of Windows environment

[Corinna Vinschen]

On Dec  7 10:58, Corinna Vinschen wrote:
> Hi,
> 
> the below patch simplifies the code which translates the Cygwin
> environment into the native Windows environment.  [...]

The patch forgets to remove the local variables which are exclusively
used to create the windows environment, namely winenv, temp, envlen
and i.  I fixed that in my local patch.  I assume that doesn't require
to resend the full patch.


Corinna

-- 
Corinna Vinschen
Cygwin Project Co-Leader
Red Hat

Re: [RFA] win32-nat.c: Simplify generation of Windows environment

[Jim Blandy]

Corinna Vinschen <vinschen@redhat.com> writes:
> the below patch simplifies the code which translates the Cygwin
> environment into the native Windows environment.  So far this is
> done in GDB manually.  However, there's a Cygwin specific function
> call which does the same for the calling process.  Using this call
> has three advantages.
>
> - We can drop a rather big chunk of code from GDB which should be the
>   task of Cygwin anyway.
> - By using the Cygwin method of converting the environment, we take
>   care of all environment variables which have to be converted in
>   some way; not only the PATH variable, but all variables which are
>   also translated by Cygwin, thus making this process more transparent.
> - Subsequent changes in Cygwin don't require to change GDB.
>
>
> Ok to apply?

I'm very much inclined to take your advice on Cygwin-related issues,
and I love the deletion of code, but I still have some questions:

Is it really okay to call cygwin_internal?  (That's not the name I'd
expect a public, stable interface to have.)

How well will this work on older versions of Cygwin?  Will people
still be able to compile GDB against the Cygwin versions they can now?

Re: [RFA] win32-nat.c: Simplify generation of Windows environment

[Corinna Vinschen]

On Dec  7 11:49, Jim Blandy wrote:
> Corinna Vinschen <vinschen@redhat.com> writes:
> > the below patch simplifies the code which translates the Cygwin
> > environment into the native Windows environment.  So far this is
> > done in GDB manually.  However, there's a Cygwin specific function
> > call which does the same for the calling process.  Using this call
> > has three advantages.
> >
> > - We can drop a rather big chunk of code from GDB which should be the
> >   task of Cygwin anyway.
> > - By using the Cygwin method of converting the environment, we take
> >   care of all environment variables which have to be converted in
> >   some way; not only the PATH variable, but all variables which are
> >   also translated by Cygwin, thus making this process more transparent.
> > - Subsequent changes in Cygwin don't require to change GDB.
> >
> >
> > Ok to apply?
> 
> I'm very much inclined to take your advice on Cygwin-related issues,
> and I love the deletion of code, but I still have some questions:
> 
> Is it really okay to call cygwin_internal?  (That's not the name I'd
> expect a public, stable interface to have.)

cygwin_internal is a stable interface existing for a lot of years.  It's
supposed to provide an interface to Cygwin internal functionality which
is not covered by any "official" POSIX/Linux/BSD API.  Functionality is
never removed from this function.

It's used already for quite some time in gdb:

$ grep -n cygwin_internal win32-nat.c
1738:      pid = cygwin_internal (CW_CYGWIN_PID_TO_WINPID, pid);
1813:  cygwin_internal (CW_LOCK_PINFO, 1000);
1816:   cygwin_internal (CW_GETPINFO, cpid | CW_NEXTPID));
1826:  cygwin_internal (CW_UNLOCK_PINFO);

> How well will this work on older versions of Cygwin?  Will people
> still be able to compile GDB against the Cygwin versions they can now?

The interface exists since at least 1999, but CW_SYNC_WINENV has been
introduced in February 2006.  This requires at least Cygwin 1.5.21,
which is the version before the current version.  I didn't expect that
we're trying to support old Cygwin versions in new GDB versions,
though.


Corinna

-- 
Corinna Vinschen
Cygwin Project Co-Leader
Red Hat

Re: [RFA] win32-nat.c: Simplify generation of Windows environment

[Eli Zaretskii]

> Date: Thu, 7 Dec 2006 10:58:39 +0100
> From: Corinna Vinschen <vinschen@redhat.com>
> 
> the below patch simplifies the code which translates the Cygwin
> environment into the native Windows environment.  So far this is
> done in GDB manually.  However, there's a Cygwin specific function
> call which does the same for the calling process.  Using this call
> has three advantages.

Is win32-nat.c used only for the Cygwin build?  I thought the native
Windows build used it as well (perhaps with a few patches that are not
yet part of the CVS), but maybe I was mistaken.

If I am right, then please make this change, and especially the call
to cygwin_internal, conditioned on __CYGWIN__, or some other
Cygwin-specific symbol.

Re: [RFA] win32-nat.c: Simplify generation of Windows environment

[Jim Blandy]

Eli Zaretskii <eliz@gnu.org> writes:
>> Date: Thu, 7 Dec 2006 10:58:39 +0100
>> From: Corinna Vinschen <vinschen@redhat.com>
>> 
>> the below patch simplifies the code which translates the Cygwin
>> environment into the native Windows environment.  So far this is
>> done in GDB manually.  However, there's a Cygwin specific function
>> call which does the same for the calling process.  Using this call
>> has three advantages.
>
> Is win32-nat.c used only for the Cygwin build?  I thought the native
> Windows build used it as well (perhaps with a few patches that are not
> yet part of the CVS), but maybe I was mistaken.
>
> If I am right, then please make this change, and especially the call
> to cygwin_internal, conditioned on __CYGWIN__, or some other
> Cygwin-specific symbol.

$ cd gdb/config
$ grep -nH -e win32-nat */*.m? 
i386/cygwin.mh:2:NATDEPFILES= i386-nat.o win32-nat.o corelow.o
$ 

I think that means it's just Cygwin.

Re: [RFA] win32-nat.c: Simplify generation of Windows environment

[Eli Zaretskii]

> Cc: gdb-patches@sourceware.org
> From: Jim Blandy <jimb@codesourcery.com>
> Date: Thu, 07 Dec 2006 16:10:14 -0800
> 
> $ cd gdb/config
> $ grep -nH -e win32-nat */*.m? 
> i386/cygwin.mh:2:NATDEPFILES= i386-nat.o win32-nat.o corelow.o
> $ 
> 
> I think that means it's just Cygwin.

No, it just means that the native W32 build is currently not part of
the CVS.

A large portion of win32-nat.c is relevant to any native Windows
debugger.  By contrast, cygwin_internal is obviously Cygwin-specific.
So I think it would be a good practice to mark such specific portions
of code explicitly.

Re: [RFA] win32-nat.c: Simplify generation of Windows environment

[Corinna Vinschen]

On Dec  8 09:31, Eli Zaretskii wrote:
> > Cc: gdb-patches@sourceware.org
> > From: Jim Blandy <jimb@codesourcery.com>
> > Date: Thu, 07 Dec 2006 16:10:14 -0800
> > 
> > $ cd gdb/config
> > $ grep -nH -e win32-nat */*.m? 
> > i386/cygwin.mh:2:NATDEPFILES= i386-nat.o win32-nat.o corelow.o
> > $ 
> > 
> > I think that means it's just Cygwin.
> 
> No, it just means that the native W32 build is currently not part of
> the CVS.

How exactly are we supposed to think of and support code which is not
part of the source tree when creating a patch?

> A large portion of win32-nat.c is relevant to any native Windows
> debugger.  By contrast, cygwin_internal is obviously Cygwin-specific.
> So I think it would be a good practice to mark such specific portions
> of code explicitly.

As I just wrote in my reply to Jim, the cygwin_internal API is used in
GDB for a long time, as a grep will show.  If win32-nat.c is used by
some not-in-the-source-tree code somewere, it will already have to deal
with cygwin_internal.  A patch for a native GDB will have created either
a matching #define or a substitute for this function anyway.  I don't
see how this new usage differs from the existing ones.

Talking of a native win32 GDB, it shouldn't be bothered at all by my
patch.  Creating a native Windows environment from Cygwin's environment
is not a concern natively.  Just setting it to NULL in the call to
CreateProcess is sufficient for a native GDB, since that will create a
copy of the parent's (GDB's) environment to the child, the same what the
longish code does, which I substituted with cygwin_internal(CW_SYN_WINENV).


Corinna

-- 
Corinna Vinschen
Cygwin Project Co-Leader
Red Hat

Re: [RFA] win32-nat.c: Simplify generation of Windows environment

[Eli Zaretskii]

> Date: Fri, 8 Dec 2006 10:12:41 +0100
> From: Corinna Vinschen <vinschen@redhat.com>
> 
> How exactly are we supposed to think of and support code which is not
> part of the source tree when creating a patch?

You aren't required to think about it.  My request was just that---a
request, not an attempt to say that you made some grave mistake.

> As I just wrote in my reply to Jim, the cygwin_internal API is used in
> GDB for a long time, as a grep will show.  If win32-nat.c is used by
> some not-in-the-source-tree code somewere, it will already have to deal
> with cygwin_internal.  A patch for a native GDB will have created either
> a matching #define or a substitute for this function anyway.  I don't
> see how this new usage differs from the existing ones.

I really fail to understand the fuss that my simple request
generated.  All I asked for is 2 lines:

  #ifdef __CYGWIN__
  ...
  #endif

Re: [RFA] win32-nat.c: Simplify generation of Windows environment

[Corinna Vinschen]

On Dec  8 13:30, Eli Zaretskii wrote:
> > From: Corinna Vinschen
> > As I just wrote in my reply to Jim, the cygwin_internal API is used in
> > GDB for a long time, as a grep will show.  If win32-nat.c is used by
> > some not-in-the-source-tree code somewere, it will already have to deal
> > with cygwin_internal.  A patch for a native GDB will have created either
> > a matching #define or a substitute for this function anyway.  I don't
> > see how this new usage differs from the existing ones.
> 
> I really fail to understand the fuss that my simple request
> generated.  All I asked for is 2 lines:
> 
>   #ifdef __CYGWIN__
>   ...
>   #endif

All I say is, why setting a precedent at this point in the code,
when other places in the code are using cygwin_internal (or, fwiw,
cygwin_conv_to_posix_path, cygwin_conv_to_full_posix_path,
cygwin_conv_to_win32_path) without #ifdef/#endif bracket anyway.

There's also a pretty clear comment at the start of the file:

  /* We assume we're being built with and will be used for cygwin.  */


Corinna

-- 
Corinna Vinschen
Cygwin Project Co-Leader
Red Hat

Re: [RFA] win32-nat.c: Simplify generation of Windows environment

[Jim Blandy]

Eli Zaretskii <eliz@gnu.org> writes:
>> Cc: gdb-patches@sourceware.org
>> From: Jim Blandy <jimb@codesourcery.com>
>> Date: Thu, 07 Dec 2006 16:10:14 -0800
>> 
>> $ cd gdb/config
>> $ grep -nH -e win32-nat */*.m? 
>> i386/cygwin.mh:2:NATDEPFILES= i386-nat.o win32-nat.o corelow.o
>> $ 
>> 
>> I think that means it's just Cygwin.
>
> No, it just means that the native W32 build is currently not part of
> the CVS.
>
> A large portion of win32-nat.c is relevant to any native Windows
> debugger.  By contrast, cygwin_internal is obviously Cygwin-specific.
> So I think it would be a good practice to mark such specific portions
> of code explicitly.

Given what I've read so far, I don't agree.  In general, for the
intents and purposes of the Project GNU debugger, win32-nat.c is
Cygwin-specific.  If someone is maintaining out-of-tree W32 patches,
then it is up to them to amend those patches to add the sort of
#ifdefs you suggest.  It's not reasonable to expect contributors to
the the public GDB sources to anticipate and accomodate the needs of
code they can't see.

But that's "in general".  If there's a further story here that can be
told where some minor effort on our part will make it easier for
someone to participate and contribute to GDB, then I think folks here
are pretty friendly and would be willing to listen.

Re: [RFA] win32-nat.c: Simplify generation of Windows environment

[Christopher Faylor]

Sorry for the delay in responding.  I'm on a business trip this week.

On Thu, Dec 07, 2006 at 11:37:02PM +0200, Eli Zaretskii wrote:
>> Date: Thu, 7 Dec 2006 10:58:39 +0100
>> From: Corinna Vinschen <vinschen@redhat.com>
>> 
>> the below patch simplifies the code which translates the Cygwin
>> environment into the native Windows environment.  So far this is
>> done in GDB manually.  However, there's a Cygwin specific function
>> call which does the same for the calling process.  Using this call
>> has three advantages.
>
>Is win32-nat.c used only for the Cygwin build?  I thought the native
>Windows build used it as well (perhaps with a few patches that are not
>yet part of the CVS), but maybe I was mistaken.
>
>If I am right, then please make this change, and especially the call
>to cygwin_internal, conditioned on __CYGWIN__, or some other
>Cygwin-specific symbol.

There is no need for an ifdef.

  % grep -C 2 cygwin_internal win32-nat.c
      {
	/* Try fall back to Cygwin pid */
	pid = cygwin_internal (CW_CYGWIN_PID_TO_WINPID, pid);

	if (pid > 0)
  --
    struct external_pinfo *pinfo;

    cygwin_internal (CW_LOCK_PINFO, 1000);
    for (cpid = 0;
	 (pinfo = (struct external_pinfo *)
	  cygwin_internal (CW_GETPINFO, cpid | CW_NEXTPID));
	 cpid = pinfo->pid)
      {
  --
	 }
      }
    cygwin_internal (CW_UNLOCK_PINFO);
    return path_ptr;
  }

Corinna, please go ahead and check in your patch.

cgf

Re: [RFA] win32-nat.c: Simplify generation of Windows environment

[Corinna Vinschen]

On Dec  8 15:43, Christopher Faylor wrote:
> Corinna, please go ahead and check in your patch.

Thanks, applied.


Corinna

-- 
Corinna Vinschen
Cygwin Project Co-Leader
Red Hat