From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-15.2 required=3.0 tests=BAYES_00,DKIM_INVALID, DKIM_SIGNED,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_CR_TRAILER,INCLUDES_PATCH, MAILING_LIST_MULTI,NICE_REPLY_A,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED, USER_AGENT_SANE_1 autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id D5238C433DB for ; Fri, 12 Feb 2021 16:38:12 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 9B26E64E76 for ; Fri, 12 Feb 2021 16:38:12 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S231255AbhBLQhg (ORCPT ); Fri, 12 Feb 2021 11:37:36 -0500 Received: from mail.codeweavers.com ([50.203.203.244]:60222 "EHLO mail.codeweavers.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S230405AbhBLQfX (ORCPT ); Fri, 12 Feb 2021 11:35:23 -0500 DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=codeweavers.com; s=6377696661; h=Content-Transfer-Encoding:Content-Type: In-Reply-To:MIME-Version:Date:Message-ID:From:References:Cc:To:Subject:Sender :Reply-To: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=1VfmQ77PxuEOdhUY66vFbUOVKfvsA0MA2w0xGaCPvFg=; b=QpKFRrMsA3t3aSKEWX1bJ+tztV +Bzc0phGL9JLcv69msldabhngBTJ8Wgv6jWhfLs6FvxbVlAnIgFOlZGQvWXc9YDKO1G9iLNqSgMFR PLkX4w6+z9hfz4fIFmfuxQNk7C9JKyd7LpPtCX06Iu3dwD/7v7cukVCHL7j5uCYCbC/I=; Received: from [10.69.141.136] by mail.codeweavers.com with esmtpsa (TLS1.3:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.92) (envelope-from ) id 1lAbOo-0007xA-6U; Fri, 12 Feb 2021 10:34:32 -0600 Subject: Re: [PATCH 2/4] perf report: Load PE files from debug cache only To: Arnaldo Carvalho de Melo Cc: Peter Zijlstra , Ingo Molnar , Mark Rutland , Alexander Shishkin , Jiri Olsa , Namhyung Kim , Ian Rogers , "Frank Ch. Eigler" , Song Liu , Adrian Hunter , Kim Phillips , Tommi Rantala , Remi Bernon , linux-kernel@vger.kernel.org, Ulrich Czekalla , Huw Davies References: <20210212122800.GA1398414@kernel.org> From: Nicholas Fraser Message-ID: <367456d0-78ea-f2e1-2269-3e6cf95ea3fb@codeweavers.com> Date: Fri, 12 Feb 2021 11:34:24 -0500 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.7.1 MIME-Version: 1.0 In-Reply-To: <20210212122800.GA1398414@kernel.org> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Sorry, I should have been more clear in the commit message. The use case you outlined still works even with this patch. dso__load_bfd_symbols() is called in a loop from dso__load() for a variety of paths. These are generated by the various DSO_BINARY_TYPEs in the binary_type_symtab list at the top of util/symbol.c. In each case the debugfile passed to dso__load_bfd_symbols() is the path to try. One of those iterations (the first one I believe) passes the original path as the debugfile. If the file still exists at the original path, this is the one that ends up being used in case the debugcache was deleted or the PE file doesn't have a build-id. A later iteration (BUILD_ID_CACHE) passes debugfile as the file in the debugcache if it has a build-id. Even if the file was previously loaded at its original path, (if I understand correctly) this load will override it so the debugcache file ends up being used. Nick On 2021-02-12 7:28 a.m., Arnaldo Carvalho de Melo wrote: > Em Wed, Feb 10, 2021 at 02:17:38PM -0500, Nicholas Fraser escreveu: >> dso__load_bfd_symbols() attempts to load a DSO at its original path, >> then closes it and loads the file in the debug cache. This is incorrect. >> It should ignore the original file and work with only the debug cache. >> The original file may have changed or may not even exist, for example if >> the debug cache has been transferred to another machine via "perf >> archive". >> >> This fix makes it only load the file in the debug cache. > > Well this improves your current use case and only affects PE files, so I > am applying, but consider a slightly different workflow: > > 1. perf record ./foo.exe > 2. perf report # works, finds the file in the ~/.debug cache, as stored > # by 'perf record' > 3. rm -rf ~/.debug # I need more space > 4. perf report # Fails, as it looks only in the ~/.debug cache, that > # was nuked > > > So at 4 it should look at the original pathname, and hope for the best. > > All this is moot if we have something like a build-id in PE files, > where we can look in any order since we'll verify the unique ID to see > if it is the one we need, right? > > - Arnaldo > >> Signed-off-by: Nicholas Fraser >> --- >> tools/perf/util/symbol.c | 8 +------- >> 1 file changed, 1 insertion(+), 7 deletions(-) >> >> diff --git a/tools/perf/util/symbol.c b/tools/perf/util/symbol.c >> index 64a039cbba1b..aa9ae875b995 100644 >> --- a/tools/perf/util/symbol.c >> +++ b/tools/perf/util/symbol.c >> @@ -1569,7 +1569,7 @@ int dso__load_bfd_symbols(struct dso *dso, const char *debugfile) >> u_int i; >> u64 start, len; >> >> - abfd = bfd_openr(dso->long_name, NULL); >> + abfd = bfd_openr(debugfile, NULL); >> if (!abfd) >> return -1; >> >> @@ -1586,12 +1586,6 @@ int dso__load_bfd_symbols(struct dso *dso, const char *debugfile) >> if (section) >> dso->text_offset = section->vma - section->filepos; >> >> - bfd_close(abfd); >> - >> - abfd = bfd_openr(debugfile, NULL); >> - if (!abfd) >> - return -1; >> - >> if (!bfd_check_format(abfd, bfd_object)) { >> pr_debug2("%s: cannot read %s bfd file.\n", __func__, >> debugfile); >> -- >> 2.30.0 >> >