Patchwork [4/5] Altera Nios II: dwarf generation fix

login
register
mail settings
Submitter Chung-Lin Tang
Date April 18, 2013, 1:49 p.m.
Message ID <516FF9F4.10501@codesourcery.com>
Download mbox | patch
Permalink /patch/237644/
State New
Headers show

Comments

Chung-Lin Tang - April 18, 2013, 1:49 p.m.
This patch was a fix by Julian which corrected a HOST_BITS_PER_WIDE_INT
host dependency in dwarf generation. Nios II does not have
need_64bit_hwint switched on during configuring, and ran into GDB test
FAILs originating from this problem.

2013-04-18  Julian Brown  <julian@codesourcery.com>

        * dwarf2out.c (gen_enumeration_type_die): Fix
	HOST_BITS_PER_WIDE_INT dependency behavior in enumeration type
	DIE generation.
Cary Coutant - April 18, 2013, 4:56 p.m.
On Thu, Apr 18, 2013 at 6:49 AM, Chung-Lin Tang <cltang@codesourcery.com> wrote:
> This patch was a fix by Julian which corrected a HOST_BITS_PER_WIDE_INT
> host dependency in dwarf generation. Nios II does not have
> need_64bit_hwint switched on during configuring, and ran into GDB test
> FAILs originating from this problem.
>
> 2013-04-18  Julian Brown  <julian@codesourcery.com>
>
>         * dwarf2out.c (gen_enumeration_type_die): Fix
>         HOST_BITS_PER_WIDE_INT dependency behavior in enumeration type
>         DIE generation.

+  if (host_integerp (value, TYPE_UNSIGNED (TREE_TYPE (value)))
+      && (simple_type_size_in_bits (TREE_TYPE (value))
+  <= HOST_BITS_PER_WIDE_INT || host_integerp (value, 0)))
     /* DWARF2 does not provide a way of indicating whether or
        not enumeration constants are signed or unsigned.  GDB
        always assumes the values are signed, so we output all
        values as if they were signed.  That means that
        enumeration constants with very large unsigned values
        will appear to have negative values in the debugger.  */
+    add_AT_int (enum_die, DW_AT_const_value, TREE_INT_CST_LOW (value));
+  else
+    /* Enumeration constants may be wider than HOST_WIDE_INT.  Handle
+       that here.  */
+    add_AT_double (enum_die, DW_AT_const_value,
+                   TREE_INT_CST_HIGH (value), TREE_INT_CST_LOW (value));

I'm not sure I understand the logic here. I'd think either the value
fits in a signed HOST_WIDE_INT, and we use add_AT_int, or it doesn't,
and we use add_AT_double:

  if (host_integerp (value, 0))
    add_AT_int (enum_die, DW_AT_const_value, TREE_INT_CST_LOW (value));
  else
    add_AT_double (enum_die, DW_AT_const_value,
                   TREE_INT_CST_HIGH (value), TREE_INT_CST_LOW (value));

Why wouldn't that work? I'd think this would even eliminate the need
for the comment about signed vs. unsigned.

-cary
Chung-Lin Tang - April 22, 2013, 1:36 p.m.
On 2013/4/19 12:56 AM, Cary Coutant wrote:
> On Thu, Apr 18, 2013 at 6:49 AM, Chung-Lin Tang <cltang@codesourcery.com> wrote:
>> This patch was a fix by Julian which corrected a HOST_BITS_PER_WIDE_INT
>> host dependency in dwarf generation. Nios II does not have
>> need_64bit_hwint switched on during configuring, and ran into GDB test
>> FAILs originating from this problem.
>>
>> 2013-04-18  Julian Brown  <julian@codesourcery.com>
>>
>>         * dwarf2out.c (gen_enumeration_type_die): Fix
>>         HOST_BITS_PER_WIDE_INT dependency behavior in enumeration type
>>         DIE generation.
> 
> +  if (host_integerp (value, TYPE_UNSIGNED (TREE_TYPE (value)))
> +      && (simple_type_size_in_bits (TREE_TYPE (value))
> +  <= HOST_BITS_PER_WIDE_INT || host_integerp (value, 0)))
>      /* DWARF2 does not provide a way of indicating whether or
>         not enumeration constants are signed or unsigned.  GDB
>         always assumes the values are signed, so we output all
>         values as if they were signed.  That means that
>         enumeration constants with very large unsigned values
>         will appear to have negative values in the debugger.  */
> +    add_AT_int (enum_die, DW_AT_const_value, TREE_INT_CST_LOW (value));
> +  else
> +    /* Enumeration constants may be wider than HOST_WIDE_INT.  Handle
> +       that here.  */
> +    add_AT_double (enum_die, DW_AT_const_value,
> +                   TREE_INT_CST_HIGH (value), TREE_INT_CST_LOW (value));
> 
> I'm not sure I understand the logic here. I'd think either the value
> fits in a signed HOST_WIDE_INT, and we use add_AT_int, or it doesn't,
> and we use add_AT_double:
> 
>   if (host_integerp (value, 0))
>     add_AT_int (enum_die, DW_AT_const_value, TREE_INT_CST_LOW (value));
>   else
>     add_AT_double (enum_die, DW_AT_const_value,
>                    TREE_INT_CST_HIGH (value), TREE_INT_CST_LOW (value));
> 
> Why wouldn't that work? I'd think this would even eliminate the need
> for the comment about signed vs. unsigned.
> 
> -cary

I think Julian might be able to fill clearer, but IIUC, if you use
host_integer(value,0) as the test, while functionally also correct, for
values like:

TREE_INT_CST_HIGH (value) == 00000000...
TREE_INT_CST_LOW (value)  == 1xxxxxxx...

you will end up placing it as a double, even if TREE_TYPE (value) is
something within 32-bits, which you can actually place as an 'int'. In
other words, the more complex condition saves a bit of dwarf size.

Julian, can you comment further?

Thanks,
Chung-Lin
Julian Brown - April 22, 2013, 4:11 p.m.
On Mon, 22 Apr 2013 21:36:38 +0800
Chung-Lin Tang <cltang@codesourcery.com> wrote:

> On 2013/4/19 12:56 AM, Cary Coutant wrote:
> > On Thu, Apr 18, 2013 at 6:49 AM, Chung-Lin Tang
> > <cltang@codesourcery.com> wrote:
> >> This patch was a fix by Julian which corrected a
> >> HOST_BITS_PER_WIDE_INT host dependency in dwarf generation. Nios
> >> II does not have need_64bit_hwint switched on during configuring,
> >> and ran into GDB test FAILs originating from this problem.
> >>
> >> 2013-04-18  Julian Brown  <julian@codesourcery.com>
> >>
> >>         * dwarf2out.c (gen_enumeration_type_die): Fix
> >>         HOST_BITS_PER_WIDE_INT dependency behavior in enumeration
> >> type DIE generation.
> > 
> > +  if (host_integerp (value, TYPE_UNSIGNED (TREE_TYPE (value)))
> > +      && (simple_type_size_in_bits (TREE_TYPE (value))
> > +  <= HOST_BITS_PER_WIDE_INT || host_integerp (value, 0)))
> >      /* DWARF2 does not provide a way of indicating whether or
> >         not enumeration constants are signed or unsigned.  GDB
> >         always assumes the values are signed, so we output all
> >         values as if they were signed.  That means that
> >         enumeration constants with very large unsigned values
> >         will appear to have negative values in the debugger.  */
> > +    add_AT_int (enum_die, DW_AT_const_value, TREE_INT_CST_LOW
> > (value));
> > +  else
> > +    /* Enumeration constants may be wider than HOST_WIDE_INT.
> > Handle
> > +       that here.  */
> > +    add_AT_double (enum_die, DW_AT_const_value,
> > +                   TREE_INT_CST_HIGH (value), TREE_INT_CST_LOW
> > (value));
> > 
> > I'm not sure I understand the logic here. I'd think either the value
> > fits in a signed HOST_WIDE_INT, and we use add_AT_int, or it
> > doesn't, and we use add_AT_double:
> > 
> >   if (host_integerp (value, 0))
> >     add_AT_int (enum_die, DW_AT_const_value, TREE_INT_CST_LOW
> > (value)); else
> >     add_AT_double (enum_die, DW_AT_const_value,
> >                    TREE_INT_CST_HIGH (value), TREE_INT_CST_LOW
> > (value));
> > 
> > Why wouldn't that work? I'd think this would even eliminate the need
> > for the comment about signed vs. unsigned.
> > 
> > -cary
> 
> I think Julian might be able to fill clearer, but IIUC, if you use
> host_integer(value,0) as the test, while functionally also correct,
> for values like:
> 
> TREE_INT_CST_HIGH (value) == 00000000...
> TREE_INT_CST_LOW (value)  == 1xxxxxxx...
> 
> you will end up placing it as a double, even if TREE_TYPE (value) is
> something within 32-bits, which you can actually place as an 'int'. In
> other words, the more complex condition saves a bit of dwarf size.
> 
> Julian, can you comment further?

I think that's basically right -- I'd swapped this patch out of my head
by now, so I had a go at reconstructing the logic behind it. Here's an
ASCII-art table depicting the current behaviour of this clause in the
gen_enumeration_type_die function:

A : host_integerp (value, TYPE_UNSIGNED (TREE_TYPE (value)))
B : host_integerp (value, 0)

		AB	AB	AB	AB
type_size,hwi | 00	01	10	11
--------------+-------------------------------
32,32	      | X	X	int	int
64,32	      | X	X	int	int
32,64	      | X	X	-	int
64,64	      | X	X	int	int

entries marked 'X' emit no value at all for the DIE at present. Entries
marked '-' will not occur in practice, i.e. if the type size is
32 bits, and H_W_I is 64 bits, then all values of the former can be
represented in the latter, so the predicate will never be true.

Here's a table representing the behaviour with my patch:

A : host_integerp (value, TYPE_UNSIGNED (TREE_TYPE (value)))
B : host_integerp (value, 0)

	       	AB	AB	AB	AB
type_size,hwi |	00	01	10	11
--------------+-------------------------------
32,32	      |	-	-	int	int
64,32	      |	double	double	double	int
32,64	      |	-	-	-	int
64,64	      |	-	-	int	int

(Entries marked 'int' emit an int const_value, those marked 'double'
emit a double one.)

So: the only row affected is the one where HOST_WIDE_INT is 32 bits,
but we're trying to represent a 64-bit enum type. In the particular
case where both host_integerp predicates are true (e.g. the type is
signed or unsigned, but fits within the value range of a signed type)
we can still emit it as a single int.

With Cary's proposed simplification of the patch, the table would look
like:

A : host_integerp (value, TYPE_UNSIGNED (TREE_TYPE (value)))
B : host_integerp (value, 0)

		AB	AB	AB	AB
type_size,hwi | 00	01	10	11
--------------+-------------------------------
32,32	      | -	-	double	int
64,32	      | double	int	double	int
32,64	      | -	-	-	int
64,64	      | -	-	double	int

Unfortunately I think this breaks for e.g. type_size and hwi size being
64 bits, predicate A being true and B being false (the bottom entry on
the second-to-rightmost column). Such a value might be the unsigned
quantity:

0x8000000000000000

Emitting this as a double DW_AT_const_value (i.e. 128 bits) would not
only be wasteful, but would probably actually be wrong: I wouldn't be
surprised if GDB couldn't handle that. In the case where type_size & hwi
are both 32 bits it might or might not be wrong to emit a double
const_value, but it might also confuse GDB (it's a change of semantics
for big unsigned values at least, I think).

(The second row, AB=01 entry I'm not sure about -- that might be
another '-' case in practice...)

HTH,

Julian
Cary Coutant - April 22, 2013, 5:35 p.m.
> A : host_integerp (value, TYPE_UNSIGNED (TREE_TYPE (value)))
> B : host_integerp (value, 0)
>
>                 AB      AB      AB      AB
> type_size,hwi | 00      01      10      11
> --------------+-------------------------------
> 32,32         | X       X       int     int
> 64,32         | X       X       int     int
> 32,64         | X       X       -       int
> 64,64         | X       X       int     int

In the third column (AB == 10), we're emitting a single int today even
though we know it's not technically correct: GDB will display the
unsigned value as a negative number. That's marginally better than
emitting nothing at all when the value is larger than an hwi, but I
was arguing that as long as we're adding the ability to emit the
constant as a double, why not also use a double for an unsigned that
doesn't fit in a signed hwi? Yes, it'll waste some space, but the
value will be correctly displayed as a result.

Upon further reflection, however...

This comment is wrong:

    /* DWARF2 does not provide a way of indicating whether or
       not enumeration constants are signed or unsigned.  GDB
       always assumes the values are signed, so we output all
       values as if they were signed.  That means that
       enumeration constants with very large unsigned values
       will appear to have negative values in the debugger.  */

DWARF does in fact provide a way of indicating whether a constant is
signed or unsigned: DW_FORM_sdata and DW_FORM_udata. These forms were
in DWARF-2, and the following comment was added to the DWARF-3 spec:

"If one of the DW_FORM_data<n> forms is used to represent a signed or
unsigned integer, it can be hard for a consumer to discover the
context necessary to determine which interpretation is intended.
Producers are therefore strongly encouraged to use DW_FORM_sdata or
DW_FORM_udata for signed and unsigned integers respectively, rather
than DW_FORM_data<n>."

We should really be emitting unsigned constants using add_AT_unsigned:

  if (TYPE_UNSIGNED (TREE_TYPE (value)))
    {
      if (host_integerp (value, 1))
        add_AT_unsigned (enum_die, DW_AT_const_value, TREE_INT_CST_LOW (value));
      else
        add_AT_unsigned_double (enum_die, DW_AT_const_value,
                                TREE_INT_CST_HIGH (value),
                                TREE_INT_CST_LOW (value));
    }
  else
    {
      if (host_integerp (value, 0))
        add_AT_int (enum_die, DW_AT_const_value, TREE_INT_CST_LOW (value));
      else
        add_AT_double (enum_die, DW_AT_const_value,
                       TREE_INT_CST_HIGH (value), TREE_INT_CST_LOW (value));
    }

add_AT_unsigned_double would be new, and would need a new
dw_val_class_unsigned_const_double enum.

size_of_die() and value_format() will need to be changed to force the
use of DW_FORM_udata for dw_val_class_unsigned_const and
dw_val_class_unsigned_const_double. Given that GDB always treats
DW_FORM_data{1,2,4,8} as signed, we can leave signed values as they
are, but DW_FORM_sdata could also be used in cases where it would save
space.

> (The second row, AB=01 entry I'm not sure about -- that might be
> another '-' case in practice...)

I think that whole column is a '-' case: if an unsigned value fits in
a signed hwi, then it will also fit in an unsigned hwi (i.e., B => A).

-cary
Cary Coutant - April 22, 2013, 5:50 p.m.
> We should really be emitting unsigned constants using add_AT_unsigned:
>
>   if (TYPE_UNSIGNED (TREE_TYPE (value)))
>     {
>       if (host_integerp (value, 1))
>         add_AT_unsigned (enum_die, DW_AT_const_value, TREE_INT_CST_LOW (value));
>       else
>         add_AT_unsigned_double (enum_die, DW_AT_const_value,
>                                 TREE_INT_CST_HIGH (value),
>                                 TREE_INT_CST_LOW (value));
>     }
>   else
>     {
>       if (host_integerp (value, 0))
>         add_AT_int (enum_die, DW_AT_const_value, TREE_INT_CST_LOW (value));
>       else
>         add_AT_double (enum_die, DW_AT_const_value,
>                        TREE_INT_CST_HIGH (value), TREE_INT_CST_LOW (value));
>     }
>
> add_AT_unsigned_double would be new, and would need a new
> dw_val_class_unsigned_const_double enum.
>
> size_of_die() and value_format() will need to be changed to force the
> use of DW_FORM_udata for dw_val_class_unsigned_const and
> dw_val_class_unsigned_const_double. Given that GDB always treats
> DW_FORM_data{1,2,4,8} as signed, we can leave signed values as they
> are, but DW_FORM_sdata could also be used in cases where it would save
> space.

Since this is well beyond the scope of your series of patches, I'll
take this on as a TODO for myself, and give an OK to your original
patch. (Just be aware in your toolchain that I might be changing GCC
to emit DW_FORM_udata at some point.)

-cary

Patch

Index: gcc/dwarf2out.c
===================================================================
--- gcc/dwarf2out.c	(revision 407083)
+++ gcc/dwarf2out.c	(revision 409063)
@@ -17025,15 +17025,21 @@ 
 	  if (TREE_CODE (value) == CONST_DECL)
 	    value = DECL_INITIAL (value);
 
-	  if (host_integerp (value, TYPE_UNSIGNED (TREE_TYPE (value))))
+	  if (host_integerp (value, TYPE_UNSIGNED (TREE_TYPE (value)))
+	      && (simple_type_size_in_bits (TREE_TYPE (value))
+		  <= HOST_BITS_PER_WIDE_INT || host_integerp (value, 0)))
 	    /* DWARF2 does not provide a way of indicating whether or
 	       not enumeration constants are signed or unsigned.  GDB
 	       always assumes the values are signed, so we output all
 	       values as if they were signed.  That means that
 	       enumeration constants with very large unsigned values
 	       will appear to have negative values in the debugger.  */
-	    add_AT_int (enum_die, DW_AT_const_value,
-			tree_low_cst (value, tree_int_cst_sgn (value) > 0));
+	    add_AT_int (enum_die, DW_AT_const_value, TREE_INT_CST_LOW (value));
+	  else
+	    /* Enumeration constants may be wider than HOST_WIDE_INT.  Handle
+	       that here.  */
+	    add_AT_double (enum_die, DW_AT_const_value,
+			   TREE_INT_CST_HIGH (value), TREE_INT_CST_LOW (value));
 	}
 
       add_gnat_descriptive_type_attribute (type_die, type, context_die);