From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 30722 invoked by alias); 8 Nov 2010 19:26:32 -0000 Received: (qmail 30714 invoked by uid 22791); 8 Nov 2010 19:26:31 -0000 X-SWARE-Spam-Status: No, hits=-2.1 required=5.0 tests=AWL,BAYES_00 X-Spam-Check-By: sourceware.org Received: from rock.gnat.com (HELO rock.gnat.com) (205.232.38.15) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Mon, 08 Nov 2010 19:26:27 +0000 Received: from localhost (localhost.localdomain [127.0.0.1]) by filtered-rock.gnat.com (Postfix) with ESMTP id CAD3B2BAC82; Mon, 8 Nov 2010 14:26:25 -0500 (EST) Received: from rock.gnat.com ([127.0.0.1]) by localhost (rock.gnat.com [127.0.0.1]) (amavisd-new, port 10024) with LMTP id 3sz9w1Aelhrc; Mon, 8 Nov 2010 14:26:25 -0500 (EST) Received: from joel.gnat.com (localhost.localdomain [127.0.0.1]) by rock.gnat.com (Postfix) with ESMTP id 8E75F2BAC69; Mon, 8 Nov 2010 14:26:25 -0500 (EST) Received: by joel.gnat.com (Postfix, from userid 1000) id BCB50145907; Mon, 8 Nov 2010 11:26:19 -0800 (PST) Date: Mon, 08 Nov 2010 19:26:00 -0000 From: Joel Brobecker To: Sebasti?n Puebla Castro Cc: gdb-patches@sourceware.org Subject: Re: [PATCH/Windows] Pass correct environment to a Windows executable started as an inferior Message-ID: <20101108192619.GH2933@adacore.com> References: MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: User-Agent: Mutt/1.5.20 (2009-06-14) Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org X-SW-Source: 2010-11/txt/msg00128.txt.bz2 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 > >            * 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