[RFC][02/19] Target FP: Simplify floatformat_from_type

[RFC][02/19] Target FP: Simplify floatformat_from_type

[Ulrich Weigand]

[RFC][02/19] Target FP: Simplify floatformat_from_type

For historical reasons, the TYPE_FLOATFORMAT element is still set to hold
an array of two floatformat structs, one for big-endian and the other for
little-endian.  When accessing the element via floatformat_from_type,
the code would check the type's byte order and return the appropriate
floatformat.

However, these days this is quite unnecessary, since the type's byte order
is already known at the time the type is allocated and the floatformat is
installed into TYPE_FLOATFORMAT.  Therefore, we can just install the correct
version here.

Also, moved the (now trivially simple) floatformat_from_type accessor to
gdbtypes.{c,h}, since it doesn't really need to be in doublest.c now.

Bye,
Ulrich

ChangeLog:

	* doublest.h (floatformat_from_type): Move to gdbtypes.h.
	* doublest.c (floatformat_from_type): Move to gdbtypes.c.

	* gdbtypes.h (union type_specific): Make field floatformat hold
	just a single struct floatformat, not an array.
	(floatformat_from_type): Move here.
	* gdbtypes.c (floatformat_from_type): Move here.  Update to
	changed TYPE_FLOATFORMAT definition.
	(verify_floatformat): Update to changed TYPE_FLOATFORMAT.
	(recursive_dump_type): Likewise.
	(init_float_type): Install correct floatformat for byte order.
	(arch_float_type): Likewise.


Index: binutils-gdb/gdb/doublest.c
===================================================================
--- binutils-gdb.orig/gdb/doublest.c
+++ binutils-gdb/gdb/doublest.c
@@ -770,22 +770,6 @@ floatformat_from_doublest (const struct
 }
 
 \f
-/* Return the floating-point format for a floating-point variable of
-   type TYPE.  */
-
-const struct floatformat *
-floatformat_from_type (const struct type *type)
-{
-  struct gdbarch *gdbarch = get_type_arch (type);
-  const struct floatformat *fmt;
-
-  gdb_assert (TYPE_CODE (type) == TYPE_CODE_FLT);
-  gdb_assert (TYPE_FLOATFORMAT (type));
-  fmt = TYPE_FLOATFORMAT (type)[gdbarch_byte_order (gdbarch)];
-  gdb_assert (TYPE_LENGTH (type) >= floatformat_totalsize_bytes (fmt));
-  return fmt;
-}
-
 /* Extract a floating-point number of type TYPE from a target-order
    byte-stream at ADDR.  Returns the value as type DOUBLEST.  */
 
Index: binutils-gdb/gdb/doublest.h
===================================================================
--- binutils-gdb.orig/gdb/doublest.h
+++ binutils-gdb/gdb/doublest.h
@@ -71,12 +71,6 @@ extern enum float_kind floatformat_class
 extern const char *floatformat_mantissa (const struct floatformat *,
 					 const bfd_byte *);
 
-/* Given TYPE, return its floatformat.  TYPE_FLOATFORMAT() may return
-   NULL.  type_floatformat() detects that and returns a floatformat
-   based on the type size when FLOATFORMAT is NULL.  */
-
-const struct floatformat *floatformat_from_type (const struct type *type);
-
 /* Return the floatformat's total size in host bytes.  */
 
 extern size_t floatformat_totalsize_bytes (const struct floatformat *fmt);
Index: binutils-gdb/gdb/gdbtypes.c
===================================================================
--- binutils-gdb.orig/gdb/gdbtypes.c
+++ binutils-gdb/gdb/gdbtypes.c
@@ -2735,22 +2735,31 @@ set_type_code (struct type *type, enum t
    determined by the floatformat.  Returns size to be used.  */
 
 static int
-verify_floatformat (int bit, const struct floatformat **floatformats)
+verify_floatformat (int bit, const struct floatformat *floatformat)
 {
-  gdb_assert (floatformats != NULL);
-  gdb_assert (floatformats[0] != NULL && floatformats[1] != NULL);
+  gdb_assert (floatformat != NULL);
 
   if (bit == -1)
-    bit = floatformats[0]->totalsize;
+    bit = floatformat->totalsize;
   gdb_assert (bit >= 0);
 
   size_t len = bit / TARGET_CHAR_BIT;
-  gdb_assert (len >= floatformat_totalsize_bytes (floatformats[0]));
-  gdb_assert (len >= floatformat_totalsize_bytes (floatformats[1]));
+  gdb_assert (len * TARGET_CHAR_BIT >= floatformat->totalsize);
 
   return bit;
 }
 
+/* Return the floating-point format for a floating-point variable of
+   type TYPE.  */
+
+const struct floatformat *
+floatformat_from_type (const struct type *type)
+{
+  gdb_assert (TYPE_CODE (type) == TYPE_CODE_FLT);
+  gdb_assert (TYPE_FLOATFORMAT (type));
+  return TYPE_FLOATFORMAT (type);
+}
+
 /* Helper function to initialize the standard scalar types.
 
    If NAME is non-NULL, then it is used to initialize the type name.
@@ -2842,11 +2851,13 @@ init_float_type (struct objfile *objfile
 		 int bit, const char *name,
 		 const struct floatformat **floatformats)
 {
+  struct gdbarch *gdbarch = get_objfile_arch (objfile);
+  const struct floatformat *fmt = floatformats[gdbarch_byte_order (gdbarch)];
   struct type *t;
 
-  bit = verify_floatformat (bit, floatformats);
+  bit = verify_floatformat (bit, fmt);
   t = init_type (objfile, TYPE_CODE_FLT, bit / TARGET_CHAR_BIT, name);
-  TYPE_FLOATFORMAT (t) = floatformats;
+  TYPE_FLOATFORMAT (t) = fmt;
 
   return t;
 }
@@ -4544,26 +4555,11 @@ recursive_dump_type (struct type *type,
 
       case TYPE_SPECIFIC_FLOATFORMAT:
 	printfi_filtered (spaces, "floatformat ");
-	if (TYPE_FLOATFORMAT (type) == NULL)
+	if (TYPE_FLOATFORMAT (type) == NULL
+	    || TYPE_FLOATFORMAT (type)->name == NULL)
 	  puts_filtered ("(null)");
 	else
-	  {
-	    puts_filtered ("{ ");
-	    if (TYPE_FLOATFORMAT (type)[0] == NULL
-		|| TYPE_FLOATFORMAT (type)[0]->name == NULL)
-	      puts_filtered ("(null)");
-	    else
-	      puts_filtered (TYPE_FLOATFORMAT (type)[0]->name);
-
-	    puts_filtered (", ");
-	    if (TYPE_FLOATFORMAT (type)[1] == NULL
-		|| TYPE_FLOATFORMAT (type)[1]->name == NULL)
-	      puts_filtered ("(null)");
-	    else
-	      puts_filtered (TYPE_FLOATFORMAT (type)[1]->name);
-
-	    puts_filtered (" }");
-	  }
+	  puts_filtered (TYPE_FLOATFORMAT (type)->name);
 	puts_filtered ("\n");
 	break;
 
@@ -4907,11 +4903,12 @@ arch_float_type (struct gdbarch *gdbarch
 		 int bit, const char *name,
 		 const struct floatformat **floatformats)
 {
+  const struct floatformat *fmt = floatformats[gdbarch_byte_order (gdbarch)];
   struct type *t;
 
-  bit = verify_floatformat (bit, floatformats);
+  bit = verify_floatformat (bit, fmt);
   t = arch_type (gdbarch, TYPE_CODE_FLT, bit / TARGET_CHAR_BIT, name);
-  TYPE_FLOATFORMAT (t) = floatformats;
+  TYPE_FLOATFORMAT (t) = fmt;
 
   return t;
 }
Index: binutils-gdb/gdb/gdbtypes.h
===================================================================
--- binutils-gdb.orig/gdb/gdbtypes.h
+++ binutils-gdb/gdb/gdbtypes.h
@@ -571,12 +571,11 @@ union type_specific
 
   struct gnat_aux_type *gnat_stuff;
 
-  /* * FLOATFORMAT is for TYPE_CODE_FLT.  It is a pointer to two
-     floatformat objects that describe the floating-point value
-     that resides within the type.  The first is for big endian
-     targets and the second is for little endian targets.  */
+  /* * FLOATFORMAT is for TYPE_CODE_FLT.  It is a pointer to a
+     floatformat object that describes the floating-point value
+     that resides within the type.  */
 
-  const struct floatformat **floatformat;
+  const struct floatformat *floatformat;
 
   /* * For TYPE_CODE_FUNC and TYPE_CODE_METHOD types.  */
 
@@ -1434,6 +1433,9 @@ extern void set_type_vptr_basetype (stru
 #define TYPE_ERROR_NAME(type) \
   (TYPE_NAME (type) ? TYPE_NAME (type) : _("<error type>"))
 
+/* Given TYPE, return its floatformat.  */
+const struct floatformat *floatformat_from_type (const struct type *type);
+
 struct builtin_type
 {
   /* Integral types.  */

Re: [RFC][02/19] Target FP: Simplify floatformat_from_type

[Simon Marchi]

On 2017-09-05 20:20, Ulrich Weigand wrote:
> Index: binutils-gdb/gdb/gdbtypes.c
> ===================================================================
> --- binutils-gdb.orig/gdb/gdbtypes.c
> +++ binutils-gdb/gdb/gdbtypes.c
> @@ -2735,22 +2735,31 @@ set_type_code (struct type *type, enum t
>     determined by the floatformat.  Returns size to be used.  */
> 
>  static int
> -verify_floatformat (int bit, const struct floatformat **floatformats)
> +verify_floatformat (int bit, const struct floatformat *floatformat)
>  {
> -  gdb_assert (floatformats != NULL);
> -  gdb_assert (floatformats[0] != NULL && floatformats[1] != NULL);
> +  gdb_assert (floatformat != NULL);
> 
>    if (bit == -1)
> -    bit = floatformats[0]->totalsize;
> +    bit = floatformat->totalsize;
>    gdb_assert (bit >= 0);
> 
>    size_t len = bit / TARGET_CHAR_BIT;
> -  gdb_assert (len >= floatformat_totalsize_bytes (floatformats[0]));
> -  gdb_assert (len >= floatformat_totalsize_bytes (floatformats[1]));
> +  gdb_assert (len * TARGET_CHAR_BIT >= floatformat->totalsize);

That looks funny now.  Is there a reason not to do

   gdb_assert (bit >= floatformat->totalsize);

directly?

Simon

Re: [RFC][02/19] Target FP: Simplify floatformat_from_type

[Ulrich Weigand]

Simon Marchi wrote:

> On 2017-09-05 20:20, Ulrich Weigand wrote:
> >    size_t len = bit / TARGET_CHAR_BIT;
> > -  gdb_assert (len >= floatformat_totalsize_bytes (floatformats[0]));
> > -  gdb_assert (len >= floatformat_totalsize_bytes (floatformats[1]));
> > +  gdb_assert (len * TARGET_CHAR_BIT >= floatformat->totalsize);
> 
> That looks funny now.  Is there a reason not to do
> 
>    gdb_assert (bit >= floatformat->totalsize);
> 
> directly?

Well, that would use different rounding ...   Of course, this only
matters when floatformat->totalsize is not a multiple of TARGET_CHAR_BIT,
which doesn't happen for any of the floatformats GDB supports, so it
is probably moot.  Maybe we should instead use an assert to verify that
floatformat->totalsize is in fact a multiple of TARGET_CHAR_BIT?

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU/Linux compilers and toolchain
  Ulrich.Weigand@de.ibm.com

Re: [RFC][02/19] Target FP: Simplify floatformat_from_type

[Simon Marchi]

On 2017-09-18 13:49, Ulrich Weigand wrote:
> Simon Marchi wrote:
> 
>> On 2017-09-05 20:20, Ulrich Weigand wrote:
>> >    size_t len = bit / TARGET_CHAR_BIT;
>> > -  gdb_assert (len >= floatformat_totalsize_bytes (floatformats[0]));
>> > -  gdb_assert (len >= floatformat_totalsize_bytes (floatformats[1]));
>> > +  gdb_assert (len * TARGET_CHAR_BIT >= floatformat->totalsize);
>> 
>> That looks funny now.  Is there a reason not to do
>> 
>>    gdb_assert (bit >= floatformat->totalsize);
>> 
>> directly?
> 
> Well, that would use different rounding ...   Of course, this only
> matters when floatformat->totalsize is not a multiple of 
> TARGET_CHAR_BIT,
> which doesn't happen for any of the floatformats GDB supports, so it
> is probably moot. Maybe we should instead use an assert to verify that
> floatformat->totalsize is in fact a multiple of TARGET_CHAR_BIT?

I indeed think we should add an assert that the type size in bits is a 
multiple of TARGET_CHAR_BIT.  But it should not be in 
verify_floatformat, because it's not specific to float.  In most calls 
to arch_type/init_type, we have that division that rounds down:

   t = init_type (objfile, TYPE_CODE_FLT, bit / TARGET_CHAR_BIT, name);

Let's say we try to add an integer with bit == 24 and TARGET_CHAR_BIT == 
16, we'll create a type with a size of 1 target byte, and things will 
likely break down the line.  Since this is where we do the division by 
TARGET_CHAR_BIT, and therefore assume (implicitly) that bit is a 
multiple of TARGET_CHAR_BIT, I think this is where the assert should be 
added.  To avoid adding them everywhere, we could make 
arch_type/init_type take a size in bits, and do the division and assert 
there.

As long as verify_floatformat is concerned, its job is to verify (IIUC) 
that the type you want to create with BIT bits has enough room to hold a 
float of that kind.  It doesn't have to know that we'll later divide 
that value by TARGET_CHAR_BIT.  So here I think asserting "bit >= 
floatformat->totalsize)" is fine.

I don't think floatformat->totalsize has any requirement to be a 
multiple of TARGET_CHAR_BIT.  For example, my hardware could have a 
float of 29 bits, but when stored in memory it uses 32 bits (just like 
on x86 long double is an 80 bit float stored in a 96 bit data type).  
Then, bit would be 32 and floatformat->totalsize would be 29.

Simon

Re: [RFC][02/19] Target FP: Simplify floatformat_from_type

[Ulrich Weigand]

Simon Marchi wrote:

> Since this is where we do the division by 
> TARGET_CHAR_BIT, and therefore assume (implicitly) that bit is a 
> multiple of TARGET_CHAR_BIT, I think this is where the assert should be 
> added.  To avoid adding them everywhere, we could make 
> arch_type/init_type take a size in bits, and do the division and assert 
> there.

I agree, this seems the best solution.  And it makes more sense for all
the type-creation interfaces to use size in bits instead of the
mixture we have now.  I'll come up with a patch to do that.

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU/Linux compilers and toolchain
  Ulrich.Weigand@de.ibm.com

Re: [RFC][02/19] Target FP: Simplify floatformat_from_type

[Ulrich Weigand]

Simon Marchi wrote:

> Since this is where we do the division by 
> TARGET_CHAR_BIT, and therefore assume (implicitly) that bit is a 
> multiple of TARGET_CHAR_BIT, I think this is where the assert should be 
> added.  To avoid adding them everywhere, we could make 
> arch_type/init_type take a size in bits, and do the division and assert 
> there.

And here's the updated floatformat_from_type patch, assuming the
above change is in.

Bye,
Ulrich

ChangeLog:

	* doublest.h (floatformat_from_type): Move to gdbtypes.h.
	* doublest.c (floatformat_from_type): Move to gdbtypes.c.

	* gdbtypes.h (union type_specific): Make field floatformat hold
	just a single struct floatformat, not an array.
	(floatformat_from_type): Move here.
	* gdbtypes.c (floatformat_from_type): Move here.  Update to
	changed TYPE_FLOATFORMAT definition.
	(verify_floatformat): Update to changed TYPE_FLOATFORMAT.
	(recursive_dump_type): Likewise.
	(init_float_type): Install correct floatformat for byte order.
	(arch_float_type): Likewise.


Index: binutils-gdb/gdb/doublest.c
===================================================================
--- binutils-gdb.orig/gdb/doublest.c
+++ binutils-gdb/gdb/doublest.c
@@ -770,22 +770,6 @@ floatformat_from_doublest (const struct
 }
 
 \f
-/* Return the floating-point format for a floating-point variable of
-   type TYPE.  */
-
-const struct floatformat *
-floatformat_from_type (const struct type *type)
-{
-  struct gdbarch *gdbarch = get_type_arch (type);
-  const struct floatformat *fmt;
-
-  gdb_assert (TYPE_CODE (type) == TYPE_CODE_FLT);
-  gdb_assert (TYPE_FLOATFORMAT (type));
-  fmt = TYPE_FLOATFORMAT (type)[gdbarch_byte_order (gdbarch)];
-  gdb_assert (TYPE_LENGTH (type) >= floatformat_totalsize_bytes (fmt));
-  return fmt;
-}
-
 /* Extract a floating-point number of type TYPE from a target-order
    byte-stream at ADDR.  Returns the value as type DOUBLEST.  */
 
Index: binutils-gdb/gdb/doublest.h
===================================================================
--- binutils-gdb.orig/gdb/doublest.h
+++ binutils-gdb/gdb/doublest.h
@@ -71,12 +71,6 @@ extern enum float_kind floatformat_class
 extern const char *floatformat_mantissa (const struct floatformat *,
 					 const bfd_byte *);
 
-/* Given TYPE, return its floatformat.  TYPE_FLOATFORMAT() may return
-   NULL.  type_floatformat() detects that and returns a floatformat
-   based on the type size when FLOATFORMAT is NULL.  */
-
-const struct floatformat *floatformat_from_type (const struct type *type);
-
 /* Return the floatformat's total size in host bytes.  */
 
 extern size_t floatformat_totalsize_bytes (const struct floatformat *fmt);
Index: binutils-gdb/gdb/gdbtypes.c
===================================================================
--- binutils-gdb.orig/gdb/gdbtypes.c
+++ binutils-gdb/gdb/gdbtypes.c
@@ -2735,22 +2735,30 @@ set_type_code (struct type *type, enum t
    determined by the floatformat.  Returns size to be used.  */
 
 static int
-verify_floatformat (int bit, const struct floatformat **floatformats)
+verify_floatformat (int bit, const struct floatformat *floatformat)
 {
-  gdb_assert (floatformats != NULL);
-  gdb_assert (floatformats[0] != NULL && floatformats[1] != NULL);
+  gdb_assert (floatformat != NULL);
 
   if (bit == -1)
-    bit = floatformats[0]->totalsize;
-  gdb_assert (bit >= 0);
+    bit = floatformat->totalsize;
 
-  size_t len = bit / TARGET_CHAR_BIT;
-  gdb_assert (len >= floatformat_totalsize_bytes (floatformats[0]));
-  gdb_assert (len >= floatformat_totalsize_bytes (floatformats[1]));
+  gdb_assert (bit >= 0);
+  gdb_assert (bit >= floatformat->totalsize);
 
   return bit;
 }
 
+/* Return the floating-point format for a floating-point variable of
+   type TYPE.  */
+
+const struct floatformat *
+floatformat_from_type (const struct type *type)
+{
+  gdb_assert (TYPE_CODE (type) == TYPE_CODE_FLT);
+  gdb_assert (TYPE_FLOATFORMAT (type));
+  return TYPE_FLOATFORMAT (type);
+}
+
 /* Helper function to initialize the standard scalar types.
 
    If NAME is non-NULL, then it is used to initialize the type name.
@@ -2843,11 +2851,13 @@ init_float_type (struct objfile *objfile
 		 int bit, const char *name,
 		 const struct floatformat **floatformats)
 {
+  struct gdbarch *gdbarch = get_objfile_arch (objfile);
+  const struct floatformat *fmt = floatformats[gdbarch_byte_order (gdbarch)];
   struct type *t;
 
-  bit = verify_floatformat (bit, floatformats);
+  bit = verify_floatformat (bit, fmt);
   t = init_type (objfile, TYPE_CODE_FLT, bit, name);
-  TYPE_FLOATFORMAT (t) = floatformats;
+  TYPE_FLOATFORMAT (t) = fmt;
 
   return t;
 }
@@ -4545,26 +4555,11 @@ recursive_dump_type (struct type *type,
 
       case TYPE_SPECIFIC_FLOATFORMAT:
 	printfi_filtered (spaces, "floatformat ");
-	if (TYPE_FLOATFORMAT (type) == NULL)
+	if (TYPE_FLOATFORMAT (type) == NULL
+	    || TYPE_FLOATFORMAT (type)->name == NULL)
 	  puts_filtered ("(null)");
 	else
-	  {
-	    puts_filtered ("{ ");
-	    if (TYPE_FLOATFORMAT (type)[0] == NULL
-		|| TYPE_FLOATFORMAT (type)[0]->name == NULL)
-	      puts_filtered ("(null)");
-	    else
-	      puts_filtered (TYPE_FLOATFORMAT (type)[0]->name);
-
-	    puts_filtered (", ");
-	    if (TYPE_FLOATFORMAT (type)[1] == NULL
-		|| TYPE_FLOATFORMAT (type)[1]->name == NULL)
-	      puts_filtered ("(null)");
-	    else
-	      puts_filtered (TYPE_FLOATFORMAT (type)[1]->name);
-
-	    puts_filtered (" }");
-	  }
+	  puts_filtered (TYPE_FLOATFORMAT (type)->name);
 	puts_filtered ("\n");
 	break;
 
@@ -4909,11 +4904,12 @@ arch_float_type (struct gdbarch *gdbarch
 		 int bit, const char *name,
 		 const struct floatformat **floatformats)
 {
+  const struct floatformat *fmt = floatformats[gdbarch_byte_order (gdbarch)];
   struct type *t;
 
-  bit = verify_floatformat (bit, floatformats);
+  bit = verify_floatformat (bit, fmt);
   t = arch_type (gdbarch, TYPE_CODE_FLT, bit, name);
-  TYPE_FLOATFORMAT (t) = floatformats;
+  TYPE_FLOATFORMAT (t) = fmt;
 
   return t;
 }
Index: binutils-gdb/gdb/gdbtypes.h
===================================================================
--- binutils-gdb.orig/gdb/gdbtypes.h
+++ binutils-gdb/gdb/gdbtypes.h
@@ -571,12 +571,11 @@ union type_specific
 
   struct gnat_aux_type *gnat_stuff;
 
-  /* * FLOATFORMAT is for TYPE_CODE_FLT.  It is a pointer to two
-     floatformat objects that describe the floating-point value
-     that resides within the type.  The first is for big endian
-     targets and the second is for little endian targets.  */
+  /* * FLOATFORMAT is for TYPE_CODE_FLT.  It is a pointer to a
+     floatformat object that describes the floating-point value
+     that resides within the type.  */
 
-  const struct floatformat **floatformat;
+  const struct floatformat *floatformat;
 
   /* * For TYPE_CODE_FUNC and TYPE_CODE_METHOD types.  */
 
@@ -1434,6 +1433,9 @@ extern void set_type_vptr_basetype (stru
 #define TYPE_ERROR_NAME(type) \
   (TYPE_NAME (type) ? TYPE_NAME (type) : _("<error type>"))
 
+/* Given TYPE, return its floatformat.  */
+const struct floatformat *floatformat_from_type (const struct type *type);
+
 struct builtin_type
 {
   /* Integral types.  */

-- 
  Dr. Ulrich Weigand
  GNU/Linux compilers and toolchain
  Ulrich.Weigand@de.ibm.com

Re: [RFC][02/19] Target FP: Simplify floatformat_from_type

[Ulrich Weigand]

> ChangeLog:
> 
> 	* doublest.h (floatformat_from_type): Move to gdbtypes.h.
> 	* doublest.c (floatformat_from_type): Move to gdbtypes.c.
> 
> 	* gdbtypes.h (union type_specific): Make field floatformat hold
> 	just a single struct floatformat, not an array.
> 	(floatformat_from_type): Move here.
> 	* gdbtypes.c (floatformat_from_type): Move here.  Update to
> 	changed TYPE_FLOATFORMAT definition.
> 	(verify_floatformat): Update to changed TYPE_FLOATFORMAT.
> 	(recursive_dump_type): Likewise.
> 	(init_float_type): Install correct floatformat for byte order.
> 	(arch_float_type): Likewise.

I've pushed this now.

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU/Linux compilers and toolchain
  Ulrich.Weigand@de.ibm.com