A case for `void *' for pointers to arbitrary (byte) buffers

A case for `void *' for pointers to arbitrary (byte) buffers

[Mark Kettenis]

   Date: Tue, 03 May 2005 10:42:46 -0400
   From: Andrew Cagney <cagney@gnu.org>

   ... and such debate belongs on gdb@.  Presumably Mark will respond to 
   your proposal.

OK, here it is.

The Problem
-----------

In many places GDB needs to pass along and pick apart blobs of memory.
In many cases, GDB does this using `char *' pointers.  Small blobs of
memory are often allocated on the stack as `char[]' or `unsigned
char[]'.  Unfortunately ISO C doesn't specify whether `char' is signed
or unsigned.  On most systems, char is signed, but on some systems
(most notably powerpc) it is unsigned.  This leads to many
platform-dependent type conversion bugs in code.  Since we have been
sloppy about the (un)signedness of char in the GDB codebase, there
probably are quite a few of these bugs in GDB.

Fortunately recent versions of GCC have started to warn about
suspicous type conversions.  But because of our sloppyness, the
current GDB doesn't compiler with -Werror on systems with this new
version of GCC.  Compiling without -Werror will hide many bugs in our
codebase, so we should fix this in GDB.


The proper pointer type for arbitrary buffers
---------------------------------------------

Looking at ISO C and in particular the ISO C standard library, we
notice that there are two families of functions that deal with blobs
of memory:

* Functions starting with mem, such as memcpy(), memset(), etc.  These
  functions all use the `void *' type to refer to the blobs of memory
  they operate on.

* Functions starting with str, such as strcpy(), strlen(), etc.  These
  functions don't really operate on arbitrary blobs of memory, but
  rather on NUL-terminated character strings.  They all use the `char
  *' type to refer to these strings.

So from a language standpoint of view we should use `char *' for
NUL-terminated sequences of bytes and `void *' for arbitrary sequences
of bytes.  A nice thing about `void *' is that you can implicitly cast
it to every other pointer type in the world.


Why doesn't GDB use `void *'?
-----------------------------

Well, it does, in some places.  But the vast majority of our codebase
predates ISO C, or at least GDB was supposed to be compiled on
compilers that predate ISO C.  We couldn't count on `void *' being
part of the standard.  And even after we made an ISO C compiler a
requirement for compiling GDB new code still followed the bad examples
from the K&R era.  And of course there is also some laziness involved.
One of the good things about `void *' pointers is that you can't do
any pointer arithmetic with them.  So if you want to do pointer
arithmetic (and GDB has some genuine need to do that) you'll have to
explicitly cast to the right pointer type.  This is a good thing.
This forces you to think about what you're doing.  In the cases where
it matters.


An example
----------

An example of a good GDB interface where the above applies is
extract_unsigned_integer().  Its full prototype is:

LONGEST extract_unsigned_integer (const void *addr, int len);

Its implementation converts the `void *' pointer into an `unsigned
char *' pointer before doing pointer arithmetic.


An example of a bad interface is read_memory().  Its full prototype is:

void read_memory (CORE_ADDR memaddr, char *myaddr, int len)

This function reads LEN bytes from MEMADDR and stores them in MYADDR.
But MYADDR is not necessarily a NUL-terminated string.  The second
argument should be `void *'.


Recently a few `struct value'-related functions were converted from
using `char *' to `bfd_byte *'.  If we take for example unpack_long():

LONGEST unpack_long(struct type *, const bfd_byte *valaddr);

Its interface is very similar to extract_unsigned_integer(), in fact
it is implemented using extract_unsigned_integer().  Therefore it
should use the same pointer type as extract_unsigned_integer(): `void
*'.


Do we need a `bfd_byte *' or `gdb_byte *'?
------------------------------------------

We might.  Many implementations will need to cast to some sort of byte
type to do arithmetic or look at individual bytes.  As we have seen,
using can be `char *' is problematic.  In most (problematic) cases we
really mean `unsigned char', only a few will warrant `signed char'.
And `bfd_byte *' is a nice abbreviation.  Personally I'm perfectly
happy with writing `unsigned char'.  IMHO it has the benefit that it
is perfectly clear that this type is unsigned, even to someone who has
never been exposed to the GDB codebase.


Why not use `xxx_byte *' instead of `void *'?
---------------------------------------------

* It's nonstandard.  Why do we need a nonstandard type if a perfectly
  god standard type is available?

* It doesn't really solve the issue.  It only propagates the problem
  one level up or down.  Since `xxx_byte *' is nothing but a typedef
  for `unsigned char *', someone calling a functions with `xxx_byte *'
  as one of its arguments with a `char *' argument will suffer from
  the warning that raised this entire issue; `void *' breaks the chain
  immediately.


Conclusion
----------

It's clear that we should work actively towards reserving the
unqualified `char *' for strings, and probably never ever use an
unqualified `char' as a data type (the C type of a single character is
`int').  Since GCC 4.0 has been released and will probably be used by
many in a few months time, we should solve this issue now, and
preferably in our complete codebase.  Hopefully we can get consesus
soon, and start taking this issue into account when we review patches.

Mark

Re: A case for `void *' for pointers to arbitrary (byte) buffers

[Daniel Jacobowitz]

On Tue, May 03, 2005 at 10:13:01PM +0200, Mark Kettenis wrote:
> Do we need a `bfd_byte *' or `gdb_byte *'?
> ------------------------------------------
> 
> We might.  Many implementations will need to cast to some sort of byte
> type to do arithmetic or look at individual bytes.  As we have seen,
> using can be `char *' is problematic.  In most (problematic) cases we
> really mean `unsigned char', only a few will warrant `signed char'.
> And `bfd_byte *' is a nice abbreviation.  Personally I'm perfectly
> happy with writing `unsigned char'.  IMHO it has the benefit that it
> is perfectly clear that this type is unsigned, even to someone who has
> never been exposed to the GDB codebase.

I think that using gdb_byte has a consistency advantage over typing
"unsigned char" everywhere.  We will know that we are dealing with
opaque blobs of (usually) target memory, but retain the ability
to do arithmetic on them.

> Why not use `xxx_byte *' instead of `void *'?
> ---------------------------------------------
> 
> * It's nonstandard.  Why do we need a nonstandard type if a perfectly
>   god standard type is available?
> 
> * It doesn't really solve the issue.  It only propagates the problem
>   one level up or down.  Since `xxx_byte *' is nothing but a typedef
>   for `unsigned char *', someone calling a functions with `xxx_byte *'
>   as one of its arguments with a `char *' argument will suffer from
>   the warning that raised this entire issue; `void *' breaks the chain
>   immediately.

I think that's a bad thing!  For the same reason that we use -Werror:
where possible, we can let GCC enforce consistency within our source
base.  Use of gdb_byte (as unsigned char) buys you the advantage that
any other pointer type won't silently convert to it.

If you want to use a standard type, play the necessary autoconf games
to acquire stdint.h.  Use uint8_t *.

-- 
Daniel Jacobowitz
CodeSourcery, LLC

Re: A case for `void *' for pointers to arbitrary (byte) buffers

[Mark Kettenis]

   Date: Tue, 3 May 2005 16:23:52 -0400
   From: Daniel Jacobowitz <drow@false.org>

   > Why not use `xxx_byte *' instead of `void *'?
   > ---------------------------------------------
   > 
   > * It's nonstandard.  Why do we need a nonstandard type if a perfectly
   >   god standard type is available?
   > 
   > * It doesn't really solve the issue.  It only propagates the problem
   >   one level up or down.  Since `xxx_byte *' is nothing but a typedef
   >   for `unsigned char *', someone calling a functions with `xxx_byte *'
   >   as one of its arguments with a `char *' argument will suffer from
   >   the warning that raised this entire issue; `void *' breaks the chain
   >   immediately.

   I think that's a bad thing!  For the same reason that we use -Werror:
   where possible, we can let GCC enforce consistency within our source
   base.  Use of gdb_byte (as unsigned char) buys you the advantage that
   any other pointer type won't silently convert to it.

Ah, but these are supposed to be opaque blobs of memory.

   If you want to use a standard type, play the necessary autoconf games
   to acquire stdint.h.  Use uint8_t *.

That's an interesting suggestion.  It might take a few iterations to
get that right though.

Mark

Re: A case for `void *' for pointers to arbitrary (byte) buffers

[Daniel Jacobowitz]

On Tue, May 03, 2005 at 11:13:24PM +0200, Mark Kettenis wrote:
>    Date: Tue, 3 May 2005 16:23:52 -0400
>    From: Daniel Jacobowitz <drow@false.org>
> 
>    > Why not use `xxx_byte *' instead of `void *'?
>    > ---------------------------------------------
>    > 
>    > * It's nonstandard.  Why do we need a nonstandard type if a perfectly
>    >   god standard type is available?
>    > 
>    > * It doesn't really solve the issue.  It only propagates the problem
>    >   one level up or down.  Since `xxx_byte *' is nothing but a typedef
>    >   for `unsigned char *', someone calling a functions with `xxx_byte *'
>    >   as one of its arguments with a `char *' argument will suffer from
>    >   the warning that raised this entire issue; `void *' breaks the chain
>    >   immediately.
> 
>    I think that's a bad thing!  For the same reason that we use -Werror:
>    where possible, we can let GCC enforce consistency within our source
>    base.  Use of gdb_byte (as unsigned char) buys you the advantage that
>    any other pointer type won't silently convert to it.
> 
> Ah, but these are supposed to be opaque blobs of memory.

That's my point.  This way we can only pass "opaque blobs of memory" to
the interfaces that want opaque blobs.  It's a question of
categorization.  For instance, this is a bug:

  long foo;
  target_read_memory (addr, &foo, 4);

If the second argument is "void *", a C compiler won't complain about
this.  But for "uint8_t *", it will.

>    If you want to use a standard type, play the necessary autoconf games
>    to acquire stdint.h.  Use uint8_t *.
> 
> That's an interesting suggestion.  It might take a few iterations to
> get that right though.

I'm willing to do the work.

-- 
Daniel Jacobowitz
CodeSourcery, LLC

Re: A case for `void *' for pointers to arbitrary (byte) buffers

[Stan Shebs]

Mark Kettenis wrote:

>Why not use `xxx_byte *' instead of `void *'?
>---------------------------------------------
>
>* It's nonstandard.  Why do we need a nonstandard type if a perfectly
>  god standard type is available?
>
My inclination would be for gdb_byte *, as an abstraction of
"what people who understand the issue more than I do think is
the right type to use". 1/2 :-)

Personally I've never been comfortable with using void * because
it has its own set of semi-mysterious rules for use - yeah, if I
wear my compiler-guy hat, they work that way for a reason, but
when I wear my just-wanna-write-code hat, they are more of a
distraction.

Another downside of void * is it doesn't tell the reader whether
it means GDB's byte abstraction, or something else, such as a
declaration that has to be void * for consistency with system
headers or some such.

GDB seems like a big enough program to justify its own byte type.

Stan

Re: A case for `void *' for pointers to arbitrary (byte) buffers

[Mark Kettenis]

   Date: Tue, 3 May 2005 17:16:46 -0400
   From: Daniel Jacobowitz <drow@false.org>

   On Tue, May 03, 2005 at 11:13:24PM +0200, Mark Kettenis wrote:
   >    Date: Tue, 3 May 2005 16:23:52 -0400
   >    From: Daniel Jacobowitz <drow@false.org>
   > 
   >    > Why not use `xxx_byte *' instead of `void *'?
   >    > ---------------------------------------------
   >    > 
   >    > * It's nonstandard.  Why do we need a nonstandard type if a perfectly
   >    >   god standard type is available?
   >    > 
   >    > * It doesn't really solve the issue.  It only propagates the problem
   >    >   one level up or down.  Since `xxx_byte *' is nothing but a typedef
   >    >   for `unsigned char *', someone calling a functions with `xxx_byte *'
   >    >   as one of its arguments with a `char *' argument will suffer from
   >    >   the warning that raised this entire issue; `void *' breaks the chain
   >    >   immediately.
   > 
   >    I think that's a bad thing!  For the same reason that we use -Werror:
   >    where possible, we can let GCC enforce consistency within our source
   >    base.  Use of gdb_byte (as unsigned char) buys you the advantage that
   >    any other pointer type won't silently convert to it.
   > 
   > Ah, but these are supposed to be opaque blobs of memory.

   That's my point.  This way we can only pass "opaque blobs of memory" to
   the interfaces that want opaque blobs.  It's a question of
   categorization.  For instance, this is a bug:

     long foo;
     target_read_memory (addr, &foo, 4);

   If the second argument is "void *", a C compiler won't complain about
   this.  But for "uint8_t *", it will.

Hmm, you have a point here...

   >    If you want to use a standard type, play the necessary autoconf games
   >    to acquire stdint.h.  Use uint8_t *.
   > 
   > That's an interesting suggestion.  It might take a few iterations to
   > get that right though.

   I'm willing to do the work.

OK great!  I really like being able to use those (u)intN_t types, and
especially (u)intptr_t.  You can probably steal the autoconf checks
from GNU coreutils.  Those will have been pretty well tested I sppose.
But the base sequence should something like:

#ifdef HAVE_STDINT_H
#include <stdint.h>
#else
# ifdef HAVE_INTTYPES_H
# include <inttypes.h>
# else
typedef unsigned char uint8_t;
# endif
#endif

Mark

Re: A case for `void *' for pointers to arbitrary (byte) buffers

[Daniel Jacobowitz]

On Wed, May 04, 2005 at 12:06:26AM +0200, Mark Kettenis wrote:
>    >    If you want to use a standard type, play the necessary autoconf games
>    >    to acquire stdint.h.  Use uint8_t *.
>    > 
>    > That's an interesting suggestion.  It might take a few iterations to
>    > get that right though.
> 
>    I'm willing to do the work.
> 
> OK great!  I really like being able to use those (u)intN_t types, and
> especially (u)intptr_t.  You can probably steal the autoconf checks
> from GNU coreutils.  Those will have been pretty well tested I sppose.
> But the base sequence should something like:
> 
> #ifdef HAVE_STDINT_H
> #include <stdint.h>
> #else
> # ifdef HAVE_INTTYPES_H
> # include <inttypes.h>
> # else
> typedef unsigned char uint8_t;
> # endif
> #endif

To be honest, my preference is still for gdb_byte * over uint8_t *, for
exactly the reason Stan just mentioned: consistency.  Whether that's
defined as unsigned char or uint8_t is less important to me.

Does gdb_byte, defined in terms of uint8_t, work for you?

-- 
Daniel Jacobowitz
CodeSourcery, LLC

Re: A case for `void *' for pointers to arbitrary (byte) buffers

[Joel Brobecker]

> To be honest, my preference is still for gdb_byte * over uint8_t *, for
> exactly the reason Stan just mentioned: consistency.  Whether that's
> defined as unsigned char or uint8_t is less important to me.

This is my preference too.

-- 
Joel

Re: A case for `void *' for pointers to arbitrary (byte) buffers

[Mark Kettenis]

   Date: Tue, 3 May 2005 18:27:44 -0400
   From: Daniel Jacobowitz <drow@false.org>

   To be honest, my preference is still for gdb_byte * over uint8_t *, for
   exactly the reason Stan just mentioned: consistency.  Whether that's
   defined as unsigned char or uint8_t is less important to me.

So there seems to be a concensus for using `gdb_byte' for target byte
buffers.  Unless Eli has anything to add to the discussion, I'll
consider this discussion closed.

   Does gdb_byte, defined in terms of uint8_t, work for you?

This would be silly.  It's not that `uint8_t' will be anything other
than `unsigned char' on any system supported by gdb in the foreseeable
future.  Let's simply add

typedef unsigned char gdb_byte;

to "defs.h" and use that in any future changes.  Andrew, you want to
do the honour?

Once that is done, people should update the targets they maintain to
use it and get rid of `bfd_byte' ASAP.

Mark

Re: A case for `void *' for pointers to arbitrary (byte) buffers

[Andrew Cagney]

(yet again something ate my e-mail, I'm reading from the web)

Mark described the problem as:
> In many places GDB needs to pass along and pick apart blobs of memory.
> In many cases, GDB does this using `char *' pointers.  Small blobs of
> memory are often allocated on the stack as `char[]' or `unsigned
> char[]'.

This is not correct.  'void *' is more applicable to arbitrary opaque 
objects.  Here, though, we know the object's type (a byte buffer) and 
hence should use that.  Daniel correctly explains it thus (from a 
disconnected thread):

> These are byte-oriented buffers, so using a type where we can perform
> byte-oriented arithmetic without superfluous casts seems like a good
> choice to me.  Converting to void * (and not using the GCC extension
> which allows arithmetic on void *) would be a painful process.

(And as someone that actually thought to try out the two alternatives 
and determine the damage, I can tell you that void* and casts totally sux.)

Also, in support of this move away from void*, BINUTILS, several months 
ago, and in response to this rumored warning, re-vamped their types 
moving more strongly towards bfd_byte* (one consequence was that I had 
to update some of GDB's interfaces).

As for the names bfd_byte vs gdb_byte that really is arbitrary.  To 
ensure that our byte buffers are compatible with BFDs we'll want to use 
"typedef bfd_byte gdb_byte" anyway.  Given that we're already doing 
similar for LONGEST, ADDRESS, et.al., that shouldn't be contraversial.

Anyway, since the prefered name is "gdb_byte", and that was Eli's idea, 
I think it is fit and proper that Eli take the honours.   Can we get it 
done by tomorrow?   If not let me know (Hopefully I can now go back to 
fixing my GCC 4 warnings).

Andrew

Re: A case for `void *' for pointers to arbitrary (byte) buffers

[Eli Zaretskii]

> Date: Wed, 04 May 2005 11:36:56 -0400
> From: Andrew Cagney <cagney@gnu.org>
> 
> To ensure that our byte buffers are compatible with BFDs we'll want to use 
> "typedef bfd_byte gdb_byte" anyway.

Can you explain why?  I don't see why we will want that.

> Anyway, since the prefered name is "gdb_byte", and that was Eli's idea, 
> I think it is fit and proper that Eli take the honours.   Can we get it 
> done by tomorrow?

If ``we'' here means me, then no, ``we'' cannot do this by tomorrow.
I won't have enough free time until then, and on top of that, since
the FSF moved to their new offices, I have trouble even reading my
mail, let alone doing something more serious.  Sorry.

Re: A case for `void *' for pointers to arbitrary (byte) buffers

[Eli Zaretskii]

> Date: Wed, 4 May 2005 16:39:39 +0200 (CEST)
> From: Mark Kettenis <mark.kettenis@xs4all.nl>
> CC: gdb@sourceware.org, cagney@gnu.org, eliz@gnu.org
> 
> So there seems to be a concensus for using `gdb_byte' for target byte
> buffers.  Unless Eli has anything to add to the discussion, I'll
> consider this discussion closed.

I'm happy with gdb_byte; after all, it was my suggestion to begin
with.  I thought that your idea of void * was better, but if everyone
else is for gdb_byte, let's go with that.

Thanks.