From patchwork Thu Jan 22 00:34:10 2015 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Victor Kamensky X-Patchwork-Id: 5681231 Return-Path: X-Original-To: patchwork-linux-arm@patchwork.kernel.org Delivered-To: patchwork-parsemail@patchwork2.web.kernel.org Received: from mail.kernel.org (mail.kernel.org [198.145.29.136]) by patchwork2.web.kernel.org (Postfix) with ESMTP id 35F2FC058E for ; Thu, 22 Jan 2015 00:36:24 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id 73CF020303 for ; Thu, 22 Jan 2015 00:36:22 +0000 (UTC) Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.9]) (using TLSv1.2 with cipher DHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id EDFDB20304 for ; Thu, 22 Jan 2015 00:36:20 +0000 (UTC) Received: from localhost ([127.0.0.1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.80.1 #2 (Red Hat Linux)) id 1YE5jK-0005Ei-FX; Thu, 22 Jan 2015 00:34:38 +0000 Received: from mail-qg0-f42.google.com ([209.85.192.42]) by bombadil.infradead.org with esmtps (Exim 4.80.1 #2 (Red Hat Linux)) id 1YE5jF-00053i-Nh for linux-arm-kernel@lists.infradead.org; Thu, 22 Jan 2015 00:34:34 +0000 Received: by mail-qg0-f42.google.com with SMTP id q107so24064958qgd.1 for ; Wed, 21 Jan 2015 16:34:10 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:mime-version:in-reply-to:references:date :message-id:subject:from:to:cc:content-type; bh=ontJEK29fXicdKjOQDQgNWIZSqEU94rGYtBnC0pCmJc=; b=meJQvKrphSUzqsHHYV2DXCo4WhnYueRP2DchIy3IaiveLsXqoUgxCnQNdRqcvUm09b PQrnUgZmyQNornPw3XraJDVudTD/f2kCmXSwvAO6sTKiml++spjEUwDbL7dzAUYC/9ru 1VfD30FxsZ8EfeAqp3tcKFZLxG3ElJGKl0UFSVNHLtKw6VVCSL2eObeuQeP7evVuh8IG xa0ffGFuUHPwh4SPdXJKMl0/ki4BK6aNsNmmAKihseXjFJ0tjmd/ACZlYXvMPKYttAJ8 esTAAvCpPvKzeTgyPDOsDyEO8nL8Ercmo3ifI50hmrWalyx3C8/pQuLE5doMCtBqYyZQ tX/Q== X-Gm-Message-State: ALoCoQlyVFRAQau4r0lFrYiGLNAvaEeuTXkhdN6bXR+I2m+u2rYl8dH+nijJzQOkeOQAabfIixj3 MIME-Version: 1.0 X-Received: by 10.140.108.74 with SMTP id i68mr59533574qgf.35.1421886850776; Wed, 21 Jan 2015 16:34:10 -0800 (PST) Received: by 10.229.68.197 with HTTP; Wed, 21 Jan 2015 16:34:10 -0800 (PST) In-Reply-To: <54C0216F.3080404@gmail.com> References: <1421168344-5363-1-git-send-email-victor.kamensky@linaro.org> <1421168344-5363-2-git-send-email-victor.kamensky@linaro.org> <54C0216F.3080404@gmail.com> Date: Wed, 21 Jan 2015 16:34:10 -0800 Message-ID: Subject: Re: [PATCH 2/2] perf symbols: debuglink should take symfs option into account From: Victor Kamensky To: David Ahern X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20150121_163433_926125_5DB7CC32 X-CRM114-Status: GOOD ( 21.91 ) X-Spam-Score: -0.7 (/) Cc: Waiman Long , Avi Kivity , Peter Zijlstra , Will Deacon , Adrian Hunter , Arnaldo Carvalho de Melo , open list , Ingo Molnar , Paul Mackerras , Anton Blanchard , Jiri Olsa , Masami Hiramatsu , Namhyung Kim , Jiri Olsa , Dave Martin , "linux-arm-kernel@lists.infradead.org" X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.18-1 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+patchwork-linux-arm=patchwork.kernel.org@lists.infradead.org X-Spam-Status: No, score=-4.2 required=5.0 tests=BAYES_00, RCVD_IN_DNSWL_MED, T_RP_MATCHES_RCVD, UNPARSEABLE_RELAY autolearn=unavailable version=3.3.1 X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on mail.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP David, Thank you for response! On 21 January 2015 at 14:00, David Ahern wrote: > On 1/19/15 10:50 AM, Victor Kamensky wrote: >>> >>> diff --git a/tools/perf/util/dso.c b/tools/perf/util/dso.c >>> index 45be944..6a2f663 100644 >>> --- a/tools/perf/util/dso.c >>> +++ b/tools/perf/util/dso.c >>> @@ -42,19 +42,30 @@ int dso__read_binary_type_filename(const struct dso >>> *dso, >>> size_t len; >>> >>> switch (type) { >>> - case DSO_BINARY_TYPE__DEBUGLINK: { >>> + case DSO_BINARY_TYPE__DEBUGLINK: >>> + { >>> char *debuglink; >>> - >>> - strncpy(filename, dso->long_name, size); >>> - debuglink = filename + dso->long_name_len; >>> - while (debuglink != filename && *debuglink != '/') >>> - debuglink--; >>> - if (*debuglink == '/') >>> - debuglink++; >>> - ret = filename__read_debuglink(dso->long_name, debuglink, >>> - size - (debuglink - >>> filename)); >>> - } >>> + char *filename_copy; >>> + >>> + filename_copy = malloc(PATH_MAX); >>> + if (filename_copy) { >>> + len = __symbol__join_symfs(filename, size, >>> + dso->long_name); >>> + strncpy(filename_copy, filename, PATH_MAX); >>> + debuglink = filename + len; >>> + while (debuglink != filename && *debuglink != >>> '/') >>> + debuglink--; >>> + if (*debuglink == '/') >>> + debuglink++; >>> + ret = filename__read_debuglink(filename_copy, >>> debuglink, >>> + size - (debuglink >>> - >>> + >>> filename)); >>> + free(filename_copy); >>> + } else >>> + ret = -1; >>> break; >>> + } >>> + > > > I do not believe the filename_copy is needed; just add the symfs path to > filename and pass filename to read_debuglink My first version of the fix did not create filename_copy and looked like as patch below. But then I noticed that filename__read_debuglink function receives two pointers: 'filename' first parameter pointer to executable path from which .gnu_debuglink sections content will be read 'debuglink' path to directory that will be updated by the function (note strncpy at line 531) to point to debug symbol file. Before my change offset within 'filename' parameter of dso__read_binary_type_filename function is passed as 'debuglink' parameter and it will be updated, and dso->long_name is separate, passed as 'filename' parameter to filename__read_debuglink function. When in the first version of patch, as below, I pass filename buffer to both (filename to open first and pointer within filename to be updated as debuglink) as in patch below, it will work with current implementation because open happens first but update happens after that. But that is specific dependency on current implementation of filename__read_debuglink function. It did not feel quite right to me, but practically it could be OK. Here is the first version of the without filename_copy. Practically I am OK with it. Please let me know if you prefer this version: From cafc06d95886f1d82f7b127af58a51384c0fe931 Mon Sep 17 00:00:00 2001 From: Victor Kamensky Date: Mon, 12 Jan 2015 17:33:06 -0800 Subject: [PATCH 2/2] perf symbols: debuglink should take symfs option into account Currently code that tries to read corresponding debug symbol file from .gnu_debuglink section (DSO_BINARY_TYPE__DEBUGLINK) does not take in account symfs option, so filename__read_debuglink function cannot open ELF file, if symfs option is used. Fix is to add proper handling of symfs as it is done in other places: use __symbol__join_symfs function to get real file name of target ELF file. Signed-off-by: Victor Kamensky --- tools/perf/util/dso.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/tools/perf/util/dso.c b/tools/perf/util/dso.c index 45be944..ca8d8d5 100644 --- a/tools/perf/util/dso.c +++ b/tools/perf/util/dso.c @@ -45,13 +45,13 @@ int dso__read_binary_type_filename(const struct dso *dso, case DSO_BINARY_TYPE__DEBUGLINK: { char *debuglink; - strncpy(filename, dso->long_name, size); - debuglink = filename + dso->long_name_len; + len = __symbol__join_symfs(filename, size, dso->long_name); + debuglink = filename + len; while (debuglink != filename && *debuglink != '/') debuglink--; if (*debuglink == '/') debuglink++; - ret = filename__read_debuglink(dso->long_name, debuglink, + ret = filename__read_debuglink(filename, debuglink, size - (debuglink - filename)); } break;