[lto] PATCH: Add DIE readers for array and enum types
Thu Aug 31 21:01:00 GMT 2006
Sandra Loosemore wrote:
> 2006-08-31 Sandra Loosemore <email@example.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
> (lto_read_compile_unit_DIE): Return tree instead of void. Set the
> language in the context.
> (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
! /* FIXME: Do we need to build and record a DECL for the
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.
> (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.
! 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.
(650) 331-3385 x713
More information about the Gcc-patches