Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751020Ab0DJEU7 (ORCPT ); Sat, 10 Apr 2010 00:20:59 -0400 Received: from mx1.redhat.com ([209.132.183.28]:27053 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750838Ab0DJEU5 (ORCPT ); Sat, 10 Apr 2010 00:20:57 -0400 Message-ID: <4BBFFC86.4020201@redhat.com> Date: Sat, 10 Apr 2010 00:20:22 -0400 From: Masami Hiramatsu User-Agent: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.1.9) Gecko/20100330 Fedora/3.0.4-1.fc11 Thunderbird/3.0.4 MIME-Version: 1.0 To: Arnaldo Carvalho de Melo CC: Ingo Molnar , lkml , systemtap , DLE , Paul Mackerras , Peter Zijlstra , Mike Galbraith , Frederic Weisbecker Subject: Re: [PATCH -tip v2 7/8] perf probe: Remove die() from probe-finder code References: <20100406220542.13329.45837.stgit@localhost6.localdomain6> <20100406220629.13329.71869.stgit@localhost6.localdomain6> <4BBFB5CE.7070104@redhat.com> <20100410012815.GC16721@ghostprotocols.net> In-Reply-To: <20100410012815.GC16721@ghostprotocols.net> X-Enigmail-Version: 1.0.1 Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 5819 Lines: 168 Arnaldo Carvalho de Melo wrote: > Em Fri, Apr 09, 2010 at 07:18:38PM -0400, Masami Hiramatsu escreveu: >> Hi Arnaldo, >> >> Has this code done what you suggested? :) >> I'd like to have your comment. > > It improves the current situation, yes, but there are still cases there > where die() is called, I assume that is left for later, right? With the next (8/8) patch, all die()s are removed at least from probe-{event,finder}.c, except for all x*() wrappers. > > More comments below. > >> Thank you, >> >> Masami Hiramatsu wrote: >>> Remove die() and DIE_IF() code from util/probe-finder.c since >>> these 'sudden death' in utility functions make reusing it from >>> other code (especially tui/gui) difficult. >>> >>> Signed-off-by: Masami Hiramatsu >>> Cc: Ingo Molnar >>> Cc: Paul Mackerras >>> Cc: Arnaldo Carvalho de Melo >>> Cc: Peter Zijlstra >>> Cc: Mike Galbraith >>> Cc: Frederic Weisbecker >>> --- >>> >>> tools/perf/util/probe-event.c | 4 >>> tools/perf/util/probe-finder.c | 517 +++++++++++++++++++++++++--------------- >>> 2 files changed, 322 insertions(+), 199 deletions(-) >>> >>> diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c >>> index bef2805..7893b32 100644 >>> --- a/tools/perf/util/probe-event.c >>> +++ b/tools/perf/util/probe-event.c >>> @@ -151,10 +151,10 @@ static int try_to_find_kprobe_trace_events(struct perf_probe_event *pev, >>> >>> /* Error path */ >>> if (need_dwarf) { >>> - if (ntevs == -ENOENT) >>> + if (ntevs == -EBADF) >>> pr_warning("No dwarf info found in the vmlinux - " >>> "please rebuild with CONFIG_DEBUG_INFO=y.\n"); >>> - die("Could not analyze debuginfo."); >>> + die("Failed to analyze debuginfo."); > > Like here, the TUI/GUI can try to add a probe but if it fails it can > still continue providing things like a "perf top" window, analysing > perf.data files, doing 'perf diffs', etc. Sure, this die() is removed by next (8/8) patch. Sorry, I've split it because of easy to review... >>> } >>> pr_debug("An error occurred in debuginfo analysis." >>> " Try to use symbols.\n"); >>> diff --git a/tools/perf/util/probe-finder.c b/tools/perf/util/probe-finder.c >>> index 1f45285..4a57504 100644 >>> --- a/tools/perf/util/probe-finder.c >>> +++ b/tools/perf/util/probe-finder.c >>> @@ -196,19 +196,7 @@ static bool die_compare_name(Dwarf_Die *dw_die, const char *tname) >>> { >>> const char *name; >>> name = dwarf_diename(dw_die); >>> - DIE_IF(name == NULL); >>> - return strcmp(tname, name); >>> -} >>> - >>> -/* Get entry pc(or low pc, 1st entry of ranges) of the die */ >>> -static Dwarf_Addr die_get_entrypc(Dwarf_Die *dw_die) >>> -{ >>> - Dwarf_Addr epc; >>> - int ret; >>> - >>> - ret = dwarf_entrypc(dw_die, &epc); >>> - DIE_IF(ret == -1); >>> - return epc; >>> + return name ? strcmp(tname, name) : -1; > > This change is correct, with the stated intent :-) > >>> } >>> >>> /* Get type die, but skip qualifiers and typedef */ >>> @@ -390,7 +378,7 @@ static Dwarf_Die *die_find_member(Dwarf_Die *st_die, const char *name, >>> */ >>> >>> /* Show a location */ >>> -static void convert_location(Dwarf_Op *op, struct probe_finder *pf) >>> +static int convert_location(Dwarf_Op *op, struct probe_finder *pf) > > Yeah, there are still lots of places, in other areas that don't return a > status, just printing a warning and silently failing, like in the trace > parsing routines > >>> { >>> unsigned int regn; >>> Dwarf_Word offs = 0; >>> @@ -400,8 +388,11 @@ static void convert_location(Dwarf_Op *op, struct probe_finder *pf) >>> >>> /* If this is based on frame buffer, set the offset */ >>> if (op->atom == DW_OP_fbreg) { >>> - if (pf->fb_ops == NULL) >>> - die("The attribute of frame base is not supported.\n"); >>> + if (pf->fb_ops == NULL) { >>> + pr_warning("The attribute of frame base is not " >>> + "supported.\n"); >>> + return -ENOTSUP; >>> + } > > Correct, if you look at tools/perf/util/debug.c, eprintf() looks if > we're in TUI mode, use_browser is true and either fprintfs(stderr) if > not or calls a routine to put it at the "status line" (bottom) in the > TUI. > > Having a list with the last messages so that we can have a log window, > or keeping it in a log file that would then be browser is an enhancement > to be made. > >>> ref = true; >>> offs = op->number; >>> op = &pf->fb_ops[0]; >>> @@ -419,50 +410,63 @@ static void convert_location(Dwarf_Op *op, struct probe_finder *pf) >>> ref = true; >>> } else if (op->atom == DW_OP_regx) { >>> regn = op->number; >>> - } else >>> - die("DW_OP %d is not supported.", op->atom); >>> + } else { >>> + pr_warning("DW_OP %d is not supported.\n", op->atom); >>> + return -ENOTSUP; >>> + } > > correct > >>> regs = get_arch_regstr(regn); >>> - if (!regs) >>> - die("%u exceeds max register number.", regn); >>> + if (!regs) { >>> + pr_warning("%u exceeds max register number.\n", regn); >>> + return -ERANGE; >>> + } >>> >>> tvar->value = xstrdup(regs); >>> if (ref) { >>> tvar->ref = xzalloc(sizeof(struct kprobe_trace_arg_ref)); > > We have to kill those xzcalloc, etc, too they are die() in disguise :-) Hmm, I think that will cost high, because only failing to allocate memory, which theoretically means we can't continue to operate it. In that case, we'd better just use backtrace() and die(). Thank you, -- Masami Hiramatsu e-mail: mhiramat@redhat.com -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/