Message ID | 516FF9F4.10501@codesourcery.com |
---|---|
State | New |
Headers | show |
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
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
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
> 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
> 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
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);