Re: [New WinCE support] [patch 4/4] The bulk of the code.

["pedro alves" <alves dot ped at gmail dot com>]

Hi Eli,
thanks for taking a look.

Eli Zaretskii wrote:
> > Date: Fri, 16 Mar 2007 02:08:51 +0000
> > From: Pedro Alves <pedro_alves@portugalmail.pt>
> >
> > 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