From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 7917 invoked by alias); 10 May 2018 21:27:19 -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 7891 invoked by uid 89); 10 May 2018 21:27:19 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-7.8 required=5.0 tests=AWL,BAYES_00,GIT_PATCH_1,RCVD_IN_DNSWL_NONE,SPF_HELO_PASS autolearn=ham version=3.3.2 spammy=flush X-HELO: gateway22.websitewelcome.com Received: from gateway22.websitewelcome.com (HELO gateway22.websitewelcome.com) (192.185.47.125) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Thu, 10 May 2018 21:27:15 +0000 Received: from cm17.websitewelcome.com (cm17.websitewelcome.com [100.42.49.20]) by gateway22.websitewelcome.com (Postfix) with ESMTP id 96612F912 for ; Thu, 10 May 2018 16:27:14 -0500 (CDT) Received: from box5379.bluehost.com ([162.241.216.53]) by cmsmtp with SMTP id Gt5mf1UWXy2aLGt5mfKZDJ; Thu, 10 May 2018 16:27:14 -0500 X-Authority-Reason: nr=8 Received: from 97-122-176-117.hlrn.qwest.net ([97.122.176.117]:35498 helo=pokyo) by box5379.bluehost.com with esmtpsa (TLSv1.2:ECDHE-RSA-AES256-GCM-SHA384:256) (Exim 4.89_1) (envelope-from ) id 1fGt5m-001Ys0-BA; Thu, 10 May 2018 16:27:14 -0500 From: Tom Tromey To: Simon Marchi Cc: Subject: Re: [PATCH RFC 5/5] Add DWARF index cache References: <1525901216-15031-1-git-send-email-simon.marchi@ericsson.com> <1525901216-15031-6-git-send-email-simon.marchi@ericsson.com> Date: Thu, 10 May 2018 22:24:00 -0000 In-Reply-To: <1525901216-15031-6-git-send-email-simon.marchi@ericsson.com> (Simon Marchi's message of "Wed, 9 May 2018 17:26:56 -0400") Message-ID: <87tvrfyzf2.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-BWhitelist: no X-Source-L: No X-Exim-ID: 1fGt5m-001Ys0-BA X-Source-Sender: 97-122-176-117.hlrn.qwest.net (pokyo) [97.122.176.117]:35498 X-Source-Auth: tom+tromey.com X-Email-Count: 2 X-Source-Cap: ZWx5bnJvYmk7ZWx5bnJvYmk7Ym94NTM3OS5ibHVlaG9zdC5jb20= X-Local-Domain: yes X-SW-Source: 2018-05/txt/msg00272.txt.bz2 >>>>> "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 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.) 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. I wonder if gdb should write out a short README into that directory when it is first created. Simon> - The size taken up by ~/.cache/gdb is not limited. I was thinking we Simon> could add configurable limit (like ccache does), but that would come Simon> after. Also, maybe a command to flush the cache. Seems reasonable. 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. 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? 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 "+". 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. Tom