Re: [PATCH v2 2/7] Cleanup some docs about memory write

[Simon Marchi <simon dot marchi at ericsson dot com>]

On 15-05-21 01:45 PM, Pedro Alves wrote:
> 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.

Done.

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

Actually it seems obvious now. I'll drop this part from the patch. If we want
to clarify the comments we should do it for all the functions of the interface
(in a separate patch).

> Thanks,
> Pedro Alves

Well then, all that remains would be to fix that stale comment. Is it ok?


>From c9b4d03d6f7ffd84215473456d61ca1bb0c553ac Mon Sep 17 00:00:00 2001
From: Simon Marchi <simon.marchi@ericsson.com>
Date: Fri, 12 Jun 2015 14:53:45 -0400
Subject: [PATCH] Cleanup write_memory doc

This doc about write_memory seems outdated.

gdb/ChangeLog:

	* corefile.c (write_memory): Update doc.
	* gdbcore.h (write_memory): Same.
---
 gdb/corefile.c | 4 ++--
 gdb/gdbcore.h  | 6 ++----
 2 files changed, 4 insertions(+), 6 deletions(-)

diff --git a/gdb/corefile.c b/gdb/corefile.c
index a042e6d..5246f71 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 a437c8a..0c08b37 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);
-- 
2.1.4