Re: [PATCH 1/5] Update comment for struct type's length field, introduce type_length_units

[Pedro Alves <palves@redhat.com>]

On 07/16/2015 07:51 PM, Simon Marchi wrote:
> This patch tries to clean up a bit the blur around the length field in
> struct type, regarding its use with architectures with non-8-bits
> addressable memory.  It clarifies that the field is expressed in bytes,
> which is what is the closest to the current reality.
> 
> It also introduces a new function to get the length of the type in
> addressable memory units.
> 

LGTM, with:

> gdb/ChangeLog:
> 
> 	* gdbtypes.c (type_length_units): New function.
> 	* gdbtypes.h (type_length_units): New declaration.
> 	(struct type): Update LENGTH's comment.

Write:

	(struct type) <length>: Update comment.

> +
>  /* Alloc a new type instance structure, fill it with some defaults,
>     and point it at OLDTYPE.  Allocate the new type instance from the
>     same place as OLDTYPE.  */
> diff --git a/gdb/gdbtypes.h b/gdb/gdbtypes.h
> index c166e48..83f85a6 100644
> --- a/gdb/gdbtypes.h
> +++ b/gdb/gdbtypes.h
> @@ -780,31 +780,23 @@ struct type
>       check_typedef.  */
>    int instance_flags;
>  
> -  /* * Length of storage for a value of this type.  This is what
> -     sizeof(type) would return; use it for address arithmetic, memory
> -     reads and writes, etc.  This size includes padding.  For example,
> -     an i386 extended-precision floating point value really only
> -     occupies ten bytes, but most ABI's declare its size to be 12
> -     bytes, to preserve alignment.  A `struct type' representing such
> -     a floating-point type would have a `length' value of 12, even
> -     though the last two bytes are unused.
> -
> -     There's a bit of a host/target mess here, if you're concerned
> -     about machines whose bytes aren't eight bits long, or who don't
> -     have byte-addressed memory.  Various places pass this to memcpy
> -     and such, meaning it must be in units of host bytes.  Various
> -     other places expect they can calculate addresses by adding it
> -     and such, meaning it must be in units of target bytes.  For
> -     some DSP targets, in which HOST_CHAR_BIT will (presumably) be 8
> -     and TARGET_CHAR_BIT will be (say) 32, this is a problem.
> -
> -     One fix would be to make this field in bits (requiring that it
> -     always be a multiple of HOST_CHAR_BIT and TARGET_CHAR_BIT) ---
> -     the other choice would be to make it consistently in units of
> -     HOST_CHAR_BIT.  However, this would still fail to address
> -     machines based on a ternary or decimal representation.  */
> +  /* * Length of storage for a value of this type.  The value is the
> +     expression in bytes of of what sizeof(type) would return.  This

Double "of of".  Please say "host bytes" to make this super clear.

> +     size includes padding.  For example, an i386 extended-precision
> +     floating point value really only occupies ten bytes, but most
> +     ABI's declare its size to be 12 bytes, to preserve alignment.
> +     A `struct type' representing such a floating-point type would
> +     have a `length' value of 12, even though the last two bytes are
> +     unused.
> +
> +     Since this field is expressed in bytes, its value is appropriate to

Likewise, "host bytes".

> +     pass to memcpy and such (it is assumed that GDB itself always runs
> +     on an 8-bits addressable architecture).  However, when using it for
> +     target address arithmetic (e.g. adding it to a target address), the
> +     type_length_units function should be used in order to get the length
> +     expressed in addressable memory units.  */

"target addressable memory units" while at it.

Likewise in the other patches.

>    
> -  unsigned length;
> +  unsigned int length;

Thanks,
Pedro Alves