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

[Simon Marchi <simon.marchi@ericsson.com>]

On 15-07-24 07:26 AM, Pedro Alves wrote:
> 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

Thanks for the review.  Here is the updated version:


From bb292c235125038795185b325222dcd54bb9f36a Mon Sep 17 00:00:00 2001
From: Simon Marchi <simon.marchi@ericsson.com>
Date: Mon, 27 Jul 2015 17:17:49 -0400
Subject: [PATCH] Update comment for struct type's length field, introduce
 type_length_units

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 host
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
target addressable memory units.

gdb/ChangeLog:

	* gdbtypes.c (type_length_units): New function.
	* gdbtypes.h (type_length_units): New declaration.
	(struct type) <length>: Update comment.
---
 gdb/gdbtypes.c | 11 +++++++++++
 gdb/gdbtypes.h | 47 ++++++++++++++++++++++-------------------------
 2 files changed, 33 insertions(+), 25 deletions(-)

diff --git a/gdb/gdbtypes.c b/gdb/gdbtypes.c
index 3f1f3fb..e6a9547 100644
--- a/gdb/gdbtypes.c
+++ b/gdb/gdbtypes.c
@@ -252,6 +252,17 @@ get_target_type (struct type *type)
   return type;
 }

+/* See gdbtypes.h.  */
+
+unsigned int
+type_length_units (struct type *type)
+{
+  struct gdbarch *arch = get_type_arch (type);
+  int unit_size = gdbarch_addressable_memory_unit_size (arch);
+
+  return TYPE_LENGTH (type) / unit_size;
+}
+
 /* 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..f270855 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.  */
-
-  unsigned length;
+  /* * Length of storage for a value of this type.  The value is the
+     expression in host bytes of what sizeof(type) would return.  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.
+
+     Since this field is expressed in host bytes, its value is appropriate
+     to 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 target addressable memory units.  */
+
+  unsigned int length;

   /* * Core type, shared by a group of qualified types.  */

@@ -1659,6 +1651,11 @@ extern struct gdbarch *get_type_arch (const struct type *);

 extern struct type *get_target_type (struct type *type);

+/* Return the equivalent of TYPE_LENGTH, but in number of target
+   addressable memory units of the associated gdbarch instead of bytes.  */
+
+extern unsigned int type_length_units (struct type *type);
+
 /* * Helper function to construct objfile-owned types.  */

 extern struct type *init_type (enum type_code, int, int, const char *,
-- 
2.1.4