From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 92002 invoked by alias); 9 Jul 2018 20:56:01 -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 91993 invoked by uid 89); 9 Jul 2018 20:56:00 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-7.7 required=5.0 tests=AWL,BAYES_00,GIT_PATCH_1 autolearn=ham version=3.3.2 spammy=friendly, transparent, realize, Better X-HELO: sesbmg23.ericsson.net Received: from sesbmg23.ericsson.net (HELO sesbmg23.ericsson.net) (193.180.251.37) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Mon, 09 Jul 2018 20:55:58 +0000 DKIM-Signature: v=1; a=rsa-sha256; d=ericsson.com; s=mailgw201801; c=relaxed/simple; q=dns/txt; i=@ericsson.com; t=1531169755; h=From:Sender:Reply-To:Subject:Date:Message-ID:To:CC:MIME-Version:Content-Type: Content-Transfer-Encoding:Content-ID:Content-Description:Resent-Date:Resent-From: Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:In-Reply-To:References:List-Id: List-Help:List-Unsubscribe:List-Subscribe:List-Post:List-Owner:List-Archive; bh=T0xpom/J6ROIC7mBFEbvvbOK9bCAFv3+sQXKgtjCPSo=; b=cKSFaiYW9L6wlKEHtENFNWcPbKxVHOqEC+8KPWk7FNBrPQlGKxNjGtYgCuiMGSMg B65UTFVzNIOVJ1qFSgsnxvYYvspyE6o+3Ds4BtHQIeCsekn64Lel/1hbbqny+56w tQ5EzXlkOLWCo6kPedNiyW40w37FGZPlBgZQH3OHrEM=; Received: from ESESBMB504.ericsson.se (Unknown_Domain [153.88.183.117]) by sesbmg23.ericsson.net (Symantec Mail Security) with SMTP id 8C.8C.25360.ADBC34B5; Mon, 9 Jul 2018 22:55:55 +0200 (CEST) Received: from ESESSMR506.ericsson.se (153.88.183.128) by ESESBMB504.ericsson.se (153.88.183.117) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256_P256) id 15.1.1466.3; Mon, 9 Jul 2018 22:55:54 +0200 Received: from ESESSMB501.ericsson.se (153.88.183.162) by ESESSMR506.ericsson.se (153.88.183.128) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256_P256) id 15.1.1466.3; Mon, 9 Jul 2018 22:55:54 +0200 Received: from NAM03-DM3-obe.outbound.protection.outlook.com (153.88.183.157) by ESESSMB501.ericsson.se (153.88.183.162) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256_P256) id 15.1.1466.3 via Frontend Transport; Mon, 9 Jul 2018 22:55:53 +0200 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=ericsson.com; s=selector1; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=js9yNutBdQ88NwUeDZFy+68oxzEp9q8u8qgRkhaKyBg=; b=TPxD16O0MH4zYCzXg/7TEYy4KnlX69KX4lNRbAsk0D4J/k0NZUhp1pewcKKLXvKP0O+1OprJqZIFaKbB3bVoppBJ00jxRxTMD0ldUtMW7K4mqVP+bRx0gYGZggoQYhdu49Xh/BnapVttQWb4Ab73oYjHu2k9I7f33rxAurcIF7Q= Authentication-Results: spf=none (sender IP is ) smtp.mailfrom=simon.marchi@ericsson.com; Received: from [142.133.61.121] (192.75.88.130) by DM6PR15MB2395.namprd15.prod.outlook.com (2603:10b6:5:8d::29) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.930.20; Mon, 9 Jul 2018 20:55:48 +0000 Subject: Re: [PATCH RFC 5/5] Add DWARF index cache To: Tom Tromey CC: References: <1525901216-15031-1-git-send-email-simon.marchi@ericsson.com> <1525901216-15031-6-git-send-email-simon.marchi@ericsson.com> <87tvrfyzf2.fsf@tromey.com> From: Simon Marchi Message-ID: Date: Mon, 09 Jul 2018 20:56:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.8.0 MIME-Version: 1.0 In-Reply-To: <87tvrfyzf2.fsf@tromey.com> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Return-Path: simon.marchi@ericsson.com Received-SPF: None (protection.outlook.com: ericsson.com does not designate permitted sender hosts) X-IsSubscribed: yes X-SW-Source: 2018-07/txt/msg00228.txt.bz2 On 2018-05-10 05:27 PM, Tom Tromey wrote: >>>>>> "Simon" == Simon Marchi writes: > > Simon> To help make using the DWARF index more transparent, this patch > Simon> introduces a DWARF index cache. When enabled, loading an index-less > Simon> binary in GDB will automatically save an index file in ~/.cache/gdb. > Simon> When loading that same object file again, the index file will be looked > Simon> up and used to load the DWARF index. > > Thanks for doing this. I like this feature. > > I read through the patch, but I didn't read the tests. Some notes > below. > > I'm sure you know, since it is an RFC, but the patch needs some more > comments here and there. I didn't realize it at the time, but reading it now, I concur. I added some, hopefully it helps. > I have a wacky idea, too -- I'm testing a patch to move psymtabs out of > the objfile obstack and into a "self managed" object. With this in > place, and because psymtabs are effectively immutable, it seems to me > that the index writing could be done in a background thread. (I haven't > looked to see how the DWARF 5 index stuff works, I only really know > about .gdb_index, so this may not apply there.) Yes, sounds like a good idea. > Simon> - The cache is just a directory with files named after the object > Simon> file's build-id. It is not possible to save/load the index for an > Simon> object file without build-id in the cache. Note that Clang does not > Simon> add build-ids by default. > > One thing that would be nice is also having some way to associate file > names with the build-ids. Then gdb could notice when a build-id is > stale and unlink that cache entry. > > For example you could have a symlink whose name is the some mangled form > of the objfile name, and whose target is the cache file. Then if gdb > sees an objfile with that name but with a different link target, it can > remove the link target. This might be a pain to implement in a non-racy > way, though. Good idea, but as you said it could be racy. There needs to be more thought put into this, so I'll leave this for later. > I wonder if gdb should write out a short README into that directory when > it is first created. Good idea. In particular, it could say what these files are used for, and say that it's perfectly safe to delete them if they take too much space (and refer to the GDB manual about how to manage the cache size limit, eventually). I'll keep this idea in mind for later. > Simon> +/* A cheap (as in low-quality) recursive mkdir. Try to create all the parents > Simon> + directories up to DIR and DIR itself. Stop if we hit an error along the way. > Simon> + There is no attempt to remove created directories in case of failure. */ > Simon> + > Simon> +static void > Simon> +mkdir_recursive (const char *dir) > > gnulib has a 'mkdir-p' module which could perhaps be used instead. One problem of the 'mkdir-p' module is that it pulls the xmalloc module, and we end up with multiple definitions of xmalloc, xrealloc, etc. However, the mkdir-p module uses the mkancesdirs module internally, which is simpler and would probably be enough for our needs. However, I couldn't quite figure out how to use the savewd argument... Also, the mkancesdirs module is not C++-friendly yet, so there would need to be some patches to gnulib first, then we would need to update our gnulib import. I would leave it as-is for the initial submission to keep it simple, it can always be improved later. > Simon> +index_cache_resource::~index_cache_resource () = default; > > Simon> +struct index_cache_resource > Simon> +{ > Simon> + virtual ~index_cache_resource () = 0; > Simon> +}; > > I don't understand how to reconcile these two things - probably just > another bit of C++ knowledge I am missing. Would you mind explaining > it? The "= 0" part means the method (or destructor, in this case) is pure. This is to prevent somebody instantiating this class directly. The "= default" one is to ask the compiler to generate the default version of the destructor for this class. If you remove it, you'll get some undefined reference to "~index_cache_resource". I think it's equivalent to index_cache_resource::~index_cache_resource () {} but more C++11-y. We often see "= default" used directly in the class definition, but AFAIK it's not possible to put both "= 0" and "= default" on the same declaration, hence the need to place it in the .c file. > > Simon> +std::string > Simon> +index_cache::make_index_filename (const bfd_build_id *build_id, > Simon> + const char *suffix) const > Simon> +{ > Simon> + std::string build_id_str = build_id_to_string (build_id); > Simon> + > Simon> + return string_printf ("%s%s%s%s", m_dir.c_str (), SLASH_STRING, > Simon> + build_id_str.c_str (), suffix); > > I think this particular one would be clearer just using "+". Done. > Simon> + /* Err... I guess /tmp is better than nothing. */ > Simon> + index_cache_directory = xstrdup ("/tmp"); > Simon> + global_index_cache.set_directory ("/tmp"); > > I tend to think nothing would be better. Using /tmp means that gdb will > be reading files with (sometimes) predictable names that could be > created by anybody. Better, IMO, to issue some sort of warning and > disable the feature. Done. Thanks for looking into it! Since I didn't receive too many negative comments, I'll send a non-RFC version soon. Simon