From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 64079 invoked by alias); 24 Jul 2019 19:56:35 -0000 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 Received: (qmail 64071 invoked by uid 89); 24 Jul 2019 19:56:35 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-5.7 required=5.0 tests=AWL,BAYES_00,RCVD_IN_DNSWL_NONE,RCVD_IN_SEMBLACK,SPF_HELO_PASS autolearn=no version=3.3.1 spammy=xfree X-HELO: gateway23.websitewelcome.com Received: from gateway23.websitewelcome.com (HELO gateway23.websitewelcome.com) (192.185.50.120) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Wed, 24 Jul 2019 19:56:33 +0000 Received: from cm17.websitewelcome.com (cm17.websitewelcome.com [100.42.49.20]) by gateway23.websitewelcome.com (Postfix) with ESMTP id 4DA186892 for ; Wed, 24 Jul 2019 14:56:32 -0500 (CDT) Received: from box5379.bluehost.com ([162.241.216.53]) by cmsmtp with SMTP id qNNIhnf6M90onqNNIhgBF2; Wed, 24 Jul 2019 14:56:32 -0500 DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=tromey.com; s=default; h=Content-Type:MIME-Version:Message-ID:In-Reply-To:Date: References:Subject:Cc:To:From:Sender:Reply-To:Content-Transfer-Encoding: Content-ID:Content-Description:Resent-Date:Resent-From:Resent-Sender: Resent-To:Resent-Cc:Resent-Message-ID:List-Id:List-Help:List-Unsubscribe: List-Subscribe:List-Post:List-Owner:List-Archive; bh=mfFR2MZCVI2g7blxpQT9lyiYu1ybzdcv/VQ+zBoUy7s=; b=GjzkpuBeX0DRZGMC/7IQXNAaac /YRPl2OknOKEylLuSQDY/4OnukAHt4/0CUKIPjuxOT3nPl6OzGxGrD3ekkUPPNOEgPYAGnJv6pbiB K0mX653ITCRFxFjUicyYS8Aha; Received: from 97-122-178-82.hlrn.qwest.net ([97.122.178.82]:38056 helo=murgatroyd) by box5379.bluehost.com with esmtpsa (TLSv1.2:ECDHE-RSA-AES256-GCM-SHA384:256) (Exim 4.92) (envelope-from ) id 1hqNNI-002966-10; Wed, 24 Jul 2019 14:56:32 -0500 From: Tom Tromey To: Weimin Pan Cc: gdb-patches@sourceware.org Subject: Re: [PATCH] gdb: CTF support References: <1563926228-31569-1-git-send-email-weimin.pan@oracle.com> Date: Wed, 24 Jul 2019 19:56:00 -0000 In-Reply-To: <1563926228-31569-1-git-send-email-weimin.pan@oracle.com> (Weimin Pan's message of "Tue, 23 Jul 2019 19:57:08 -0400") Message-ID: <87v9vrtde8.fsf@tromey.com> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/26.1 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain X-SW-Source: 2019-07/txt/msg00553.txt.bz2 >>>>> ">" == Weimin Pan writes: >> This patch adds the CTF (Compact Ansi-C Type Format) support in gdb. Thank you for the patch. >> Two submissions on which this gdb work depends were posted earlier >> in May: >> * On the binutils mailing list - adding libctf which creates, updates, >> reads, and manipulates the CTF data. I suspect the top-level Makefile.def / Makefile.in will need to be modified to ensure that libctf is built before gdb. The only dependency I see on libctf right now is for binutils: dependencies = { module=all-binutils; on=all-libctf; }; >> The two-stage symbolic reading and setting strategy, partial and >> full, was used. Is there a benefit to making partial symbol tables for CTF? It's fine to do it the way you've done it, but I'm interested in the reasoning. >> +#include "buildsym-legacy.h" It's better to use the new buildsym.h if you possibly can. And, if you can't, it would be better to improve it. >> +#include Probably just "ctf.h" here? The top-level include directory is already on the include path. >> +#include Probably "ctf-api.h" here. >> +static const struct objfile_data *ctf_tid_key; >> +static const struct objfile_data *ctf_file_key; There's a type-safe registry API now. I would prefer that for new code. >> +/* The routines that read and process fields/members of a C struct, union, >> + or enumeration, pass lists of data member fields in an instance of a >> + field_info structure. It is dervied from dwarf2read.c. */ Typo, "derived". >> +struct nextfield >> +{ >> + struct field field {}; >> +}; IMO you might as well just remove this wrapper struct. >> +/* Hash function for a ctf_tid_and_type. */ >> + >> +struct ctf_tid_and_type >> +{ That comment is slightly misplaced. >> +static struct type * >> +set_tid_type (struct objfile *of, ctf_id_t tid, struct type *typ) >> +{ >> + htab_t htab; >> + htab = (htab_t) objfile_data (of, ctf_tid_key); >> + if (htab == NULL) >> + { >> + htab = htab_create_alloc_ex (1, tid_and_type_hash, >> + tid_and_type_eq, >> + NULL, &of->objfile_obstack, >> + hashtab_obstack_allocate, >> + dummy_obstack_deallocate); >> + set_objfile_data (of, ctf_tid_key, htab); A few things here. First, I think it's best not to allocate hash tables on the objfile obstack. This approach means a memory leak (not detectable though) when the hash table is resized. It's just as convenient to use xcalloc/xfree. With the type-safe registry you can use htab_deleter for the keys's deleter; see elfread.c:elf_objfile_gnu_ifunc_cache_data. Second, is this hash table needed only when expanding symbols and/or creating psymtabs, or is it needed longer term? I am not familiar with CTF and I didn't read all parts of the patch in detail. Anyway I ask because if it is temporary, it's even better not to stash it in the objfile but rather just create it while reading and destroy it when done. Unfortunately it isn't possible, yet, to allocate a type on the BFD rather than the objfile, or else I would suggest that here. >> + *slot = XOBNEW (&of->objfile_obstack, struct ctf_tid_and_type); An addendum to the above: if it's not possible to remove items from the hash, then it is fine to store the node itself on the obstack. >> +static int >> +get_bitsize (ctf_file_t *fp, ctf_id_t tid, uint32_t kind) >> +{ >> + ctf_encoding_t cet; >> + >> + if (((kind == CTF_K_INTEGER) || (kind == CTF_K_ENUM) >> + || (kind == CTF_K_FLOAT)) The gdb style uses fewer parens here. >> + && ctf_type_reference (fp, tid) != CTF_ERR >> + && ctf_type_encoding (fp, tid, &cet) != CTF_ERR) >> + { >> + return (cet.cte_bits); gdb also doesn't use "()" for a return, unless it spans multiple lines. There were a few instances of this. >> + add_symbol_to_list (sym, get_file_symbols ()); get_file_symbols means the top-level "static" scope. Is this what you intended? Or was it supposed to be the global (external) scope? thanks, Tom