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

login
register
mail settings
Submitter Chung-Lin Tang
Date April 23, 2013, 10:15 a.m.
Message ID <51765F42.3070808@codesourcery.com>
Download mbox | patch
Permalink /patch/238852/
State New
Headers show

Comments

Chung-Lin Tang - April 23, 2013, 10:15 a.m.
On 2013/4/23 01:35 AM, Cary Coutant wrote:
>> 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.

I'm not sure of other cases, but here it is only to mark the values of
an enumerator type, so as long as the values are consistent, the correct
behavior is to print out the right enum string (I know that's a bit not
ideal, but just to point that out)

> 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.

That seems like the completely correct solution; correct unsigned/signed
x int/double tags for all situations. 0000...1xxx... can then be
correctly read as an unsigned int, rather than an excessively wide
double or (incorrectly) signed int.

You said OK for Julian's patch in the last mail, so I'll take that as
approved (for an interim solution). If you don't mind, I'll add a TODO
to the comments (attached patch).

Thanks,
Chung-Lin
Cary Coutant - April 23, 2013, 5:49 p.m.
> You said OK for Julian's patch in the last mail, so I'll take that as
> approved (for an interim solution). If you don't mind, I'll add a TODO
> to the comments (attached patch).

Looks good, thanks!

-cary

Patch

Index: dwarf2out.c
===================================================================
--- dwarf2out.c	(revision 198177)
+++ dwarf2out.c	(working copy)
@@ -17027,15 +17027,27 @@  gen_enumeration_type_die (tree type, dw_die_ref co
 	  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));
+	       will appear to have negative values in the debugger.
+
+	       TODO: the above comment is wrong, DWARF2 does provide
+	       DW_FORM_sdata/DW_FORM_udata to represent signed/unsigned data.
+	       This should be re-worked to use correct signed/unsigned
+	       int/double tags for all cases, instead of always treating as
+	       signed.  */
+	    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);