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

[Julian Brown]

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