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

Mark Mitchell mark@codesourcery.com
Thu Aug 31 21:01:00 GMT 2006


Sandra Loosemore wrote:

> 2006-08-31  Sandra Loosemore  <sandra@codesourcery.com>
> 
> 	* gcc/lto/lto.c (struct lto_context): Remove type field, add language.

> 	(lto_read_uleb128): Fix to assemble bytes in the correct order.

OK.  Good catch!

> 	(lto_read_sleb128): New.

+   return (int64_t) result;

Casting an out-of-range unsigned value to a signed type is not defined 
by the ISO standard.  So, I think you have to maintain the value as 
unsigned, cast it to signed before dealing with the sign bit, and then, 
if the sign bit is set, handle that in the signed type.

> 	(lto_read_form): Make it know about some more attributes.  Make it
> 	understand DW_FORM_sdata and DW_FORM_udata.

OK.  (Here, you assign a signed value to an unsigned variable -- but the 
values are guaranteed by the standard in that case.)

> 	(lto_find_integral_type): New.

+    FIXME: This currently just barfs if both are specified and are not
+    the same size.  */

Remove the FIXME from here, and if the unhandled condition occurs, call 
sorry.

> 	(lto_read_compile_unit_DIE): Return tree instead of void.  Set the
> 	language in the context.

OK.

> 	(lto_read_array_type_DIE): New.

> !   /* Array dimensions are given as children of the DW_TAG_array_type DIE,
> !      and are tagged with either DW_TAG_subrange_type or
> !      DW_TAG_enumeration_type.  */
> ! 
> !   dims = lto_collect_child_DIEs (fd, abbrev, context);
> !   ndims = VEC_length (tree, dims);

Minor style thing: no blank line between the comment and the following 
code.  Otherwise, OK.

> 	(lto_read_enumeration_type_DIE): New.

Do you really need to reconcile the DW_AT_byte_size and DW_AT_type 
things here, using lto_find_integral_type?  I would suggest that GCC 
should always emit DW_AT_type, and that you just check that 
DW_AT_byte_size, if present, matches the size type specified by DW_AT_type.

!   if (name)
!     TYPE_NAME (type) = name;

That seems like the right thing, but it isn't.  TYPE_NAME should 
actually be a TYPE_DECL whose DECL_NAME is the IDENTIFIER_NODE with the 
obvious name.  See lto_read_base_type_DIE for the correct incantations here.

> 	(lto_read_enumerator_DIE): New.

!   /* FIXME:  enumerators have integer type in C, but in C++ they may have
!      bigger values.  What about other languages?  Probably should pass in
!      base type from the parent enumeration type.  */

Yes, you should. :-)

Here, you have:

!     case DW_AT_const_value:
!       value = build_int_cst (type, (int64_t)attr_data.u.constant);

this is the unsigned->signed problem again.  Here, I think you should 
use lto_check_int64_t_val; look for LTO_CHECK_INT_VAL to see how this 
works.  It's worst than that, actually -- build_int_cst takes a 
HOST_WIDE_INT.  So, you should be casting to HOST_WIDE_INT, via 
lto_check_HOST_WIDE_INT_val.

!       /* FIXME:  Do we need to build and record a DECL for the 
enumerators? */

No, we shouldn't need to do that.  So, please remove the FIXME.

> 	(lto_read_variable_formal_parameter_constant_DIE): Return the decl
> 	instead of void.
> 	(lto_read_pointer_type_DIE): Return the type instead of void.

OK.

> 	(lto_read_subrange_type_DIE): New.

Need lto_check_HOST_WIDE_INT_val here too.  Or build_int_cstu?

I would again just verify that DW_AT_byte_size matches DW_AT_type here. 
  I don't think they'll ever be different for GCC, and that allows us to 
ditch lto_find_integral_type.  (Though I would keep the code somewhere; 
we might still need that.)

As a meta-note, if I'm correct that DW_AT_byte_size matches DW_AT_type, 
then we could simplify the debug generators not to spit out both, at 
least when the size matches the size of the type.  Generating less 
debugging information (without losing information) is a good thing.

> 	(lto_read_base_type_DIE): Return the type instead of void.  Also,
> 	GCC generates unnamed base_type DIEs, so do something sensible if
> 	we get one.

How ill-mannered to be generating unnamed base-types!  I think we should 
figure out where that's coming from and fix it; I can't see any reason 
that the range type for an array shouldn't be based on "int" or "long 
long", or some such.  But, we can live with this for now.

> 	(lto_read_DIE): Change type to return the tree value constructed from
> 	reading the DIE.  Make the boolean "more" flag a parameter instead.
> 	Updated all callers.  Make it know about the readers for array_type,
> 	enumeration_type, and subrange_type, and enumerator DIEs.

Style comment:

!       lto_read_array_type_DIE, /* array_type */

I don't think we need the comment here once we will in the function; 
we've intentionally made the function names reflect the DIE names.

> 	(lto_collect_child_DIEs): New.

OK.  You're missing a ChangeLog entry for lto_file_read, but that's OK too.

Thanks,

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



More information about the Gcc-patches mailing list