Re: [PATCH/Windows] Pass correct environment to a Windows executable started as an inferior

[Joel Brobecker <brobecker@adacore.com>]

Sebastian,

> This patch fixes a bug found in ports of GDB 7.0/7.1 for Windows: an
> ejecutable started as an inferior doesn't receive its own environment,
> possibly modified, as expected; instead, it inherits the environment
> from current GDB instance.

Thanks for the contribution, and my sincere apologies for the delay
in getting back to you.  It seems that all global maintainers became
super busy all at the same time.

This change is specific to Windows, and we do have a Windows maintainer.
To have a better chance of attracting his attention, I suggest the use
of "Windows" in the subject. Eg:

    "[RFA/Windows] set environment not propagated to inferior"

This is only a preliminary review, since changes to windows-nat
are normally reviewed and approved by Chris, the Windows maintainer.
However, I noticed a few things that are worth commenting on now:

> 2010-05-10 Sebastián Puebla <spuebla@hotmail.com>
> 
>            * windows-nat.c (windows_create_inferior): Create environment
>              block for new inferior.

First of all, do you have a copyright assignement on file.  In my
opinion, this change is a little too large to be accepted under the
"small change" rule.

Good job on providing a ChangeLog entry :).

> RCS file: /cvs/src/src/gdb/windows-nat.c,v
> retrieving revision 1.208
> diff -c -p -r1.208 windows-nat.c

Most maintainers here prefer unified diff (diff -u instead of diff -c).
I have a strong preference for unified...

Please also consider sending the patch as an attachment rather than
inline your email text, because it appears that your mail swaps spaces
for another weird character, and that makes it impossible to apply your
patch as is.

> + #ifdef __USEWIDE
> +   for(i = 0; !in_env[i]; i++)
> +   {
> +     env_size += mbstowcs(NULL, in_env[i], 0) + 1;
> +   }
> +   
> +   env_block = (cygwin_buf_t *) alloca(env_size * sizeof(wchar_t));
> + 
> +   for(i = j = 0; !in_env[i]; i++)
> +   {
> +     j += mbstowcs(&env_block[j], in_env[i], env_size) + 1;
> +   }
> + #else
> +   for(i = 0; !in_env[i]; i++)
> +   {
> +     env_size += strlen(in_env[i]) + 1;
> +   }
> + 
> +   env_block = (cygwin_buf_t *) alloca(env_size);
> + 
> +   for(i = j = 0; !in_env[i]; i++)
> +   {
> +     len = strlen(in_env[i]) + 1;
> +     memcpy(&env_block[j], in_env[i], len);
> +     j += len;
> +   }
> + #endif
> +   env_block[j] = 0;
> + 

Several comments:

  - It seems worth putting that code in a separate function.  But why
    can't we use the in_env array? Is it because of the mbstowcs
    conversion in the __USEWIDE case?

  - Curly braces should be omitted if the block is going to have
    one statement only. Eg:

      for (i = 0, !in_env[i]; i++)
        env_size += mbstowcs(NULL, in_env[i], 0) + 1;

  - Another style gotcha: You need a space before '(' in function calls.  Eg:

      alloca (env_size)

    (the same should apply to sizeof)

  - And last but not least: I don't think your code is going to compile
    on MinGW (use of "cygwin_buf_t").

I don't think that there is anything cygwin-related to this part, is
there?
      
    
-- 
Joel