Re: [lto] PATCH: DIE readers for struct/union/class types, + other bug fixes

[Mark Mitchell <mark at codesourcery dot com>]

Sandra Loosemore wrote:

> 2006-09-29  Sandra Loosemore  <sandra@codesourcery.com>
>
> 	* gcc/lto/lto.c (lto_check_HOST_WIDE_INT_val): New.
> 	(lto_read_form): Make it know about DW_AT_bit_offset, DW_AT_bit_size,
> 	DW_AT_data_member_location, DW_AT_specification.
> 	(struct lto_die_cache_entry): Add sibling field.
> 	(lto_cache_store_DIE): Set sibling field.
> 	(lto_cache_lookup_DIE): Add SKIP argument; if true, skip to stored
> 	sibling pointer when a cache hit is found.
> 	(lto_read_referenced_type_DIE): Save/restore context->parentdata
> 	while reading the DIE.  Add FIXME note about potential bugs.
> 	(lto_read_structure_union_class_type_DIE): New.
> 	(lto_read_member_DIE): New.
> 	(lto_read_pointer_reference_type_DIE): Treat missing type attribute
> 	as void instead of error, for compatibility with dwarf2out.c behavior.
> 	(lto_read_const_volatile_restrict_type_DIE): Likewise.
> 	(lto_read_DIE): Add entries for new DIE readers to lookup table.
> 	Don't read children of an already-processed DIE a second time.

This looks very good.  A few nits:

> + 	  /* This is kind of nasty.  DWARF doesn't encode the overall
> + 	     alignment of the struct/union type, so we'll take a stab at
> + 	     recomputing it and hope it comes out the same as for the
> + 	     original type.  */
> + 	  if (DECL_ALIGN (child) > align)
> + 	    align = DECL_ALIGN (child);

This is OK for the moment, but I think we need a different long-term 
solution.  We should either use the usual stor-layout.c routines 
(place_field, etc.), or we should encode everything in DWARF and then 
just completely bypass all of that stuff.  I like the second alternative 
better because it eliminates language-dependent oddities.

So, I think we should take advantage of DWARF's extensibility to add 
additional attributes for the things we need.  One such would be 
alignment of the record type.

> + 	  /* FIXME: mess with propagating mutable/volatile/etc attributes
> + 	     back to parent struct type.  */

I'd prefer this to be:

  if (TYPE_MUTABLE_P || TYPE_VOLATILE_P)
    sorry (...);

Blowing up for things we don't understand is going to make it easier to 
debug things later.  But, it's not a huge deal either way.

> + 	case VAR_DECL:
> + 	  /* Static variables in a class.  */
> + 	  DECL_CONTEXT (child) = type;
> + 	  *fields_tail = child;
> + 	  fields_tail = &TREE_CHAIN (child);
> + 	  break;
> + 
> + 	case FUNCTION_DECL:
> + 	  /* Member functions of a class.
> + 	     FIXME: Extend the DW_TAG_subprogram reader to recognize
> + 	     additional attributes for member functions.  */
> + 	  DECL_CONTEXT (child) = type;
> + 	  *methods_tail = child;
> + 	  methods_tail = &TREE_CHAIN (child);
> + 	  break;

I think the DECL_CONTEXT logic should be pushed down into the routines 
that create the FUNCTION_DECLs/VAR_DECLs.  When we create a 
{FUNCTION,VAR}_DECL we want to set up all of its fields, so that the 
merge routines can check them at that point.  Since this is just for 
C++, maybe we should just drop these bits for now?

> + 	  /* FIXME: Add support for DW_TAG_access_declaration,
> + 	     DW_TAG_inheritance,  DW_TAG_friend, and DW_TAG_variant_part
> + 	     which can appear as children of a class/struct type.  */

I would add these cases, with a sorry or equivalent; let's not silently 
accept them.

> +   /* The type mode isn't encoded in the DWARF spec, either, so just recompute
> +      it from scratch.  */

Yup; we should probably encode that as well, as above for alignment.

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