From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 3405 invoked by alias); 25 Jun 2009 19:47:04 -0000 Received: (qmail 3391 invoked by uid 22791); 25 Jun 2009 19:47:03 -0000 X-SWARE-Spam-Status: No, hits=-2.3 required=5.0 tests=AWL,BAYES_00,SPF_HELO_PASS,SPF_PASS X-Spam-Check-By: sourceware.org Received: from mx2.redhat.com (HELO mx2.redhat.com) (66.187.237.31) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Thu, 25 Jun 2009 19:46:54 +0000 Received: from int-mx2.corp.redhat.com (int-mx2.corp.redhat.com [172.16.27.26]) by mx2.redhat.com (8.13.8/8.13.8) with ESMTP id n5PJkmGU014521; Thu, 25 Jun 2009 15:46:49 -0400 Received: from ns3.rdu.redhat.com (ns3.rdu.redhat.com [10.11.255.199]) by int-mx2.corp.redhat.com (8.13.1/8.13.1) with ESMTP id n5PJklZd007397; Thu, 25 Jun 2009 15:46:48 -0400 Received: from opsy.redhat.com (vpn-13-18.rdu.redhat.com [10.11.13.18]) by ns3.rdu.redhat.com (8.13.8/8.13.8) with ESMTP id n5PJkktc003764; Thu, 25 Jun 2009 15:46:47 -0400 Received: by opsy.redhat.com (Postfix, from userid 500) id 1EA983785DB; Thu, 25 Jun 2009 13:46:46 -0600 (MDT) To: dje@google.com (Doug Evans) Cc: gdb-patches@sourceware.org, ccoutant@google.com Subject: Re: [RFA] comdat types References: <20090620000402.C37E5843F5@localhost> From: Tom Tromey Reply-To: tromey@redhat.com Date: Thu, 25 Jun 2009 19:47:00 -0000 In-Reply-To: <20090620000402.C37E5843F5@localhost> (Doug Evans's message of "Fri\, 19 Jun 2009 17\:04\:02 -0700 \(PDT\)") Message-ID: User-Agent: Gnus/5.11 (Gnus v5.11) Emacs/22.2 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org X-SW-Source: 2009-06/txt/msg00685.txt.bz2 >>>>> "Doug" == Doug Evans 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