Re: [RFA] windows-nat.c: Handle ERROR_PARTIAL_COPY in windows_xfer_memory function

Re: [RFA] windows-nat.c: Handle ERROR_PARTIAL_COPY in windows_xfer_memory function

[Pedro Alves]

On 09/01/2013 11:10 PM, Pierre Muller wrote:
> 
> PS: the use of plongest function is because I got an warning about %d
> used for 'long long integer' type.

Then that's a regression from my deprecated_xfer_memory change.
Please keep logically independent changes as separate patches.

> 2013-09-01  Pierre Muller  <muller@sourceware.org>
> 
> 	* windows-nat.c (windows_xfer_memory): Fix compilation failure
> 	by use of plongest function.

This part is OK.  Please split it out, and commit it.

-- 
Pedro Alves

RE: [RFA] windows-nat.c: Handle ERROR_PARTIAL_COPY in windows_xfer_memory function

[Pierre Muller]

  

> -----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 14:34
> À : Pierre Muller
> Cc : gdb-patches@sourceware.org
> Objet : Re: [RFA] windows-nat.c: Handle ERROR_PARTIAL_COPY in
> windows_xfer_memory function
> 
> On 09/01/2013 11:10 PM, Pierre Muller wrote:
> >
> > PS: the use of plongest function is because I got an warning about %d
> > used for 'long long integer' type.
> 
> Then that's a regression from my deprecated_xfer_memory change.
> Please keep logically independent changes as separate patches.

  I just noticed the problem while
trying to update the patch...
 
  You are off course right that this should be separate...

> > 2013-09-01  Pierre Muller  <muller@sourceware.org>
> >
> > 	* windows-nat.c (windows_xfer_memory): Fix compilation failure
> > 	by use of plongest function.
> 
> This part is OK.  Please split it out, and commit it.

Done and committed,

thanks,

Pierre

PS: Do you want me to resubmit and new version of the ERROR_PARTIAL_COPY
RFA?

Re: [RFA] windows-nat.c: Handle ERROR_PARTIAL_COPY in windows_xfer_memory function

[Pedro Alves]

On 09/01/2013 11:10 PM, Pierre Muller wrote:
> This is the patch that Pedro suggested I send
> after his commit to remove deprecated_xfer_memory
> in windows-nat.c.

Thanks.

> 
>   Pedro suggested that I submit this patch separately
> (which I do here)... and with a gdbserver counterpart,
> which I don't...
> 
>   I tried, but finally realized that given the
> read_memory / write_memory functions type defined
> in target.h target_ops structure,
> there is no way of passing information of partial 
> copy and of the length of this partial copy.
> Indeed, the comments state that the return value is either 0 for success
> or errno...
> 
> This is not compatible with returning information that only part of the
> request length
> was read/written.

Well, we could just change that interface to make it possible...

The thing I don't like with doing this only on the native
side, is that we're trying to get to a point where we
can share the target backends between GDB and gdbserver:

<https://sourceware.org/gdb/wiki/LocalRemoteFeatureParity>.

Doing such a change on the GDB side only just means we're
pushing the feature-parity goal for the Windows port
further away...

> 2013-09-01  Pierre Muller  <muller@sourceware.org>
> 
...
> 	Handle ERROR_PARTIAL_COPY error code.
...

This part is OK too.  (Please commit it separately from the
plongest fix.)

-- 
Pedro Alves

RE: [RFA] windows-nat.c: Handle ERROR_PARTIAL_COPY in windows_xfer_memory function

[Pierre Muller]

> -----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 14:50
> À : Pierre Muller
> Cc : gdb-patches@sourceware.org
> Objet : Re: [RFA] windows-nat.c: Handle ERROR_PARTIAL_COPY in
> windows_xfer_memory function
> 
> On 09/01/2013 11:10 PM, Pierre Muller wrote:
> > This is the patch that Pedro suggested I send
> > after his commit to remove deprecated_xfer_memory
> > in windows-nat.c.
> 
> Thanks.
> 
> >
> >   Pedro suggested that I submit this patch separately
> > (which I do here)... and with a gdbserver counterpart,
> > which I don't...
> >
> >   I tried, but finally realized that given the
> > read_memory / write_memory functions type defined
> > in target.h target_ops structure,
> > there is no way of passing information of partial
> > copy and of the length of this partial copy.
> > Indeed, the comments state that the return value is either 0 for success
> > or errno...
> >
> > This is not compatible with returning information that only part of the
> > request length
> > was read/written.
> 
> Well, we could just change that interface to make it possible...
> 
> The thing I don't like with doing this only on the native
> side, is that we're trying to get to a point where we
> can share the target backends between GDB and gdbserver:

  Well, when you look at the code inside child_xfer_memory,
you can notice that the return value of ReadProcessMemory or
WriteProcessMemory
is discarded, which means that it does behave more or less like the
new windows-nat.c code (at least in case of ERROR_PARTIAL_COPY)
for other errors, it might also return garbage...
anyhow, the calling code compares the returned value to the requested length
(LEN value)
so that the risk of generating a successful read_memory despite a failure
of ReadProcessMemory function is small... (the uninitialized variable done
would need to return the value LEN..)
It could of course still happen theoretically...

 
> <https://sourceware.org/gdb/wiki/LocalRemoteFeatureParity>.
> 
> Doing such a change on the GDB side only just means we're
> pushing the feature-parity goal for the Windows port
> further away...
> 
> > 2013-09-01  Pierre Muller  <muller@sourceware.org>
> >
> ...
> > 	Handle ERROR_PARTIAL_COPY error code.
> ...
> 
> This part is OK too.  (Please commit it separately from the
> plongest fix.)

Thanks for the approval,
patch committed,

Pierre Muller

Re: [RFA] windows-nat.c: Handle ERROR_PARTIAL_COPY in windows_xfer_memory function

[Pedro Alves]

On 09/02/2013 02:05 PM, Pierre Muller wrote:

>>> This is not compatible with returning information that only part of the
>>> request length
>>> was read/written.
>>
>> Well, we could just change that interface to make it possible...
>>
>> The thing I don't like with doing this only on the native
>> side, is that we're trying to get to a point where we
>> can share the target backends between GDB and gdbserver:
> 
>   Well, when you look at the code inside child_xfer_memory,
> you can notice that the return value of ReadProcessMemory or
> WriteProcessMemory
> is discarded, which means that it does behave more or less like the
> new windows-nat.c code (at least in case of ERROR_PARTIAL_COPY)
> for other errors, it might also return garbage...
> anyhow, the calling code compares the returned value to the requested length
> (LEN value)

That's brittle...

> so that the risk of generating a successful read_memory despite a failure
> of ReadProcessMemory function is small... (the uninitialized variable done
> would need to return the value LEN..)
> It could of course still happen theoretically...

This is really no argument for not fixing gdbserver...  In fact,
it's an argument _for_ fixing it.

-- 
Pedro Alves

[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]

> >>> This is not compatible with returning information that only part of
the
> >>> request length
> >>> was read/written.
> >>
> >> Well, we could just change that interface to make it possible...
> >>
> >> The thing I don't like with doing this only on the native
> >> side, is that we're trying to get to a point where we
> >> can share the target backends between GDB and gdbserver:
> >
> >   Well, when you look at the code inside child_xfer_memory,
> > you can notice that the return value of ReadProcessMemory or
> > WriteProcessMemory
> > is discarded, which means that it does behave more or less like the
> > new windows-nat.c code (at least in case of ERROR_PARTIAL_COPY)
> > for other errors, it might also return garbage...
> > anyhow, the calling code compares the returned value to the requested
> length
> > (LEN value)
> 
> That's brittle...
> 
> > so that the risk of generating a successful read_memory despite a
failure
> > of ReadProcessMemory function is small... (the uninitialized variable
done
> > would need to return the value LEN..)
> > It could of course still happen theoretically...
> 
> This is really no argument for not fixing gdbserver...  In fact,
> it's an argument _for_ fixing it.

  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.

Pierre Muller




2013-09-02  Pierre Muller  <muller@sourceware.org>

	* win32-low.c (child_xfer_memory): Check if ReadProcessMemory
	or WriteProcessMemory complete successfully and handle
	ERROR_PARTIAL_COPY error.

Index: src/gdb/gdbserver/win32-low.c
===================================================================
RCS file: /cvs/src/src/gdb/gdbserver/win32-low.c,v
retrieving revision 1.66
diff -u -r1.66 win32-low.c
--- src/gdb/gdbserver/win32-low.c	2 Jul 2013 11:59:24 -0000	1.66
+++ src/gdb/gdbserver/win32-low.c	2 Sep 2013 13:31:31 -0000
@@ -278,21 +278,35 @@
 child_xfer_memory (CORE_ADDR memaddr, char *our, int len,
 		   int write, struct target_ops *target)
 {
-  SIZE_T done;
+  BOOL success;
+  SIZE_T done = 0;
+  DWORD lasterror = 0;
   uintptr_t addr = (uintptr_t) memaddr;
 
   if (write)
     {
-      WriteProcessMemory (current_process_handle, (LPVOID) addr,
-			  (LPCVOID) our, len, &done);
+      success = WriteProcessMemory (current_process_handle, (LPVOID) addr,
+				    (LPCVOID) our, len, &done);
+      if (!success)
+	lasterror = GetLastError ();
       FlushInstructionCache (current_process_handle, (LPCVOID) addr, len);
     }
   else
     {
-      ReadProcessMemory (current_process_handle, (LPCVOID) addr, (LPVOID)
our,
-			 len, &done);
+      success = ReadProcessMemory (current_process_handle, (LPCVOID) addr,
+				   (LPVOID) our, len, &done);
+      if (!success)
+	lasterror = GetLastError ();
+    }
+  if (success)
+    return done;
+  else
+    {
+      if (lasterror == ERROR_PARTIAL_COPY && done > 0)
+	return done;
+      else
+	return -1;
     }
-  return done;
 }
 
 /* Clear out any old thread list and reinitialize it to a pristine

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)

[Pedro Alves]

On 09/02/2013 02:38 PM, Pierre Muller wrote:
>>>>> This is not compatible with returning information that only part of
> the
>>>>> request length
>>>>> was read/written.
>>>>
>>>> Well, we could just change that interface to make it possible...
>>>>
>>>> The thing I don't like with doing this only on the native
>>>> side, is that we're trying to get to a point where we
>>>> can share the target backends between GDB and gdbserver:
>>>
>>>   Well, when you look at the code inside child_xfer_memory,
>>> you can notice that the return value of ReadProcessMemory or
>>> WriteProcessMemory
>>> is discarded, which means that it does behave more or less like the
>>> new windows-nat.c code (at least in case of ERROR_PARTIAL_COPY)
>>> for other errors, it might also return garbage...
>>> anyhow, the calling code compares the returned value to the requested
>> length
>>> (LEN value)
>>
>> That's brittle...
>>
>>> so that the risk of generating a successful read_memory despite a
> failure
>>> of ReadProcessMemory function is small... (the uninitialized variable
> done
>>> would need to return the value LEN..)
>>> It could of course still happen theoretically...
>>
>> This is really no argument for not fixing gdbserver...  In fact,
>> it's an argument _for_ fixing it.
> 
>   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...

-- 
Pedro Alves

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]

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


Pierre Muller

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)

[Pedro Alves]

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?

Thanks,
-- 
Pedro Alves

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]

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

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)

[Pedro Alves]

On 09/02/2013 03:18 PM, Pierre Muller wrote:

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

Yep.

Thanks,
-- 
Pedro Alves

[RFA-v2] 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]

> -----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:20
> À : Pierre Muller
> Cc : 'Pedro Alves'; 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:18 PM, Pierre Muller wrote:
> 
> >   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?
> 
> Yep.

Here is an update patch,
is this OK?


2013-09-02  Pierre Muller  <muller@sourceware.org>

	* win32-low.c (child_xfer_memory): Check if ReadProcessMemory
	or WriteProcessMemory complete successfully and handle
	ERROR_PARTIAL_COPY error.

Index: gdbserver/win32-low.c
===================================================================
RCS file: /cvs/src/src/gdb/gdbserver/win32-low.c,v
retrieving revision 1.66
diff -u -p -r1.66 win32-low.c
--- gdbserver/win32-low.c	2 Jul 2013 11:59:24 -0000	1.66
+++ gdbserver/win32-low.c	2 Sep 2013 14:23:35 -0000
@@ -278,21 +278,30 @@ static int
 child_xfer_memory (CORE_ADDR memaddr, char *our, int len,
 		   int write, struct target_ops *target)
 {
-  SIZE_T done;
+  BOOL success;
+  SIZE_T done = 0;
+  DWORD lasterror = 0;
   uintptr_t addr = (uintptr_t) memaddr;
 
   if (write)
     {
-      WriteProcessMemory (current_process_handle, (LPVOID) addr,
-			  (LPCVOID) our, len, &done);
+      success = WriteProcessMemory (current_process_handle, (LPVOID) addr,
+				    (LPCVOID) our, len, &done);
+      if (!success)
+	lasterror = GetLastError ();
       FlushInstructionCache (current_process_handle, (LPCVOID) addr, len);
     }
   else
     {
-      ReadProcessMemory (current_process_handle, (LPCVOID) addr, (LPVOID)
our,
-			 len, &done);
+      success = ReadProcessMemory (current_process_handle, (LPCVOID) addr,
+				   (LPVOID) our, len, &done);
+      if (!success)
+	lasterror = GetLastError ();
     }
-  return done;
+  if (!success && lasterror == ERROR_PARTIAL_COPY && done > 0)
+    return done;
+  else
+    return success ? done : -1;
 }
 
 /* Clear out any old thread list and reinitialize it to a pristine

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

[Pedro Alves]

On 09/02/2013 03:25 PM, Pierre Muller wrote:

> Here is an update patch,
> is this OK?

Great!  OK, thanks.

-- 
Pedro Alves

RE: [RFA-v2] 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]

> -----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:30
> À : Pierre Muller
> Cc : gdb-patches@sourceware.org
> Objet : Re: [RFA-v2] 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:25 PM, Pierre Muller wrote:
> 
> > Here is an update patch,
> > is this OK?
> 
> Great!  OK, thanks.
Thanks for the fast feedback,
patch committed,

Pierre Muller

[RFA] windows-nat.c: Handle ERROR_PARTIAL_COPY in windows_xfer_memory function

[Pierre Muller]

This is the patch that Pedro suggested I send
after his commit to remove deprecated_xfer_memory
in windows-nat.c.

  Pedro suggested that I submit this patch separately
(which I do here)... and with a gdbserver counterpart,
which I don't...

  I tried, but finally realized that given the
read_memory / write_memory functions type defined
in target.h target_ops structure,
there is no way of passing information of partial 
copy and of the length of this partial copy.
Indeed, the comments state that the return value is either 0 for success
or errno...

This is not compatible with returning information that only part of the
request length
was read/written.
  Changing this semantics is too much work with high risks of breaking
things elsewhere...

  

Pierre Muller
GDB pascal language maintainer

PS: the use of plongest function is because I got an warning about %d
used for 'long long integer' type.


2013-09-01  Pierre Muller  <muller@sourceware.org>

	* windows-nat.c (windows_xfer_memory): Fix compilation failure
	by use of plongest function.
	Handle ERROR_PARTIAL_COPY error code.


Index: src/gdb/windows-nat.c
===================================================================
RCS file: /cvs/src/src/gdb/windows-nat.c,v
retrieving revision 1.258
diff -u -p -r1.258 windows-nat.c
--- src/gdb/windows-nat.c	27 Aug 2013 11:36:09 -0000	1.258
+++ src/gdb/windows-nat.c	1 Sep 2013 21:20:51 -0000
@@ -2324,26 +2324,34 @@ windows_xfer_memory (gdb_byte *readbuf, 
 {
   SIZE_T done = 0;
   BOOL success;
+  DWORD lasterror = 0;
 
   if (writebuf != NULL)
     {
-      DEBUG_MEM (("gdb: write target memory, %d bytes at %s\n",
-		  len, core_addr_to_string (memaddr)));
+      DEBUG_MEM (("gdb: write target memory, %s bytes at %s\n",
+		  plongest (len), core_addr_to_string (memaddr)));
       success = WriteProcessMemory (current_process_handle,
 				    (LPVOID) (uintptr_t) memaddr, writebuf,
 				    len, &done);
+      if (!success)
+       lasterror = GetLastError ();
       FlushInstructionCache (current_process_handle,
 			     (LPCVOID) (uintptr_t) memaddr, len);
     }
   else
     {
-      DEBUG_MEM (("gdb: read target memory, %d bytes at %s\n",
-		  len, core_addr_to_string (memaddr)));
+      DEBUG_MEM (("gdb: read target memory, %s bytes at %s\n",
+		  plongest (len), core_addr_to_string (memaddr)));
       success = ReadProcessMemory (current_process_handle,
 				   (LPCVOID) (uintptr_t) memaddr, readbuf,
 				   len, &done);
+      if (!success)
+       lasterror = GetLastError ();
     }
-  return success ? done : TARGET_XFER_E_IO;
+  if (!success && lasterror == ERROR_PARTIAL_COPY && done > 0)
+    return done;
+  else
+    return success ? done : TARGET_XFER_E_IO;
 }
 
 static void