[PATCH] windows-nat.c: Don't install a deprecated_xfer_memory method.

[PATCH] windows-nat.c: Don't install a deprecated_xfer_memory method.

[Pedro Alves]

This stops another target from installing a
target_ops->deprecated_xfer_memory method.

I'm not setup for proper Cygwin/Windows testing, but I managed to
cross build gdb from GNU/Linux, and copy the resulting binary to a
Windows box.  Running that gdb on itself worked well enough to run to
main, so I can't imagine any regression from this.  Does anyone want
(and is willing) to run this through the testsuite?

gdb/
2013-08-23  Pedro Alves  <palves@redhat.com>

	* windows-nat.c (windows_xfer_memory): Adjust prototype to follow
	xfer_partial's interface.  Return TARGET_XFER_E_IO on error.
	(windows_xfer_partial): Defer TARGET_OBJECT_MEMORY handling to
	windows_xfer_memory directly.
	(init_windows_ops): Don't install a deprecated_xfer_memory method.
---
 gdb/windows-nat.c |   36 ++++++++++++++----------------------
 1 file changed, 14 insertions(+), 22 deletions(-)

diff --git a/gdb/windows-nat.c b/gdb/windows-nat.c
index 9a0241b..a4e7107 100644
--- a/gdb/windows-nat.c
+++ b/gdb/windows-nat.c
@@ -2315,20 +2315,20 @@ windows_stop (ptid_t ptid)
   registers_changed ();		/* refresh register state */
 }
 
-static int
-windows_xfer_memory (CORE_ADDR memaddr, gdb_byte *our, int len,
-		   int write, struct mem_attrib *mem,
-		   struct target_ops *target)
+static LONGEST
+windows_xfer_memory (gdb_byte *readbuf, const gdb_byte *writebuf,
+		     ULONGEST memaddr, LONGEST len)
 {
   SIZE_T done = 0;
-  if (write)
+  BOOL success;
+
+  if (writebuf != NULL)
     {
       DEBUG_MEM (("gdb: write target memory, %d bytes at %s\n",
 		  len, core_addr_to_string (memaddr)));
-      if (!WriteProcessMemory (current_process_handle,
-			       (LPVOID) (uintptr_t) memaddr, our,
-			       len, &done))
-	done = 0;
+      success = WriteProcessMemory (current_process_handle,
+				    (LPVOID) (uintptr_t) memaddr, writebuf,
+				    len, &done);
       FlushInstructionCache (current_process_handle,
 			     (LPCVOID) (uintptr_t) memaddr, len);
     }
@@ -2336,12 +2336,11 @@ windows_xfer_memory (CORE_ADDR memaddr, gdb_byte *our, int len,
     {
       DEBUG_MEM (("gdb: read target memory, %d bytes at %s\n",
 		  len, core_addr_to_string (memaddr)));
-      if (!ReadProcessMemory (current_process_handle,
-			      (LPCVOID) (uintptr_t) memaddr, our,
-			      len, &done))
-	done = 0;
+      success = ReadProcessMemory (current_process_handle,
+				   (LPCVOID) (uintptr_t) memaddr, readbuf,
+				   len, &done);
     }
-  return done;
+  return success ? done : TARGET_XFER_E_IO;
 }
 
 static void
@@ -2442,13 +2441,7 @@ windows_xfer_partial (struct target_ops *ops, enum target_object object,
   switch (object)
     {
     case TARGET_OBJECT_MEMORY:
-      if (readbuf)
-	return (*ops->deprecated_xfer_memory) (offset, readbuf,
-					       len, 0/*read*/, NULL, ops);
-      if (writebuf)
-	return (*ops->deprecated_xfer_memory) (offset, (gdb_byte *) writebuf,
-					       len, 1/*write*/, NULL, ops);
-      return -1;
+      return windows_xfer_memory (readbuf, writebuf, offset, len);
 
     case TARGET_OBJECT_LIBRARIES:
       return windows_xfer_shared_libraries (ops, object, annex, readbuf,
@@ -2502,7 +2495,6 @@ init_windows_ops (void)
   windows_ops.to_fetch_registers = windows_fetch_inferior_registers;
   windows_ops.to_store_registers = windows_store_inferior_registers;
   windows_ops.to_prepare_to_store = windows_prepare_to_store;
-  windows_ops.deprecated_xfer_memory = windows_xfer_memory;
   windows_ops.to_xfer_partial = windows_xfer_partial;
   windows_ops.to_files_info = windows_files_info;
   windows_ops.to_insert_breakpoint = memory_insert_breakpoint;

Re: [PATCH] windows-nat.c: Don't install a deprecated_xfer_memory method.

[Joel Brobecker]

> I'm not setup for proper Cygwin/Windows testing, but I managed to
> cross build gdb from GNU/Linux, and copy the resulting binary to a
> Windows box.  Running that gdb on itself worked well enough to run to
> main, so I can't imagine any regression from this.  Does anyone want
> (and is willing) to run this through the testsuite?

I can give it a spin, but probably not before sometime next week...

> 
-- 
Joel

RE: [PATCH] windows-nat.c: Don't install a deprecated_xfer_memory method.

[Pierre Muller]

Hi Pedro,

  I think that your patch can be further enhanced by this change:
  ReadProcessMemory and WriteProcessMemory 
function both fail and report ERROR_PARTIAL_COPY
in GetLastError
if only a part of the requested memory was read/written.

  Adding partial memory read/writes to windows native
allows for instance to get the true location at which a 
memory chuck is not available anymore instead of the
start position of the read/write attempt if
it crosses over to region not readable or not writable.


Pierre



@@ -2315,20 +2364,23 @@ windows_stop (ptid_t ptid)
   registers_changed ();                /* refresh register state */
 }

-static int
-windows_xfer_memory (CORE_ADDR memaddr, gdb_byte *our, int len,
-                  int write, struct mem_attrib *mem,
-                  struct target_ops *target)
+static LONGEST
+windows_xfer_memory (gdb_byte *readbuf, const gdb_byte *writebuf,
+                    ULONGEST memaddr, LONGEST len)
 {
   SIZE_T done = 0;
-  if (write)
+  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)));
-      if (!WriteProcessMemory (current_process_handle,
-                              (LPVOID) (uintptr_t) memaddr, our,
-                              len, &done))
-       done = 0;
+      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);
     }
@@ -2336,12 +2388,17 @@ windows_xfer_memory (CORE_ADDR memaddr,
     {
       DEBUG_MEM (("gdb: read target memory, %d bytes at %s\n",
                  len, core_addr_to_string (memaddr)));
-      if (!ReadProcessMemory (current_process_handle,
-                             (LPCVOID) (uintptr_t) memaddr, our,
-                             len, &done))
-       done = 0;
+      success = ReadProcessMemory (current_process_handle,
+                                  (LPCVOID) (uintptr_t) memaddr, readbuf,
+                                  len, &done);
+      if (!success)
+       lasterror = GetLastError ();
     }
-  return done;
+  if (!success && lasterror == ERROR_PARTIAL_COPY && done > 0)
+    return done;
+  else
+    return success ? done : TARGET_XFER_E_IO;
+
 }

 static void

> -----Message d'origine-----
> De : gdb-patches-owner@sourceware.org [mailto:gdb-patches-
> owner@sourceware.org] De la part de Pedro Alves
> Envoyé : vendredi 23 août 2013 20:13
> À : gdb-patches@sourceware.org
> Objet : [PATCH] windows-nat.c: Don't install a deprecated_xfer_memory
> method.
> 
> This stops another target from installing a
> target_ops->deprecated_xfer_memory method.
> 
> I'm not setup for proper Cygwin/Windows testing, but I managed to
> cross build gdb from GNU/Linux, and copy the resulting binary to a
> Windows box.  Running that gdb on itself worked well enough to run to
> main, so I can't imagine any regression from this.  Does anyone want
> (and is willing) to run this through the testsuite?
> 
> gdb/
> 2013-08-23  Pedro Alves  <palves@redhat.com>
> 
> 	* windows-nat.c (windows_xfer_memory): Adjust prototype to follow
> 	xfer_partial's interface.  Return TARGET_XFER_E_IO on error.
> 	(windows_xfer_partial): Defer TARGET_OBJECT_MEMORY handling to
> 	windows_xfer_memory directly.
> 	(init_windows_ops): Don't install a deprecated_xfer_memory method.
> ---
>  gdb/windows-nat.c |   36 ++++++++++++++----------------------
>  1 file changed, 14 insertions(+), 22 deletions(-)
> 
> diff --git a/gdb/windows-nat.c b/gdb/windows-nat.c
> index 9a0241b..a4e7107 100644
> --- a/gdb/windows-nat.c
> +++ b/gdb/windows-nat.c
> @@ -2315,20 +2315,20 @@ windows_stop (ptid_t ptid)
>    registers_changed ();		/* refresh register state */
>  }
> 
> -static int
> -windows_xfer_memory (CORE_ADDR memaddr, gdb_byte *our, int len,
> -		   int write, struct mem_attrib *mem,
> -		   struct target_ops *target)
> +static LONGEST
> +windows_xfer_memory (gdb_byte *readbuf, const gdb_byte *writebuf,
> +		     ULONGEST memaddr, LONGEST len)
>  {
>    SIZE_T done = 0;
> -  if (write)
> +  BOOL success;
> +
> +  if (writebuf != NULL)
>      {
>        DEBUG_MEM (("gdb: write target memory, %d bytes at %s\n",
>  		  len, core_addr_to_string (memaddr)));
> -      if (!WriteProcessMemory (current_process_handle,
> -			       (LPVOID) (uintptr_t) memaddr, our,
> -			       len, &done))
> -	done = 0;
> +      success = WriteProcessMemory (current_process_handle,
> +				    (LPVOID) (uintptr_t) memaddr, writebuf,
> +				    len, &done);
>        FlushInstructionCache (current_process_handle,
>  			     (LPCVOID) (uintptr_t) memaddr, len);
>      }
> @@ -2336,12 +2336,11 @@ windows_xfer_memory (CORE_ADDR memaddr, gdb_byte
> *our, int len,
>      {
>        DEBUG_MEM (("gdb: read target memory, %d bytes at %s\n",
>  		  len, core_addr_to_string (memaddr)));
> -      if (!ReadProcessMemory (current_process_handle,
> -			      (LPCVOID) (uintptr_t) memaddr, our,
> -			      len, &done))
> -	done = 0;
> +      success = ReadProcessMemory (current_process_handle,
> +				   (LPCVOID) (uintptr_t) memaddr, readbuf,
> +				   len, &done);
>      }
> -  return done;
> +  return success ? done : TARGET_XFER_E_IO;
>  }
> 
>  static void
> @@ -2442,13 +2441,7 @@ windows_xfer_partial (struct target_ops *ops, enum
> target_object object,
>    switch (object)
>      {
>      case TARGET_OBJECT_MEMORY:
> -      if (readbuf)
> -	return (*ops->deprecated_xfer_memory) (offset, readbuf,
> -					       len, 0/*read*/, NULL, ops);
> -      if (writebuf)
> -	return (*ops->deprecated_xfer_memory) (offset, (gdb_byte *) writebuf,
> -					       len, 1/*write*/, NULL, ops);
> -      return -1;
> +      return windows_xfer_memory (readbuf, writebuf, offset, len);
> 
>      case TARGET_OBJECT_LIBRARIES:
>        return windows_xfer_shared_libraries (ops, object, annex, readbuf,
> @@ -2502,7 +2495,6 @@ init_windows_ops (void)
>    windows_ops.to_fetch_registers = windows_fetch_inferior_registers;
>    windows_ops.to_store_registers = windows_store_inferior_registers;
>    windows_ops.to_prepare_to_store = windows_prepare_to_store;
> -  windows_ops.deprecated_xfer_memory = windows_xfer_memory;
>    windows_ops.to_xfer_partial = windows_xfer_partial;
>    windows_ops.to_files_info = windows_files_info;
>    windows_ops.to_insert_breakpoint = memory_insert_breakpoint;

Re: [PATCH] windows-nat.c: Don't install a deprecated_xfer_memory method.

[Pedro Alves]

On 08/23/2013 10:36 PM, Pierre Muller wrote:
> Hi Pedro,
> 
>   I think that your patch can be further enhanced by this change:
>   ReadProcessMemory and WriteProcessMemory 
> function both fail and report ERROR_PARTIAL_COPY
> in GetLastError
> if only a part of the requested memory was read/written.

Interesting.  It does sound like a good idea.

However, could you send that as a separate patch, please?
Otherwise, the single combined patch can potentially cause issues
due to two logically unrelated changes: potential problems caused by
not installing deprecated_xfer_memory anymore, and instead going
through xfer_partial directly; and then potential problems due to
that change.  Keeping the patches separate allows for better
bisecting for what might have caused a regression.

> 
>   Adding partial memory read/writes to windows native
> allows for instance to get the true location at which a 
> memory chuck is not available anymore instead of the
> start position of the read/write attempt if
> it crosses over to region not readable or not writable.

Thanks,
-- 
Pedro Alves

Re: [PATCH] windows-nat.c: Don't install a deprecated_xfer_memory method.

[Pedro Alves]

On 08/26/2013 05:52 PM, Pedro Alves wrote:
> On 08/23/2013 10:36 PM, Pierre Muller wrote:
>> Hi Pedro,
>>
>>   I think that your patch can be further enhanced by this change:
>>   ReadProcessMemory and WriteProcessMemory 
>> function both fail and report ERROR_PARTIAL_COPY
>> in GetLastError
>> if only a part of the requested memory was read/written.
> 
> Interesting.  It does sound like a good idea.
> 
> However, could you send that as a separate patch, please?

And I meant to add for forgot -- that separate patch could
then also propose the same change for gdbserver too.  :-)

> Otherwise, the single combined patch can potentially cause issues
> due to two logically unrelated changes: potential problems caused by
> not installing deprecated_xfer_memory anymore, and instead going
> through xfer_partial directly; and then potential problems due to
> that change.  Keeping the patches separate allows for better
> bisecting for what might have caused a regression.


-- 
Pedro Alves

Re: [PATCH] windows-nat.c: Don't install a deprecated_xfer_memory method.

[Yao Qi]

On 08/24/2013 02:12 AM, Pedro Alves wrote:
> main, so I can't imagine any regression from this.  Does anyone want
> (and is willing) to run this through the testsuite?
>

Pedro,
I tested this patch on mingw native GDB.  There is no regression.  Note 
that I also applied two local patches to make the test result usable.

  1. [PATCH] Unbuffer stdout and stderr on windows (reviewed but not 
approved yet.)
     https://sourceware.org/ml/gdb-patches/2013-08/msg00660.html
  2. A hack to set stdout and stderr to binary mode.

>
> -static int
> -windows_xfer_memory (CORE_ADDR memaddr, gdb_byte *our, int len,
> -		   int write, struct mem_attrib *mem,
> -		   struct target_ops *target)
> +static LONGEST
> +windows_xfer_memory (gdb_byte *readbuf, const gdb_byte *writebuf,
> +		     ULONGEST memaddr, LONGEST len)

It is not installed to deprecated_xfer_memory any more, so we need
documentation on this function.

-- 
Yao (齐尧)

Re: [PATCH] windows-nat.c: Don't install a deprecated_xfer_memory method.

[Pedro Alves]

On 08/27/2013 01:48 AM, Yao Qi wrote:
> On 08/24/2013 02:12 AM, Pedro Alves wrote:
>> main, so I can't imagine any regression from this.  Does anyone want
>> (and is willing) to run this through the testsuite?
>>
> 
> Pedro,
> I tested this patch on mingw native GDB.  There is no regression.  

Thanks!  I've applied this then.

>> -static int
>> -windows_xfer_memory (CORE_ADDR memaddr, gdb_byte *our, int len,
>> -		   int write, struct mem_attrib *mem,
>> -		   struct target_ops *target)
>> +static LONGEST
>> +windows_xfer_memory (gdb_byte *readbuf, const gdb_byte *writebuf,
>> +		     ULONGEST memaddr, LONGEST len)
> 
> It is not installed to deprecated_xfer_memory any more, so we need
> documentation on this function.

Guess so.  Below's what I applied.

------
windows-nat.c: Don't install a deprecated_xfer_memory method.

This stops another target from installing a
target_ops->deprecated_xfer_memory method.

Tested on native MinGW.

gdb/
2013-08-27  Pedro Alves  <palves@redhat.com>

	* windows-nat.c (windows_xfer_memory): Adjust prototype to follow
	xfer_partial's interface.  Return TARGET_XFER_E_IO on error.
	(windows_xfer_partial): Defer TARGET_OBJECT_MEMORY handling to
	windows_xfer_memory directly.
	(init_windows_ops): Don't install a deprecated_xfer_memory method.
---
 gdb/windows-nat.c |   39 +++++++++++++++++----------------------
 1 file changed, 17 insertions(+), 22 deletions(-)

diff --git a/gdb/windows-nat.c b/gdb/windows-nat.c
index 9a0241b..2ffaad4 100644
--- a/gdb/windows-nat.c
+++ b/gdb/windows-nat.c
@@ -2315,20 +2315,23 @@ windows_stop (ptid_t ptid)
   registers_changed ();		/* refresh register state */
 }
 
-static int
-windows_xfer_memory (CORE_ADDR memaddr, gdb_byte *our, int len,
-		   int write, struct mem_attrib *mem,
-		   struct target_ops *target)
+/* Helper for windows_xfer_partial that handles memory transfers.
+   Arguments are like target_xfer_partial.  */
+
+static LONGEST
+windows_xfer_memory (gdb_byte *readbuf, const gdb_byte *writebuf,
+		     ULONGEST memaddr, LONGEST len)
 {
   SIZE_T done = 0;
-  if (write)
+  BOOL success;
+
+  if (writebuf != NULL)
     {
       DEBUG_MEM (("gdb: write target memory, %d bytes at %s\n",
 		  len, core_addr_to_string (memaddr)));
-      if (!WriteProcessMemory (current_process_handle,
-			       (LPVOID) (uintptr_t) memaddr, our,
-			       len, &done))
-	done = 0;
+      success = WriteProcessMemory (current_process_handle,
+				    (LPVOID) (uintptr_t) memaddr, writebuf,
+				    len, &done);
       FlushInstructionCache (current_process_handle,
 			     (LPCVOID) (uintptr_t) memaddr, len);
     }
@@ -2336,12 +2339,11 @@ windows_xfer_memory (CORE_ADDR memaddr, gdb_byte *our, int len,
     {
       DEBUG_MEM (("gdb: read target memory, %d bytes at %s\n",
 		  len, core_addr_to_string (memaddr)));
-      if (!ReadProcessMemory (current_process_handle,
-			      (LPCVOID) (uintptr_t) memaddr, our,
-			      len, &done))
-	done = 0;
+      success = ReadProcessMemory (current_process_handle,
+				   (LPCVOID) (uintptr_t) memaddr, readbuf,
+				   len, &done);
     }
-  return done;
+  return success ? done : TARGET_XFER_E_IO;
 }
 
 static void
@@ -2442,13 +2444,7 @@ windows_xfer_partial (struct target_ops *ops, enum target_object object,
   switch (object)
     {
     case TARGET_OBJECT_MEMORY:
-      if (readbuf)
-	return (*ops->deprecated_xfer_memory) (offset, readbuf,
-					       len, 0/*read*/, NULL, ops);
-      if (writebuf)
-	return (*ops->deprecated_xfer_memory) (offset, (gdb_byte *) writebuf,
-					       len, 1/*write*/, NULL, ops);
-      return -1;
+      return windows_xfer_memory (readbuf, writebuf, offset, len);
 
     case TARGET_OBJECT_LIBRARIES:
       return windows_xfer_shared_libraries (ops, object, annex, readbuf,
@@ -2502,7 +2498,6 @@ init_windows_ops (void)
   windows_ops.to_fetch_registers = windows_fetch_inferior_registers;
   windows_ops.to_store_registers = windows_store_inferior_registers;
   windows_ops.to_prepare_to_store = windows_prepare_to_store;
-  windows_ops.deprecated_xfer_memory = windows_xfer_memory;
   windows_ops.to_xfer_partial = windows_xfer_partial;
   windows_ops.to_files_info = windows_files_info;
   windows_ops.to_insert_breakpoint = memory_insert_breakpoint;

Re: [PATCH] windows-nat.c: Don't install a deprecated_xfer_memory method.

[Pedro Alves]

On 08/23/2013 09:08 PM, Joel Brobecker wrote:
>> I'm not setup for proper Cygwin/Windows testing, but I managed to
>> cross build gdb from GNU/Linux, and copy the resulting binary to a
>> Windows box.  Running that gdb on itself worked well enough to run to
>> main, so I can't imagine any regression from this.  Does anyone want
>> (and is willing) to run this through the testsuite?
> 
> I can give it a spin, but probably not before sometime next week...

Thanks Joel.  Yao already ran this through testing, so it
won't be necessary either.

-- 
Pedro Alves