Re: [RFA] comdat types

[Tom Tromey <tromey@redhat.com>]

>>>>> "Doug" == Doug Evans <dje@google.com> writes:

Doug> This patch adds support for "comdat types" from dwarf4.
Doug> http://wiki.dwarfstd.org/index.php?title=COMDAT_Type_Sections

I am a fan of this.

I'm curious about one thing on this page, and I thought I'd take the
opportunity to ask.  DW_TAG_type_unit may have a DW_AT_language child.
But, the language is not mentioned in the suggested method for
computing a type's signature.  This seems strange to me: either the
language matters (in which case, it seems like it ought to be in the
signature); or the language does not matter, in which case, why
mention it?

Doug> I need to revisit them with the latest gcc (the dwarf4 branch)
Doug> as they seem like gcc issues.
Doug> [specifically gdb.cp/classes.exp, and "info type foo" doesn't print the
Doug> source location though it should]

Yeah; my view is that since this patch can't hurt anything, if the gcc
changes require gdb changes in turn, it won't matter.

Doug> Ok to check in?

I read through it.  Unfortunately, I don't think I am enough of a
dwarf2read.c expert to approve this.  So, I would appreciate it if
someone more expert could claim it.  Alternatively, if nobody has time
to do this, and they say so, I will learn more and take it on.

One concern I have is whether there is any chance that the
specification will change between now and when DWARF-4 is published.

I do have a few nits.

Doug> +  types_htab = htab_create_alloc_ex (41,
Doug> +				     hash_type_signature,
Doug> +				     eq_type_signature,
Doug> +				     NULL,
Doug> +				     &objfile->objfile_obstack,
Doug> +				     hashtab_obstack_allocate,
Doug> +				     dummy_obstack_deallocate);

This is just a side note -- I've seen a few hash tables allocated on
obstacks.  Doesn't resizing the table waste memory?

Too bad we don't have allocation pools instead of obstacks.  I suppose
in this specific case we could use the objfile data machinery to
deallocate hash tables.

Doug> +  if (dwarf2_die_debug)
Doug> +    {
Doug> +      fprintf_unfiltered (gdb_stdlog, "Signatured types:\n");
Doug> +    }

Over-bracing.  There's a fair amount of this in the patch.

Doug> +  if (this_cu->from_debug_types)
Doug> +    {
Doug> +      /* ??? How come this is for .debug_types only?  */
Doug> +      this_cu->offset = cu.header.offset;
Doug> +      this_cu->length = cu.header.length + cu.header.initial_length_size;

Daniel has asked before for "no new FIXMEs".  You can't escape this by
spelling it "???" :-)

Seriously, I think this is a valid concern and I would like some
definitive resolution to the various "???" comments.

Doug> + NotFound:

Should be "not_found".  There's a couple of these.

Tom