RE: [PATCH ver 2] PowerPC: fix _Float128 type output string

[Carl Love <cel@us.ibm.com>]

On Thu, 2023-04-13 at 08:18 -0600, Tom Tromey wrote:
> > > > > > "Carl" == Carl Love via Gdb-patches <
> > > > > > gdb-patches@sourceware.org> writes:
> 
> Carl> PowerPC supports two 128-bit floating point formats, the IBM
> long double
> Carl> and IEEE 128-bit float.  The issue is the DWARF information
> does not
> Carl> distinguish between the two.  There have been proposals of how
> to extend
> Carl> the DWARF information as discussed in
> Carl> 
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=104194
>  
> Carl> but has not been fully implemented.
> 
> Could it be?  I didn't read the issue but it's often better to put in
> the effort to fix the problem in the compiler.  Normally once these
> workarounds go into gdb, they can never be removed.

Yup, that is the problem right now with GCC.  Even if we could get it
fixed in GCC today, the GCC hack is already in released distro
versions.  GDB will have to deal with it for sometime after it gets
fixed in GCC.  This issue was addressed in the GCC community several
years ago from my understanding.  Bugs have been filed with regards to
the fact that the GCC hack is not transparent to GDB, but there doesn't
seem to be much traction to address the issue yet.
> 
> Carl> This patch fixes 74 regression test failures in
> Carl> gdb.base/whatis-ptype-typedefs.exp on PowerPC with IEEE float
> 128 as the
> Carl> default on GCC.  It fixes one regression test failure in
> Carl> gdb.base/complex-parts.exp.
> 
> I don't really understand how this patch works.
> 
> It took me a while to understand that maybe the issue is that the
> association between a gdb type and a float format is done by name and
> size, and because this is a typedef, the association is done instead
> by
> the underlying type -- which is then wrong.
> 
> However, doesn't copy_type also copy the TYPE_FLOATFORMAT field?
> So where would this get reset?
> 
> Or maybe this isn't the problem at all, but then I don't understand
> what
> it would be.

The following is from the private discussion I had with Ulrich while
developing the patch.  Perhaps it will help,

the GCC-generated dwarf info including the hack provides
the following two type records:

1) name: _Float128
   type: base floating-point type (TYPE_CODE_FLT)
     size: 16
     format: floatformats_ieee_quad

and

2) name: "long double"
   type: typedef (TYPE_CODE_TYPEDEF)
     target-type: _Float128 (i.e. type 1) above)

What the patch does is to keep 1) as-is, and replace 2) by

2') name: "long double"
    type: base floating-point type (TYPE_CODE_FLT)
      size: 16
      format: floatformats_ieee_quad

where the name is taken from 2), and the rest of the
record is taken from 1). 

- what the actual problem is, namely the presence of a record like
  2) above -which really cannot happen normally as "long double"
  is defined to be a base type according to the C language, so it
  can normally never be a typedef;
- and what the patch does, namely replacing that weird typedef
  with a "normal" type definition for "long double", where the
  actual type information is taken/copied from target type of
  the weird typedef.


> 
> Carl> +# The DWARF info currently does not distinquish between IEEE
> 128-bit floating
> Carl> +# point values and the IBM 128-bit floating point format.  GCC
> has an internal
> Carl> +# hack that uses the _Float128 base typdef for IEEE 128-bit
> float values.  The
> Carl> +# following method is used to "fix" the long double typedef so
> the _Float128
> Carl> +# name is not printed.
> Carl> +Function(
> Carl> +    comment="""
> Carl> +Return true if the typedef record needs to be replaced.".
> Carl> +
> Carl> +Return 0 by default""",
> 
> I think the comment field could be reworded to be more clear.
> I guess the idea is that some typedefs are replaced by their target
> type, but given the typedef's name.

The patch only replaces the typedef with information from the "target
type", which is a base type, when GCC has added the typedef to identify
the IEEE 128-bit floating point value.  That is the GCC hack so we can
identify an IEEE 128-bit value since there is no way to specify that in
the DWARY ifno at this time.

I will see if I can make that clearer in the comment. 
> 
> Carl> +bool
> Carl> +linux_dwarf2_omit_typedef_p (struct type *target_type,
> Carl> +			     const char *producer, const char
> *name)
> 
> I think this can be 'static'.

OK, I will take a look at that change.

Thanks for the input.

                           Carl