This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH/Windows] Pass correct environment to a Windows executable started as an inferior
- From: Christopher Faylor <cgf-use-the-mailinglist-please at sourceware dot org>
- To: Sebasti?n Puebla Castro <spuebla at hotmail dot com>, gdb-patches at sourceware dot org, Joel Brobecker <brobecker at adacore dot com>
- Date: Thu, 11 Nov 2010 01:16:59 -0500
- Subject: Re: [PATCH/Windows] Pass correct environment to a Windows executable started as an inferior
- References: <SNT110-W553622E76149C966A25851B3900@phx.gbl> <20101108192619.GH2933@adacore.com>
On Mon, Nov 08, 2010 at 11:26:19AM -0800, Joel Brobecker wrote:
>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"
That really isn't necessary. It already had Windows in the subject. I
just missed this the first time around.
>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...
Ditto.
>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.
Ditto.
>> + #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?
Ditto on the separate function.
> - 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").
Yep.
cgf