RE: [RFA] gdbserver/win32-low.c: Check Read/WriteProcessMemory return value (followup to [RFA] windows-nat.c: Handle ERROR_PARTIAL_COPY in windows_xfer_memory function)

["Pierre Muller" <pierre.muller@ics-cnrs.unistra.fr>]

> -----Message d'origine-----
> De : gdb-patches-owner@sourceware.org [mailto:gdb-patches-
> owner@sourceware.org] De la part de Pedro Alves
> Envoyé : lundi 2 septembre 2013 16:09
> À : Pierre Muller
> Cc : gdb-patches@sourceware.org
> Objet : Re: [RFA] gdbserver/win32-low.c: Check Read/WriteProcessMemory
> return value (followup to [RFA] windows-nat.c: Handle ERROR_PARTIAL_COPY
in
> windows_xfer_memory function)
> 
> On 09/02/2013 03:00 PM, Pierre Muller wrote:
> >>>   What about this patch,
> >>> it still does not allow to really return the number of bytes read or
> >>> written,
> >>> but at least it checks correctly if the API calls succeeded.
> >>
> >> No, as long as the read_memory/write_memory interfaces do not
> >> support partial transfers, we should only return true if the
> >> all of LEN was transferred.  Otherwise, things like:
> >>
> >> static int
> >> gdb_read_memory (CORE_ADDR memaddr, unsigned char *myaddr, int len)
> >> {
> >> ...
> >>     {
> >>       res = read_inferior_memory (memaddr, myaddr, len);
> >>       done_accessing_memory ();
> >>
> >>       return res == 0 ? len : -1;
> >>     }
> >> }
> >>
> >> will behave incorrectly in the ERROR_PARTIAL_COPY scenario...
> >
> >   This is still done in win32_{read/write}_inferior_memory which are the
> two
> > only callers of the static child_xfer_memory function in win32-low.c
> 
> >   Thus the aim was to narrow the behavior gap between
> > windows-nat.c windows_xfer_memory function
> > and the win32-low.c child_xfer_memory function,
> > without (for now) changing anything to the beghavior of gdbserver,
> > as guaranteed by the
> > static int
> > win32_write_inferior_memory (CORE_ADDR memaddr, const unsigned char
> *myaddr,
> >                              int len)
> > {
> >   return child_xfer_memory (memaddr, (char *) myaddr, len, 1, 0) != len;
> > }
> >
> > code...
> >
> > The only thing I changed is that child_xfer_memory returns the correct
> > amount of read/written memory or -1 if an error, other than
> > ERRO_PARTIAL_COPY, occurred.
> > Thus I think that your answer is missing the intermediate
> > win32_{read/write}_inferior_memory level.
> >
> 
> Ah, indeed.
> 
> 
> Why the different styles in gdb's and gdbserver patches, though?
> 
> gdb:
> 
> > +  if (!success && lasterror == ERROR_PARTIAL_COPY && done > 0)
> > +    return done;
> > +  else
> > +    return success ? done : TARGET_XFER_E_IO;
> 
> gdbserver:
> 
> > +  if (success)
> > +    return done;
> > +  else
> > +    {
> > +      if (lasterror == ERROR_PARTIAL_COPY && done > 0)
> > +	return done;
> > +      else
> > +	return -1;
> >      }
> 
> We should be able to compare the functions and see at
> a glance they are almost duplicates.  With the different
> styles, it's not immediately obvious.  Can you make the
> gdbserver code look like gdb's?

  The problem is that TARGET_XFER_E_IO 
is only defined in gdb/target.h...

  Should I just replace TARGET_XFER_E_IO by -1 and keep the gdb version
otherwise?

Pierre