linux-kernel - Re: [PATCH 1/1] powerpc/perf: Adjust callchain based on DWARF debug

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20140522100045.GA4789@krava.brq.redhat.com>
Date:	Thu, 22 May 2014 12:00:45 +0200
From:	Jiri Olsa <jolsa@...hat.com>
To:	Sukadev Bhattiprolu <sukadev@...ux.vnet.ibm.com>
Cc:	Arnaldo Carvalho de Melo <acme@...nel.org>,
	Maynard Johnson <mpjohn@...ibm.com>, Ulrich.Weigand@...ibm.com,
	Anton Blanchard <anton@....ibm.com>, linuxppc-dev@...abs.org,
	linux-kernel@...r.kernel.org, michael@...erman.id.au
Subject: Re: [PATCH 1/1] powerpc/perf: Adjust callchain based on DWARF debug

On Tue, May 20, 2014 at 06:26:44PM -0700, Sukadev Bhattiprolu wrote:

SNIP

> + *
> + * The value in LR is only needed when it holds a return address. If the
> + * return address is on the stack, we should ignore the LR value.
> + *
> + * Further, when the return address is in the LR, if a new frame was just
> + * allocated but the LR was not saved into it, then the LR contains the
> + * caller, slot 4: contains the caller's caller and the contents of slot 3:
> + * (chain->ips[3]) is undefined and must be ignored.
> + *
> + * Use DWARF debug information to determine if any entries need to be skipped.
> + *
> + * Return:
> + *	index:	of callchain entry that needs to be ignored (if any)
> + *	-1	if no entry needs to be ignored or in case of errors
> + *
> + * TODO:
> + *	Rather than returning an index into the callchain and have the
> + *	caller skip that entry, we could modify the callchain in-place
> + *	by putting a PERF_CONTEXT_IGNORE marker in the affected entry.
> + *
> + *	But @chain points to read-only mmap, so the caller needs to
> + *	duplicate the callchain to modify in-place - something like:
> + *
> + *		new_callchain = arch_duplicate_callchain();
> + *		arch_adjust_callchain(new_callchain);
> + *		...
> + *		arch_free_callchain(new_callchain);
> + *
> + *	Since we only expect to adjust <= 1 entry for now, just return
> + *	the index.

yep, that sounds more clear to me.. something like below?

calling callchain_dup from within arch_adjust_callchain in case
you want to change it and returning != 0 in this case, so
we could free the new callchain

but it might be to much overhead in case you have the support to skip
just one entry now right? is there a plan for more cleaning? ;)

thanks,
jirka


---
diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c
index 7409ac8..ec18310 100644
--- a/tools/perf/util/machine.c
+++ b/tools/perf/util/machine.c
@@ -1279,6 +1279,11 @@ struct branch_info *sample__resolve_bstack(struct perf_sample *sample,
 	return bi;
 }
 
+struct ip_callchain *callchain_dup(struct ip_callchain *chain)
+{
+	return memdup(chain, sizeof(*chain) + chain->nr * sizeof(u64));
+}
+
 static int machine__resolve_callchain_sample(struct machine *machine,
 					     struct thread *thread,
 					     struct ip_callchain *chain,
@@ -1298,6 +1303,8 @@ static int machine__resolve_callchain_sample(struct machine *machine,
 		return 0;
 	}
 
+	adjusted = arch_adjust_callchain(machine, thread, &chain);
+
 	for (i = 0; i < chain_nr; i++) {
 		u64 ip;
 		struct addr_location al;
@@ -1326,7 +1333,7 @@ static int machine__resolve_callchain_sample(struct machine *machine,
 				 * Discard all.
 				 */
 				callchain_cursor_reset(&callchain_cursor);
-				return 0;
+				goto out;
 			}
 			continue;
 		}
@@ -1350,10 +1357,14 @@ static int machine__resolve_callchain_sample(struct machine *machine,
 		err = callchain_cursor_append(&callchain_cursor,
 					      ip, al.map, al.sym);
 		if (err)
-			return err;
+			goto out;
 	}
 
-	return 0;
+ out:
+	if (adjusted)
+		free(chain);
+
+	return err;
 }
 
 static int unwind_entry(struct unwind_entry *entry, void *arg)
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/