From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 30973 invoked by alias); 16 Mar 2007 15:03:51 -0000 Received: (qmail 30941 invoked by uid 22791); 16 Mar 2007 15:03:43 -0000 X-Spam-Check-By: sourceware.org Received: from nz-out-0506.google.com (HELO nz-out-0506.google.com) (64.233.162.225) by sourceware.org (qpsmtpd/0.31) with ESMTP; Fri, 16 Mar 2007 15:03:27 +0000 Received: by nz-out-0506.google.com with SMTP id m7so220589nzf for ; Fri, 16 Mar 2007 08:03:21 -0700 (PDT) Received: by 10.35.93.19 with SMTP id v19mr4337123pyl.1174057401282; Fri, 16 Mar 2007 08:03:21 -0700 (PDT) Received: by 10.35.34.17 with HTTP; Fri, 16 Mar 2007 08:03:21 -0700 (PDT) Message-ID: <4053daab0703160803s3edf1294sfcdf4f8319787ab7@mail.gmail.com> Date: Fri, 16 Mar 2007 15:03:00 -0000 From: "pedro alves" To: "Eli Zaretskii" Subject: Re: [New WinCE support] [patch 4/4] The bulk of the code. Cc: gdb-patches@sourceware.org In-Reply-To: MIME-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Content-Disposition: inline References: <20070315235008.243411000@portugalmail.pt> <45F9FC33.9020106@portugalmail.pt> X-IsSubscribed: yes 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: 2007-03/txt/msg00153.txt.bz2 Hi Eli, thanks for taking a look. Eli Zaretskii wrote: > > Date: Fri, 16 Mar 2007 02:08:51 +0000 > > From: Pedro Alves > > > > This patch is the bulk of the new WinCE support. > > Thanks. > > I have a few comments. > > > +static char * > > +strwinerror (DWORD error) > > +{ > > + static char buf[1024]; > > + wchar_t msgbuf[1024]; > > + DWORD chars = FormatMessageW ( > > + FORMAT_MESSAGE_FROM_SYSTEM, > > + NULL, > > + error, > > + 0, /* Default language */ > > + (LPVOID)&msgbuf, > > + 0, > > + NULL); > > Instead of using an arbitrary size 1024 (btw, you don't check whether > FormatMessageW indicated that it needed more than 1024), isn't it > better to use FORMAT_MESSAGE_ALLOCATE_BUFFER? As a bonus, it would > avoid overwriting the static buffer on each call, which is not a nice > API, IMHO. > I'm using the same api as strerror, which returns a pointer into a static buffer. If I return an allocated buffer, either the client must release it (but that would change the contract), or the previous allocated one must be released on every strwinerror invocation. Hummm, I don't know how is it that I specified a buffer of size 0, and Windows (at least CE) still fills the buffer. Do you feel strongly about using FORMAT_MESSAGE_ALLOCATE_BUFFER, or would just passing the buffer limit (nSize) to FormatMessage be OK? > > +#ifdef __MINGW32CE__ > > + err = strwinerror (GetLastError ()); > > +#else > > err = strerror (errno); > > +#endif > > Why not call strwinerror strerror and avoid the ifdef? > Because then I would have to: #ifdef __MINGW32CE__ #define errno (GetLastError ()) #endif err = strerror (errno); That means I still must have an #ifdef somewhere. Since there is only one instance of it (the #ifdef) in gdbserver, I thought it is better to have strwinerror explicit. I try to avoid doing that #define errno (...) whenever possible. When someone later uses errno as an lvalue, it breaks WinCE again. Since the errno <-> GetLastError mapping is not 100% correct, it always feels dirty. As you can imagine, this is a recurring problem while doing WinCE ports - see here for some options, but none is perfect: http://sourceforge.net/mailarchive/forum.php?thread_id=31663083&forum_id=49151 The other place I used strwinerror is in: if (!ret) { - error ("Error creating process %s, (error %d): %s\n", args, - (int) GetLastError (), strerror (GetLastError ())); + error ("Error creating process \"%s%s\", (error %d): %s\n", + program, args, + (int) GetLastError (), strwinerror (GetLastError ())); } On Window 9x/NT it is wrong to do: strerror (GetLastError ()) The errno values and the windows error codes are not the same. I was going to post a patch to fix this using FormatMessage directly, but since I needed a strerror for WinCE, I ended up using strwinerror here too. Would you prefer to have that? That is, rewrite this last hunk to use FormatMessage directly, and rename strwinerror to strerror, put it wincecompat.c, and have a: #ifdef __MINGW32CE__ #define errno (GetLastError ()) #endif > > doc/ChangeLog > > > > * gdb.texinfo (WinCE): Delete subsection. > > Why? Is that subsection incorrect in some ways? > It is correct for the current WinCE support, using gdb/wince.c and gdb/wince-stub.c, which implements a custom remote protocol. This patch removes those files, removing the functionality that subsection is documenting. Cheers, Pedro Alves