Re: [lto] revised PATCH: Add DIE readers for array and enum types

[Mark Mitchell <mark at codesourcery dot com>]

Sandra Loosemore wrote:
> Hi Mark,
>  > I've addressed the concerns Mark pointed out about the handling of 
> signed vs unsigned numbers by distinguishing them in the 
> DWARF2_form_data struct and introducing new helper functions to convert 
> the data in that structure to a host int (mainly used for parsing 
> various DWARF flags) or target integer constant of a specified type.
>
> The new feature rolled into this patch that wasn't in the initial 
> version is that I added stuff to cause DWARF information to be emitted 
> for global variables, which has helped me to test things a little better 
> this time around.
>
> OK to commit yet?

> *************** lto_read_form (lto_info_fd *info_fd, 
> *** 797,802 ****
> --- 849,860 ----
>   	  case DW_FORM_data8:
>   	    data = lto_read_udword (fd);
>   	    break;
> + 	  case DW_FORM_sdata:
> + 	    sdata = lto_read_sleb128 (fd);
> + 	    break;
> + 	  case DW_FORM_udata:
> + 	    data = lto_read_sleb128 (fd);
> + 	    break;

Shouldn't that be uleb128 for the udata case?

> + /* Convert the attribute data to an int.  */
> + static int
> + attribute_value_as_int (DWARF2_form_data *attr_data)
> + {
> +   switch (attr_data->cl)
> +     {
> +     case DW_cl_uconstant:
> +       return lto_check_int_val (attr_data->u.uconstant,
> + 				"unsigned value cannot be converted to int");
> +     case DW_cl_sconstant:
> +       if (attr_data->u.sconstant <= INT_MAX
> + 	  && attr_data->u.sconstant >= INT_MIN)
> + 	return (int)attr_data->u.sconstant;
> +       else
> + 	fatal_error ("signed value cannot be converted to int");
> +     default:
> +       fatal_error ("cannot interpret attribute value as integer");
> +     }
> + }

This looks good.

> + /* Convert the attribute value to an integer constant of type TYPE.  If
> +    TYPE is null, choose an appropriate signed/unsigned int type, depending
> +    on the form of the attribute data.  This doesn't do any checking to make
> +    sure that the constant value can actually be represented in the indicated
> +    TYPE; we'll assume that the compiler is smart enough not to emit such
> +    DWARF constants in the first place.  */
> + static tree
> + attribute_value_as_constant (DWARF2_form_data *attr_data, tree type)
> + {
> +   switch (attr_data->cl)
> +     {
> +     case DW_cl_uconstant:
> +       {
> + 	uint64_t u = attr_data->u.uconstant;
> + 	if (!type)
> + 	  type = unsigned_type_node;
> + 	if (sizeof (HOST_WIDE_INT) >= sizeof (uint64_t))
> + 	  return build_int_cstu (type, u);
> + 	else
> + 	  {
> + 	    int size = sizeof (HOST_WIDE_INT);
> + 	    int nbits = size * BITS_PER_UNIT;
> + 	    gcc_assert (size * 2 >= (int) sizeof (uint64_t));
> + 	    return build_int_cst_wide (type, (unsigned HOST_WIDE_INT)u,
> + 				       (HOST_WIDE_INT)(u >> nbits));
> + 	  }

The use of BITS_PER_UNIT here is incorrect, since that's a value for the 
target, not the host.  I think you want CHAR_BIT.  (Of course, 
sizeof(char) is always 1, independent of the number of bits.)

Pedantically, the u >> nbits still isn't quite right.  If u >> nbits has 
the high-order bit set, then, its (unsigned) value is not representable 
as a signed HOST_WIDE_INT, so the conversion is not defined by the C 
standard.

You actually have do something like:

  unsigned HOST_WIDE_INT low_word = u >> nbits;
  unsigned HOST_WIDE_INT mask = (1U << (bits - 1)));
  int sign = (low_word & mask) ? -1 : 1;
  low_word &= ~mask;
  HOST_WIDE_INT value = sign * (int) low_word;

This applies to the sconstant case as well, so put it in a function...

Having fun yet?

> + static tree
> + lto_find_integral_type (tree base_type, int byte_size, bool got_byte_size)
> + {
> +   int nbits = byte_size * BITS_PER_UNIT;

This use of BITS_PER_UNIT is fine, since you're talking about target 
types here.

> Index: gcc/lto/lto.h
> ===================================================================
> *** gcc/lto/lto.h	(revision 116727)
> --- gcc/lto/lto.h	(working copy)
> *************** Boston, MA 02110-1301, USA.  */
> *** 25,30 ****
> --- 25,31 ----
>   /* Included files.  */
>   
>   #include "hashtab.h"
> + #include "tree.h"
>   #include <inttypes.h>
>   
>   /* Forward Declarations */

You need a lto/Make-lang.in change to go with this, adding a dependency 
on $(TREE_H) for all cases that presently depend on lto.h.  (Think 
manually maintened include-file dependencies are lame?  Get in line...)

Please make those changes and check in.

Thanks,

--
Mark Mitchell
CodeSourcery
mark@codesourcery.com
(650) 331-3385 x713