Patchwork [v2,2/7] Cleanup some docs about memory write

login
register
mail settings
Submitter Simon Marchi
Date April 15, 2015, 7:47 p.m.
Message ID <1429127258-1033-3-git-send-email-simon.marchi@ericsson.com>
Download mbox | patch
Permalink /patch/6238/
State New
Headers show

Comments

Simon Marchi - April 15, 2015, 7:47 p.m.
Some docs seemed outdated. In the case of target_write_memory, the docs
in target.c and target/target.h diverged a bit, so I tried to find a
reasonnable in-between version.

gdb/ChangeLog:

	* corefile.c (write_memory): Update doc.
	* gdbcore.h (write_memory): Same.
	* target.c (target_write_memory): Same.
	* target/target.h (target_write_memory): Same.
---
 gdb/corefile.c      |  4 ++--
 gdb/gdbcore.h       |  6 ++----
 gdb/target.c        |  6 +-----
 gdb/target/target.h | 10 +++++-----
 4 files changed, 10 insertions(+), 16 deletions(-)
Pedro Alves - May 21, 2015, 5:45 p.m.
On 04/15/2015 08:47 PM, Simon Marchi wrote:
> Some docs seemed outdated. In the case of target_write_memory, the docs
> in target.c and target/target.h diverged a bit, so I tried to find a
> reasonnable in-between version.

"reasonable"

> 
> gdb/ChangeLog:
> 
> 	* corefile.c (write_memory): Update doc.
> 	* gdbcore.h (write_memory): Same.
> 	* target.c (target_write_memory): Same.
> 	* target/target.h (target_write_memory): Same.
> ---
>  gdb/corefile.c      |  4 ++--
>  gdb/gdbcore.h       |  6 ++----
>  gdb/target.c        |  6 +-----
>  gdb/target/target.h | 10 +++++-----
>  4 files changed, 10 insertions(+), 16 deletions(-)
> 
> diff --git a/gdb/corefile.c b/gdb/corefile.c
> index a042e6d..83b0e80 100644
> --- a/gdb/corefile.c
> +++ b/gdb/corefile.c
> @@ -383,8 +383,8 @@ read_memory_typed_address (CORE_ADDR addr, struct type *type)
>    return extract_typed_address (buf, type);
>  }
>  
> -/* Same as target_write_memory, but report an error if can't
> -   write.  */
> +/* See gdbcore.h. */

Double space after period.

> diff --git a/gdb/target.c b/gdb/target.c
> index fcf7090..bd9a0eb 100644
>  int
>  target_write_memory (CORE_ADDR memaddr, const gdb_byte *myaddr, ssize_t len)

> diff --git a/gdb/target/target.h b/gdb/target/target.h
> index 05ac758..e0b7554 100644
> --- a/gdb/target/target.h
> +++ b/gdb/target/target.h
> @@ -49,12 +49,12 @@ extern int target_read_memory (CORE_ADDR memaddr, gdb_byte *myaddr,
>  extern int target_read_uint32 (CORE_ADDR memaddr, uint32_t *result);
>  
>  /* Write LEN bytes from MYADDR to target memory at address MEMADDR.
> -   Return zero for success, nonzero if any error occurs.  This
> +   Return zero for success or TARGET_XFER_E_IO if any error occurs.  This
>     function must be provided by the client.  Implementations of this
> -   function may define and use their own error codes, but functions
> -   in the common, nat and target directories must treat the return
> -   code as opaque.  No guarantee is made about the contents of the
> -   data at MEMADDR if any error occurs.  */
> +   function may define and use their own error codes, but functions in
> +   the common, nat and target directories must treat the return code as
> +   opaque.  No guarantee is made about how much data got written if any
> +   error occurs.  */
>  
>  extern int target_write_memory (CORE_ADDR memaddr, const gdb_byte *myaddr,
>  				ssize_t len);
> 

This one's not right.  TARGET_XFER_E_IO is a GDB-specific thing, while
this declaration is meant for both gdb and gdbserver.  It's what
"This function must be provided by the client" refers to.  It doesn't
make sense to say "TARGET_XFER_E_IO" one error and then explain that
the return code is opaque.

So probably it'd be better to leave this comment:

> --- a/gdb/target.c
> +++ b/gdb/target.c
> @@ -1464,11 +1464,7 @@ target_read_code (CORE_ADDR memaddr, gdb_byte *myaddr, ssize_t len)
>      return TARGET_XFER_E_IO;
>  }
>
> -/* Write LEN bytes from MYADDR to target memory at address MEMADDR.
> -   Returns either 0 for success or TARGET_XFER_E_IO if any
> -   error occurs.  If an error occurs, no guarantee is made about how
> -   much data got written.  Callers that can deal with partial writes
> -   should call target_write.  */
> +/* See target/target.h.  */

... but extend it by starting by saying that this is GDB's implementation
of the function as declared in target/target.h.

Thanks,
Pedro Alves

Patch

diff --git a/gdb/corefile.c b/gdb/corefile.c
index a042e6d..83b0e80 100644
--- a/gdb/corefile.c
+++ b/gdb/corefile.c
@@ -383,8 +383,8 @@  read_memory_typed_address (CORE_ADDR addr, struct type *type)
   return extract_typed_address (buf, type);
 }
 
-/* Same as target_write_memory, but report an error if can't
-   write.  */
+/* See gdbcore.h. */
+
 void
 write_memory (CORE_ADDR memaddr, 
 	      const bfd_byte *myaddr, ssize_t len)
diff --git a/gdb/gdbcore.h b/gdb/gdbcore.h
index 63a75f0..1106db8 100644
--- a/gdb/gdbcore.h
+++ b/gdb/gdbcore.h
@@ -101,10 +101,8 @@  extern void read_memory_string (CORE_ADDR, char *, int);
 
 CORE_ADDR read_memory_typed_address (CORE_ADDR addr, struct type *type);
 
-/* This takes a char *, not void *.  This is probably right, because
-   passing in an int * or whatever is wrong with respect to
-   byteswapping, alignment, different sizes for host vs. target types,
-   etc.  */
+/* Same as target_write_memory, but report an error if can't
+   write.  */
 
 extern void write_memory (CORE_ADDR memaddr, const gdb_byte *myaddr,
 			  ssize_t len);
diff --git a/gdb/target.c b/gdb/target.c
index fcf7090..bd9a0eb 100644
--- a/gdb/target.c
+++ b/gdb/target.c
@@ -1464,11 +1464,7 @@  target_read_code (CORE_ADDR memaddr, gdb_byte *myaddr, ssize_t len)
     return TARGET_XFER_E_IO;
 }
 
-/* Write LEN bytes from MYADDR to target memory at address MEMADDR.
-   Returns either 0 for success or TARGET_XFER_E_IO if any
-   error occurs.  If an error occurs, no guarantee is made about how
-   much data got written.  Callers that can deal with partial writes
-   should call target_write.  */
+/* See target/target.h.  */
 
 int
 target_write_memory (CORE_ADDR memaddr, const gdb_byte *myaddr, ssize_t len)
diff --git a/gdb/target/target.h b/gdb/target/target.h
index 05ac758..e0b7554 100644
--- a/gdb/target/target.h
+++ b/gdb/target/target.h
@@ -49,12 +49,12 @@  extern int target_read_memory (CORE_ADDR memaddr, gdb_byte *myaddr,
 extern int target_read_uint32 (CORE_ADDR memaddr, uint32_t *result);
 
 /* Write LEN bytes from MYADDR to target memory at address MEMADDR.
-   Return zero for success, nonzero if any error occurs.  This
+   Return zero for success or TARGET_XFER_E_IO if any error occurs.  This
    function must be provided by the client.  Implementations of this
-   function may define and use their own error codes, but functions
-   in the common, nat and target directories must treat the return
-   code as opaque.  No guarantee is made about the contents of the
-   data at MEMADDR if any error occurs.  */
+   function may define and use their own error codes, but functions in
+   the common, nat and target directories must treat the return code as
+   opaque.  No guarantee is made about how much data got written if any
+   error occurs.  */
 
 extern int target_write_memory (CORE_ADDR memaddr, const gdb_byte *myaddr,
 				ssize_t len);