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 Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id ECBCDC433F5 for ; Sat, 18 Dec 2021 11:36:51 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender: Content-Transfer-Encoding:Content-Type:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:References: Message-ID:Subject:Cc:To:From:Date:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=9B5NE70+zGbih7/UHH0/M70Kjaz5/NLw0caJBV7ELQQ=; b=pSOBas8e/TnxSK 05QVuCqafJ/nprH5NJR161pGI5KbgAE0HHlzWkyJBPYlBbsCfU2XERRYyL68EvgAUHwhOvLP1M9x1 6bzrkGcA8vre4eMtXWVlHVk6pT6yst1fkmKkOCGdSle10vP53LhO1yRm98wlaXlvtmSotPJ+GBl7z D3yJ4y7e7FTSw3xw2/H+BDXhpnLAYogCG2cvOJHfvDzYd8WoFjMXJ7oyzk3forApG/CJ+aWWh+s0C OdCDrlhseq9Kz62AMv/wkCGKv2iKyfTgxXhXGb0gQL/xSWELVr3phsfFmwm9pBjafMUG3OdPtAlUo RZdGeO7bXsNxZKrefxVw==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1myXzc-00DiLa-Kh; Sat, 18 Dec 2021 11:35:12 +0000 Received: from casper.infradead.org ([2001:8b0:10b:1236::1]) by bombadil.infradead.org with esmtps (Exim 4.94.2 #2 (Red Hat Linux)) id 1myXza-00DiLS-En for linux-arm-kernel@bombadil.infradead.org; Sat, 18 Dec 2021 11:35:10 +0000 DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=infradead.org; s=casper.20170209; h=In-Reply-To:Content-Type:MIME-Version: References:Message-ID:Subject:Cc:To:From:Date:Sender:Reply-To: Content-Transfer-Encoding:Content-ID:Content-Description; bh=8LMWR1IbT4P0AbF+LKQHTZq6HF8id+LfbL7FZwDEVfQ=; b=JqoirCzfnu7yUfoivO28+UzgPc qrsPtyZnTymeqYq3IiE6y49Yi019ryxIPkzDJbDqUn3BUvxMN34wBcojtJPiPW1Efra7F7dCo6Pqc 3DW5IfrGMAkAv2s2UwOGj4L/ZHZyftYr8e5kLmuUyNQh3HIZXTwt2Kb8p8E2qIWPJ8saQu0e1K+wF jOT7Tx+9l5fzdnfnCUz2X6NhpgxGA2XCI1bBRRiP3drKhI0P2mDamhSEVGwcPCxAyafw6imT5COUp sCFdSyxMgio/R6N5CzA9CNjJl+NtGMZ5oEAq/0GCGd1LjV6shUZVi21ljFUJWstZEuAN0qZ7fIjgP B30RrbJQ==; Received: from [179.97.37.151] (helo=quaco.ghostprotocols.net) by casper.infradead.org with esmtpsa (Exim 4.94.2 #2 (Red Hat Linux)) id 1myXzX-00HXN2-U1 for linux-arm-kernel@lists.infradead.org; Sat, 18 Dec 2021 11:35:09 +0000 Received: by quaco.ghostprotocols.net (Postfix, from userid 1000) id E701C40B92; Sat, 18 Dec 2021 08:35:05 -0300 (-03) Date: Sat, 18 Dec 2021 08:35:05 -0300 From: Arnaldo Carvalho de Melo To: James Clark Cc: German Gomez , linux-kernel@vger.kernel.org, linux-perf-users@vger.kernel.org, Alexandre Truong , John Garry , Will Deacon , Mathieu Poirier , Leo Yan , Mark Rutland , Alexander Shishkin , Jiri Olsa , Namhyung Kim , linux-arm-kernel@lists.infradead.org Subject: Re: [PATCH v5 6/6] perf arm64: inject missing frames if perf-record used "--call-graph=fp" Message-ID: References: <20211217154521.80603-1-german.gomez@arm.com> <20211217154521.80603-7-german.gomez@arm.com> <207eade3-6719-f028-22a1-d050c10288e0@arm.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <207eade3-6719-f028-22a1-d050c10288e0@arm.com> X-Url: http://acmel.wordpress.com X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org Em Fri, Dec 17, 2021 at 04:01:38PM +0000, James Clark escreveu: > > > On 17/12/2021 15:45, German Gomez wrote: > > From: Alexandre Truong > > > > When unwinding using frame pointers on ARM64, the return address of the > > current function may not have been pushed into the stack when a function > > was interrupted, which makes perf show an incorrect call graph to the > > user. > > > > Consider the following example program: > > > > void leaf() { > > /* long computation */ > > } > > > > void parent() { > > // (1) > > leaf(); > > // (2) > > } > > > > ... could be compiled into (using gcc -fno-inline -fno-omit-frame-pointer): > > > > leaf: > > /* long computation */ > > nop > > ret > > parent: > > // (1) > > stp x29, x30, [sp, -16]! > > mov x29, sp > > bl parent > > nop > > ldp x29, x30, [sp], 16 > > // (2) > > ret > > > > If the program is interrupted at (1), (2), or any point in "leaf:", the > > call graph will skip the callers of the current function. We can unwind > > using the dwarf info and check if the return addr is the same as the LR > > register, and inject the missing frame into the call graph. > > > > Before this patch, the above example shows the following call-graph when > > recording using "--call-graph fp" mode in ARM64: > > > > # Children Self Command Shared Object Symbol > > # ........ ........ ........ ................ ...................... > > # > > 99.86% 99.86% program3 program3 [.] leaf > > | > > ---_start > > __libc_start_main > > main > > leaf > > > > As can be seen, the "parent" function is missing. This is specially > > problematic in "leaf" because for leaf functions the compiler may always > > omit pushing the return addr into the stack. After this patch, it shows > > the correct graph: > > > > # Children Self Command Shared Object Symbol > > # ........ ........ ........ ................ ...................... > > # > > 99.86% 99.86% program3 program3 [.] leaf > > | > > ---_start > > __libc_start_main > > main > > parent > > leaf > > > > Signed-off-by: Alexandre Truong > > Signed-off-by: German Gomez > > --- > > tools/perf/util/Build | 1 + > > .../util/arm64-frame-pointer-unwind-support.c | 63 +++++++++++++++++++ > > .../util/arm64-frame-pointer-unwind-support.h | 10 +++ > > tools/perf/util/machine.c | 19 ++++-- > > tools/perf/util/machine.h | 1 + > > 5 files changed, 89 insertions(+), 5 deletions(-) > > create mode 100644 tools/perf/util/arm64-frame-pointer-unwind-support.c > > create mode 100644 tools/perf/util/arm64-frame-pointer-unwind-support.h > > > > diff --git a/tools/perf/util/Build b/tools/perf/util/Build > > index 2e5bfbb69960..03d4c647bd86 100644 > > --- a/tools/perf/util/Build > > +++ b/tools/perf/util/Build > > @@ -1,3 +1,4 @@ > > +perf-y += arm64-frame-pointer-unwind-support.o > > perf-y += annotate.o > > perf-y += block-info.o > > perf-y += block-range.o > > diff --git a/tools/perf/util/arm64-frame-pointer-unwind-support.c b/tools/perf/util/arm64-frame-pointer-unwind-support.c > > new file mode 100644 > > index 000000000000..4f5ecf51ed38 > > --- /dev/null > > +++ b/tools/perf/util/arm64-frame-pointer-unwind-support.c > > @@ -0,0 +1,63 @@ > > +// SPDX-License-Identifier: GPL-2.0 > > +#include "arm64-frame-pointer-unwind-support.h" > > +#include "callchain.h" > > +#include "event.h" > > +#include "perf_regs.h" // SMPL_REG_MASK > > +#include "unwind.h" > > + > > +#define perf_event_arm_regs perf_event_arm64_regs > > +#include "../arch/arm64/include/uapi/asm/perf_regs.h" > > +#undef perf_event_arm_regs > > + > > +struct entries { > > + u64 stack[2]; > > + size_t length; > > +}; > > + > > +static bool get_leaf_frame_caller_enabled(struct perf_sample *sample) > > +{ > > + return callchain_param.record_mode == CALLCHAIN_FP && sample->user_regs.regs > > + && sample->user_regs.mask & SMPL_REG_MASK(PERF_REG_ARM64_LR); > > +} > > + > > +static int add_entry(struct unwind_entry *entry, void *arg) > > +{ > > + struct entries *entries = arg; > > + > > + entries->stack[entries->length++] = entry->ip; > > + return 0; > > +} > > + > > +u64 get_leaf_frame_caller_aarch64(struct perf_sample *sample, struct thread *thread, int usr_idx) > > +{ > > + int ret; > > + struct entries entries = {}; > > + struct regs_dump old_regs = sample->user_regs; > > + > > + if (!get_leaf_frame_caller_enabled(sample)) > > + return 0; > > + > > + /* > > + * If PC and SP are not recorded, get the value of PC from the stack > > + * and set its mask. SP is not used when doing the unwinding but it > > + * still needs to be set to prevent failures. > > + */ > > + > > + if (!(sample->user_regs.mask & SMPL_REG_MASK(PERF_REG_ARM64_PC))) { > > + sample->user_regs.cache_mask |= SMPL_REG_MASK(PERF_REG_ARM64_PC); > > + sample->user_regs.cache_regs[PERF_REG_ARM64_PC] = sample->callchain->ips[usr_idx+1]; > > + } > > + > > + if (!(sample->user_regs.mask & SMPL_REG_MASK(PERF_REG_ARM64_SP))) { > > + sample->user_regs.cache_mask |= SMPL_REG_MASK(PERF_REG_ARM64_SP); > > + sample->user_regs.cache_regs[PERF_REG_ARM64_SP] = 0; > > + } > > + > > + ret = unwind__get_entries(add_entry, &entries, thread, sample, 2); > > + sample->user_regs = old_regs; > > + > > + if (ret || entries.length != 2) > > + return ret; > > + > > + return callchain_param.order == ORDER_CALLER ? entries.stack[0] : entries.stack[1]; > > +} > > diff --git a/tools/perf/util/arm64-frame-pointer-unwind-support.h b/tools/perf/util/arm64-frame-pointer-unwind-support.h > > new file mode 100644 > > index 000000000000..32af9ce94398 > > --- /dev/null > > +++ b/tools/perf/util/arm64-frame-pointer-unwind-support.h > > @@ -0,0 +1,10 @@ > > +/* SPDX-License-Identifier: GPL-2.0 */ > > +#ifndef __PERF_ARM_FRAME_POINTER_UNWIND_SUPPORT_H > > +#define __PERF_ARM_FRAME_POINTER_UNWIND_SUPPORT_H > > + > > +#include "event.h" > > +#include "thread.h" > > + > > +u64 get_leaf_frame_caller_aarch64(struct perf_sample *sample, struct thread *thread, int user_idx); > > + > > +#endif /* __PERF_ARM_FRAME_POINTER_UNWIND_SUPPORT_H */ > > diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c > > index 3eddad009f78..a00fd6796b35 100644 > > --- a/tools/perf/util/machine.c > > +++ b/tools/perf/util/machine.c > > @@ -34,6 +34,7 @@ > > #include "bpf-event.h" > > #include // page_size > > #include "cgroup.h" > > +#include "arm64-frame-pointer-unwind-support.h" > > > > #include > > #include > > @@ -2710,10 +2711,13 @@ static int find_prev_cpumode(struct ip_callchain *chain, struct thread *thread, > > return err; > > } > > > > -static u64 get_leaf_frame_caller(struct perf_sample *sample __maybe_unused, > > - struct thread *thread __maybe_unused, int usr_idx __maybe_unused) > > +static u64 get_leaf_frame_caller(struct perf_sample *sample, > > + struct thread *thread, int usr_idx) > > { > > - return 0; > > + if (machine__normalize_is(thread->maps->machine, "arm64")) > > + return get_leaf_frame_caller_aarch64(sample, thread, usr_idx); > > + else > > + return 0; > > } > > > > static int thread__resolve_callchain_sample(struct thread *thread, > > @@ -3114,14 +3118,19 @@ int machine__set_current_tid(struct machine *machine, int cpu, pid_t pid, > > } > > > > /* > > - * Compares the raw arch string. N.B. see instead perf_env__arch() if a > > - * normalized arch is needed. > > + * Compares the raw arch string. N.B. see instead perf_env__arch() or > > + * machine__normalize_is() if a normalized arch is needed. > > */ > > bool machine__is(struct machine *machine, const char *arch) > > { > > return machine && !strcmp(perf_env__raw_arch(machine->env), arch); > > } > > > > +bool machine__normalize_is(struct machine *machine, const char *arch) > > +{ > > + return machine && !strcmp(perf_env__arch(machine->env), arch); > > +} > > + > > I think this function name would be clearer as something like "machine__normalized_is" or > "machine__normalized_arch_is". The tense is slightly off because it's a test rather than a > verb. Agreed, its a question, not a command. - Arnaldo > With that change, for the whole set: > > Reviewed-by: James Clark > > > > int machine__nr_cpus_avail(struct machine *machine) > > { > > return machine ? perf_env__nr_cpus_avail(machine->env) : 0; > > diff --git a/tools/perf/util/machine.h b/tools/perf/util/machine.h > > index a143087eeb47..665535153411 100644 > > --- a/tools/perf/util/machine.h > > +++ b/tools/perf/util/machine.h > > @@ -208,6 +208,7 @@ static inline bool machine__is_host(struct machine *machine) > > } > > > > bool machine__is(struct machine *machine, const char *arch); > > +bool machine__normalize_is(struct machine *machine, const char *arch); > > int machine__nr_cpus_avail(struct machine *machine); > > > > struct thread *__machine__findnew_thread(struct machine *machine, pid_t pid, pid_t tid); > > -- - Arnaldo _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel