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] PATCH: Add DIE readers for array and enum types

Sandra Loosemore wrote:

2006-08-31 Sandra Loosemore <>

* 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.


(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.


(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.


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]