[commit] Tighten memory read/write methods

[commit] Tighten memory read/write methods

[Andrew Cagney]

[-- Attachment #1: Type: text/plain, Size: 267 bytes --]

Hello,

This cleans up the {target_,}{read,write}_memory methods making the 
buffer parameter a bfd_byte (instead of "is it signed?" char) and for 
the write methods a constant buffer.

fallout from trying to make some of the value stuff constant.

committed,
Andrew

[-- Attachment #2: diffs --]
[-- Type: text/plain, Size: 5314 bytes --]

2005-01-27  Andrew Cagney  <cagney@gnu.org>

	* symfile-mem.c (do_target_read_memory): New function.
	(symbol_file_add_from_memory): Pass do_target_read_memory to
	bfd_elf_bfd_from_remote_memory.
	* corefile.c (write_memory): Update, make a copy of the read-only
	buffer.
	* target.c (target_read_memory): Update.
	(target_write_memory): Update, make a copy of the read-only
	buffer.
	* gdbcore.h (write_memory): Change buffer type to bfd_byte, make
	const.
	* target.h (target_read_memory, target_write_memory): Change
	buffer type to bfd_byte; for write_memory, make it const.

Index: corefile.c
===================================================================
RCS file: /cvs/src/src/gdb/corefile.c,v
retrieving revision 1.28
diff -p -u -r1.28 corefile.c
--- corefile.c	13 Jan 2005 23:56:25 -0000	1.28
+++ corefile.c	27 Jan 2005 20:07:21 -0000
@@ -347,11 +347,13 @@ read_memory_typed_address (CORE_ADDR add
 
 /* Same as target_write_memory, but report an error if can't write.  */
 void
-write_memory (CORE_ADDR memaddr, char *myaddr, int len)
+write_memory (CORE_ADDR memaddr, const bfd_byte *myaddr, int len)
 {
   int status;
-
-  status = target_write_memory (memaddr, myaddr, len);
+  bfd_byte *bytes = alloca (len);
+  
+  memcpy (bytes, myaddr, len);
+  status = target_write_memory (memaddr, bytes, len);
   if (status != 0)
     memory_error (status, memaddr);
 }
Index: gdbcore.h
===================================================================
RCS file: /cvs/src/src/gdb/gdbcore.h,v
retrieving revision 1.16
diff -p -u -r1.16 gdbcore.h
--- gdbcore.h	14 Jan 2005 00:21:19 -0000	1.16
+++ gdbcore.h	27 Jan 2005 20:07:21 -0000
@@ -86,7 +86,7 @@ CORE_ADDR read_memory_typed_address (COR
    byteswapping, alignment, different sizes for host vs. target types,
    etc.  */
 
-extern void write_memory (CORE_ADDR memaddr, char *myaddr, int len);
+extern void write_memory (CORE_ADDR memaddr, const bfd_byte *myaddr, int len);
 
 /* Store VALUE at ADDR in the inferior as a LEN-byte unsigned integer.  */
 extern void write_memory_unsigned_integer (CORE_ADDR addr, int len,
Index: symfile-mem.c
===================================================================
RCS file: /cvs/src/src/gdb/symfile-mem.c,v
retrieving revision 1.6
diff -p -u -r1.6 symfile-mem.c
--- symfile-mem.c	14 Jan 2005 23:27:14 -0000	1.6
+++ symfile-mem.c	27 Jan 2005 20:07:21 -0000
@@ -58,6 +58,15 @@
 #include "elf/common.h"
 
 
+/* FIXME: cagney/2005-01-27: Should be a function with the signature:
+   int (void *object, const bfd_byte *myaddr, int len).  */
+
+static int
+do_target_read_memory (bfd_vma vma, char *myaddr, int len)
+{
+  return target_read_memory (vma, myaddr, len);
+}
+
 /* Read inferior memory at ADDR to find the header of a loaded object file
    and read its in-core symbols out of inferior memory.  TEMPL is a bfd
    representing the target's format.  NAME is the name to use for this
@@ -78,7 +87,7 @@ symbol_file_add_from_memory (struct bfd 
     error ("add-symbol-file-from-memory not supported for this target");
 
   nbfd = bfd_elf_bfd_from_remote_memory (templ, addr, &loadbase,
-					 target_read_memory);
+					 do_target_read_memory);
   if (nbfd == NULL)
     error ("Failed to read a valid object file image from memory.");
 
Index: target.c
===================================================================
RCS file: /cvs/src/src/gdb/target.c,v
retrieving revision 1.91
diff -p -u -r1.91 target.c
--- target.c	18 Jan 2005 17:04:28 -0000	1.91
+++ target.c	27 Jan 2005 20:07:21 -0000
@@ -995,7 +995,7 @@ xfer_using_stratum (enum target_object o
    deal with partial reads should call target_read_memory_partial. */
 
 int
-target_read_memory (CORE_ADDR memaddr, char *myaddr, int len)
+target_read_memory (CORE_ADDR memaddr, bfd_byte *myaddr, int len)
 {
   if (target_xfer_partial_p ())
     return xfer_using_stratum (TARGET_OBJECT_MEMORY, NULL,
@@ -1005,13 +1005,15 @@ target_read_memory (CORE_ADDR memaddr, c
 }
 
 int
-target_write_memory (CORE_ADDR memaddr, char *myaddr, int len)
+target_write_memory (CORE_ADDR memaddr, const bfd_byte *myaddr, int len)
 {
+  bfd_byte *bytes = alloca (len);
+  memcpy (bytes, myaddr, len);
   if (target_xfer_partial_p ())
     return xfer_using_stratum (TARGET_OBJECT_MEMORY, NULL,
-			       memaddr, len, NULL, myaddr);
+			       memaddr, len, NULL, bytes);
   else
-    return target_xfer_memory (memaddr, myaddr, len, 1);
+    return target_xfer_memory (memaddr, bytes, len, 1);
 }
 
 #ifndef target_stopped_data_address_p
Index: target.h
===================================================================
RCS file: /cvs/src/src/gdb/target.h,v
retrieving revision 1.67
diff -p -u -r1.67 target.h
--- target.h	18 Jan 2005 17:04:28 -0000	1.67
+++ target.h	27 Jan 2005 20:07:21 -0000
@@ -537,9 +537,10 @@ extern int do_xfer_memory (CORE_ADDR mem
 
 extern int target_read_string (CORE_ADDR, char **, int, int *);
 
-extern int target_read_memory (CORE_ADDR memaddr, char *myaddr, int len);
+extern int target_read_memory (CORE_ADDR memaddr, bfd_byte *myaddr, int len);
 
-extern int target_write_memory (CORE_ADDR memaddr, char *myaddr, int len);
+extern int target_write_memory (CORE_ADDR memaddr, const bfd_byte *myaddr,
+				int len);
 
 extern int xfer_memory (CORE_ADDR, char *, int, int,
 			struct mem_attrib *, struct target_ops *);

Re: [commit] Tighten memory read/write methods

[Mark Kettenis]

   Date: Thu, 27 Jan 2005 15:11:19 -0500
   From: Andrew Cagney <cagney@gnu.org>

   Hello,

   This cleans up the {target_,}{read,write}_memory methods making the 
   buffer parameter a bfd_byte (instead of "is it signed?" char)

Just curious, but is that really an issue in these buffer-like
contexts?  I thought signed-ness is only an issue if there is an
(implcit) conversion to an integer type involved.  If there are such
issues, bfd_byte seems like the appropriate type to use (but please
read on), but otherwise I'd prefer using standard ISO C types.

Anyway, isn't it better to sidestep the issue entirely, and use 'void
*' in these contexts?  That's what we have been doing in the past I
think.  Most 'char *' stuff is only there because too many people
still remember K&R C.

Anyway, I think we shouldn't change these things haphazardly.  Can we
formulate a set of programming guidelines such that we can try to be a
bit more consistent.  My set of rules would be:

* Use const wherever possible.
* Use 'void *' wherever possible.
* Use 'char *' in context where you need to add an offset to a pointer.

Mark

Re: [commit] Tighten memory read/write methods

[Andrew Cagney]

Mark Kettenis wrote:
>    Date: Thu, 27 Jan 2005 15:11:19 -0500
>    From: Andrew Cagney <cagney@gnu.org>
> 
>    Hello,
> 
>    This cleans up the {target_,}{read,write}_memory methods making the 
>    buffer parameter a bfd_byte (instead of "is it signed?" char)

In constant propogating I'm making the following mind numbing 
transformations:

char -> const bfd_byte
unsigned char -> const bfd_byte
void -> const void

The first two are important.  Some compilers [rightly] complain about 
incompatibility between signed/unsigned char; and on ppc with it's 
unsigned char, results just get weird.

We can certainly debate the merits of ISO vs BFD and bfd_byte vs void, 
however lets keep that debate separate to my current task - getting 
constants sufficiently propogated for me to do my next value.h commit 
which in turn finishes DW_OP_piece.

Andrew

Re: [commit] Tighten memory read/write methods

[Mark Kettenis]

   Date: Thu, 27 Jan 2005 18:06:18 -0500
   From: Andrew Cagney <cagney@gnu.org>

   Mark Kettenis wrote:
   >    Date: Thu, 27 Jan 2005 15:11:19 -0500
   >    From: Andrew Cagney <cagney@gnu.org>
   > 
   >    Hello,
   > 
   >    This cleans up the {target_,}{read,write}_memory methods making the 
   >    buffer parameter a bfd_byte (instead of "is it signed?" char)

   In constant propogating I'm making the following mind numbing 
   transformations:

   char -> const bfd_byte
   unsigned char -> const bfd_byte
   void -> const void

   The first two are important.  Some compilers [rightly] complain about 
   incompatibility between signed/unsigned char; and on ppc with it's 
   unsigned char, results just get weird.

   We can certainly debate the merits of ISO vs BFD and bfd_byte vs void, 
   however lets keep that debate separate to my current task - getting 
   constants sufficiently propogated for me to do my next value.h commit 
   which in turn finishes DW_OP_piece.

OK, I can understand your rationale for seperating const-correctness
from other transformations; but then why don't you seperate the
(unsigned) char -> bfd_byte transformation too?  I'm all for
consistent use of 'bfd_byte *' as the canonical way to point to a
buffer interpreted as seperate bytes.  However, I think that pointers
to generic bits of memory should be 'void *' (which specific bits of
code might want to cast to 'bfd_byte *' if they're going to interpret
the bytes individually).  Doing the mind-numbing conversion means that
we'll have to re-evaluate all occurances of 'bfd_byte' again later.

Mark

Re: [commit] Tighten memory read/write methods

[Eli Zaretskii]

> Date: Fri, 28 Jan 2005 09:45:46 +0100 (CET)
> From: Mark Kettenis <kettenis@gnu.org>
> CC: gdb-patches@sources.redhat.com
> 
> OK, I can understand your rationale for seperating const-correctness
> from other transformations; but then why don't you seperate the
> (unsigned) char -> bfd_byte transformation too?  I'm all for
> consistent use of 'bfd_byte *' as the canonical way to point to a
> buffer interpreted as seperate bytes.  However, I think that pointers
> to generic bits of memory should be 'void *' (which specific bits of
> code might want to cast to 'bfd_byte *' if they're going to interpret
> the bytes individually).  Doing the mind-numbing conversion means that
> we'll have to re-evaluate all occurances of 'bfd_byte' again later.

I don't like the use of bfd_byte either.  I think we shouldn't use BFD
types in our sources for data types that don't have anything to do
with the BFD library.  If, for some reason, "void *" somehow doesn't
fit the bill (and I'd like to see evidence to that before I agree),
I'd suggest our own data type, like gdb_byte or some such.

P.S.  IMHO, this is one more example of a situation where a discussion
that preceded commits would be in order.  I don't know about others,
but as far as I'm concerned, when such controversial changes are
committed without discussions, it doesn't help me to feel a part of a
team.  (And please don't tell me that Andrew had a right to do this:
this is not about rights, but about using them indiscriminantly.)

Re: [commit] Tighten memory read/write methods

[Andrew Cagney]

Eli,

Like I said to Mark:

> We can certainly debate the merits of ISO vs BFD and bfd_byte vs void [vs gdb_byte], however lets keep that debate separate to my current task - getting constants sufficiently propogated for me to do my next value.h commit which in turn finishes DW_OP_piece. 

Andrew

Re: [commit] Tighten memory read/write methods

[Mark Kettenis]

   Date: Fri, 28 Jan 2005 13:48:14 -0500
   From: Andrew Cagney <cagney@gnu.org>

   Eli,

   Like I said to Mark:

   > We can certainly debate the merits of ISO vs BFD and bfd_byte vs
   > void [vs gdb_byte], however lets keep that debate separate to my
   > current task - getting constants sufficiently propogated for me
   > to do my next value.h commit which in turn finishes DW_OP_piece.

You can constify without introducing bfd_byte.

Mark

Re: [commit] Tighten memory read/write methods

[Eli Zaretskii]

> Date: Fri, 28 Jan 2005 13:48:14 -0500
> From: Andrew Cagney <cagney@gnu.org>
> Cc: gdb-patches@sources.redhat.com, Mark Kettenis <kettenis@gnu.org>
> 
> Like I said to Mark:
> 
> > We can certainly debate the merits of ISO vs BFD and bfd_byte vs void [vs gdb_byte], however lets keep that debate separate to my current task - getting constants sufficiently propogated for me to do my next value.h commit which in turn finishes DW_OP_piece. 

(Please don't assume that I didn't read your messages, nor that
reiterating them will help resolving the issues.)

If finishing DW_OP_piece causes contamination of GDB sources with
extraneous identifiers that should not be there, I object to your
doing that without asking for consensus.

In other words, these are indeed two separate issues, but since the
side effect of your solution is much broader than what is strictly
needed for DW_OP_piece, we should discuss and decide on the bfd_byte
thingy separately, _before_ it is used, not _after_.

So please stop committing changes that spread bfd_byte across the
sources until we discuss this and come to some consensus.  You've
heard 2 maintainers object to that, and yet you still continue with
committing more and more of the changes to which we object.

Re: [commit] Tighten memory read/write methods

[Mark Kettenis]

   Date: Thu, 27 Jan 2005 15:11:19 -0500
   From: Andrew Cagney <cagney@gnu.org>

   Index: corefile.c
   ===================================================================
   RCS file: /cvs/src/src/gdb/corefile.c,v
   retrieving revision 1.28
   diff -p -u -r1.28 corefile.c
   --- corefile.c	13 Jan 2005 23:56:25 -0000	1.28
   +++ corefile.c	27 Jan 2005 20:07:21 -0000
   @@ -347,11 +347,13 @@ read_memory_typed_address (CORE_ADDR add

    /* Same as target_write_memory, but report an error if can't write.  */
    void
   -write_memory (CORE_ADDR memaddr, char *myaddr, int len)
   +write_memory (CORE_ADDR memaddr, const bfd_byte *myaddr, int len)
    {
      int status;
   -
   -  status = target_write_memory (memaddr, myaddr, len);
   +  bfd_byte *bytes = alloca (len);
   +  
   +  memcpy (bytes, myaddr, len);
   +  status = target_write_memory (memaddr, bytes, len);
      if (status != 0)
	memory_error (status, memaddr);
    }

Is this addiotional copy really necessary now that you've properly
consified target_write_memory?

Mark