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

[Eli Zaretskii <eliz@gnu.org>]

> Date: Fri, 16 Mar 2007 15:03:21 +0000
> From: "pedro alves" <alves.ped@gmail.com>
> Cc: gdb-patches@sourceware.org
> 
> > 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.

In that case, I guess it's okay to use a static buffer.  But please at
least check inside the function that the buffer size was enough, by
comparing the value returned by FormatMessageW with the size of the
buffer you passed to it.

> 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.

I think it fills the buffer because the buffer is larger than 0.  The
return value tells you how many characters it _really_ needed to
format the message.  By comparing that value with the size of the
buffer, you can find out whether the buffer was large enough.

> > > +#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.

But that ifdef would be in only one place, while with a different
function you need an ifdef each time you use the function.

> Since there is only one instance of it (the #ifdef) in gdbserver, I
> thought it is better to have strwinerror explicit.

There's only one instance _today_.  Tomorrow we could have more of
them.

Btw, I see in your patch two instances of strwinerror and two places
that call it: one in gdbreplay.c, the other in utils.c.  Why did you
need two almost identical functions?

> When someone later uses errno as an lvalue, it breaks WinCE again.

I think such usage of errno is a bad idea anyway, since on many
platforms errno is a function already (to support multi-threading).

> 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.

This is not a problem: errno should not be used with any literal
values anyway, only with symbolical constants.

> 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

Yes, I'd prefer that solution.  gdbserver/win32-low.c is already
Windows specific, so it's okay to use FormatMessage there.

> > > 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.

Are there any WinCE-specific commands or features left after your
patch?  If there are, please describe them in this section.  If not,
then it's okay to delete the section.

Thanks.