From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 96778 invoked by alias); 24 Jul 2019 23:50:51 -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 96411 invoked by uid 89); 24 Jul 2019 23:50:51 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-13.7 required=5.0 tests=AWL,BAYES_00,SPF_HELO_PASS autolearn=ham version=3.3.1 spammy=xfree, sk:hashtab X-HELO: userp2120.oracle.com Received: from userp2120.oracle.com (HELO userp2120.oracle.com) (156.151.31.85) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Wed, 24 Jul 2019 23:50:49 +0000 Received: from pps.filterd (userp2120.oracle.com [127.0.0.1]) by userp2120.oracle.com (8.16.0.27/8.16.0.27) with SMTP id x6ONn44u139549; Wed, 24 Jul 2019 23:50:48 GMT DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=oracle.com; h=subject : to : cc : references : from : message-id : date : mime-version : in-reply-to : content-type : content-transfer-encoding; s=corp-2018-07-02; bh=4lXhzEsDtQ7Tq+69F9v2S5YG4lz6n5iXHpVDvTsrQiE=; b=4Bp4q7kgxE1wdwMMir3MYcqQVf/pfLzEzMzE1sIJMt13deytkY1aQoHtr9BHknGPZkxO 0x+1Un43A8JpRrkm01s5OZOyrsCXLT49SfeVBvjnBJB1RS+2y0j61lc7jCnXkH/qqjJB GSZD3aXGS187av32Vpn5zSU2jwuEU2NWEjExbK0cGw1IBvN25QroemDxh83m35y8yjc9 1s++nMhgNCMvPRUY4OGV+uCJ639rbT52bKPFwy4xFbaH/EdziyfjU/6xM4Qtt7axNlQg Pw98fBoJroV0eBtZhlJo2NNpjnsUR6pIXL2qkuateyJcH35hbTZMmIzwtNjoAUrQXsta tQ== Received: from aserp3030.oracle.com (aserp3030.oracle.com [141.146.126.71]) by userp2120.oracle.com with ESMTP id 2tx61c0d90-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK); Wed, 24 Jul 2019 23:50:48 +0000 Received: from pps.filterd (aserp3030.oracle.com [127.0.0.1]) by aserp3030.oracle.com (8.16.0.27/8.16.0.27) with SMTP id x6ONm21I087441; Wed, 24 Jul 2019 23:50:47 GMT Received: from aserv0122.oracle.com (aserv0122.oracle.com [141.146.126.236]) by aserp3030.oracle.com with ESMTP id 2tx60xghxw-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK); Wed, 24 Jul 2019 23:50:47 +0000 Received: from abhmp0001.oracle.com (abhmp0001.oracle.com [141.146.116.7]) by aserv0122.oracle.com (8.14.4/8.14.4) with ESMTP id x6ONokgZ022220; Wed, 24 Jul 2019 23:50:46 GMT Received: from [10.159.233.50] (/10.159.233.50) by default (Oracle Beehive Gateway v4.0) with ESMTP ; Wed, 24 Jul 2019 16:50:46 -0700 Subject: Re: [PATCH] gdb: CTF support To: Tom Tromey Cc: gdb-patches@sourceware.org References: <1563926228-31569-1-git-send-email-weimin.pan@oracle.com> <87v9vrtde8.fsf@tromey.com> From: Wei-min Pan Message-ID: Date: Wed, 24 Jul 2019 23:50:00 -0000 User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:60.0) Gecko/20100101 Thunderbird/60.8.0 MIME-Version: 1.0 In-Reply-To: <87v9vrtde8.fsf@tromey.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit X-SW-Source: 2019-07/txt/msg00555.txt.bz2 On 7/24/2019 12:56 PM, Tom Tromey wrote: > > >>>>> ">" == 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; }; There are more libctf changes in both Makefile.def and Makefile.in. Please see commit 0e65dfbaf3a0299e4837216a103c28625d4b4f1d which should address your concern? > >> 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. Currently ctf toolchain supports the ctf info for single CU only but is expected to be expanded to support multiple CU's. At which point, handling partial symbol table will make more sense. > >> +#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. Good catch, this item has been on my to-do list but I didn't get a chance to really look into buildsym.h or fully understand it. > >> +#include > > Probably just "ctf.h" here? The top-level include directory is already > on the include path. There is already a gdb/ctf.h for tracing? > >> +#include > > Probably "ctf-api.h" here. Done. > >> +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. Where can I get more info on this API? > >> +/* 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". Corrected. > >> +struct nextfield > >> +{ > >> + struct field field {}; > >> +}; > > IMO you might as well just remove this wrapper struct. Since it's likely that CTF will be expanded to support C++, new members such as "virtuality" and "accessibility" might be added to this struct. > >> +/* Hash function for a ctf_tid_and_type. */ > >> + > >> +struct ctf_tid_and_type > >> +{ > > That comment is slightly misplaced. It's been moved down to function tid_and_type_hash. > >> +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. Will take a look at elfread.c. Thanks. > 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. It's needed only when creating psymtabs and expanding symbols. BTW I borrowed this idea from dwarf2read.c. So you recommend that I use xcalloc/xfree instead? > 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. Corrected. > > >> + && 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. OK, fixed this in several places. So we don't want "()" in stmt like: return (ids_lhs->tid == ids_rhs->tid); > > >> + 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? I'm pretty certain that is the intention. But will take another look to see if I need get_global_symbols. > > thanks, > Tom Thank you very much for your comments. Weimin