This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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


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


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]