On Mon, 22 Apr 2013 21:36:38 +0800
Chung-Lin Tang <clt...@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
> > <clt...@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 <jul...@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
