This is the mail archive of the mailing list for the GCC project.

Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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

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);
+ 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/ 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.


Mark Mitchell
(650) 331-3385 x713

Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]