Re: [PATCH V7 15/15] perf script: Show text poke address symbol

From: Arnaldo Carvalho de Melo <arnaldo.melo@gmail.com>
To: Adrian Hunter <adrian.hunter@intel.com>
Cc: Peter Zijlstra <peterz@infradead.org>,
	Ingo Molnar <mingo@redhat.com>,
	Masami Hiramatsu <mhiramat@kernel.org>,
	Steven Rostedt <rostedt@goodmis.org>,
	Borislav Petkov <bp@alien8.de>, "H . Peter Anvin" <hpa@zytor.com>,
	x86@kernel.org, Mark Rutland <mark.rutland@arm.com>,
	Alexander Shishkin <alexander.shishkin@linux.intel.com>,
	Mathieu Poirier <mathieu.poirier@linaro.org>,
	Leo Yan <leo.yan@linaro.org>, Jiri Olsa <jolsa@redhat.com>,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH V7 15/15] perf script: Show text poke address symbol
Date: Tue, 19 May 2020 22:38:20 -0300	[thread overview]
Message-ID: <20200520013820.GE28228@kernel.org> (raw)
In-Reply-To: <20200512121922.8997-16-adrian.hunter@intel.com>

Em Tue, May 12, 2020 at 03:19:22PM +0300, Adrian Hunter escreveu:
> It is generally more useful to show the symbol with an address. In this
> case, the print function requires the 'machine' which means changing
> callers to provide it as a parameter. It is optional because most events
> do not need it and the callers that matter can provide it.
> 
> Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
> ---
>  tools/perf/builtin-script.c    |  4 ++--
>  tools/perf/tests/perf-record.c |  4 ++--
>  tools/perf/util/event.c        | 23 ++++++++++++++++++-----
>  tools/perf/util/event.h        |  6 ++++--
>  tools/perf/util/intel-pt.c     |  2 +-
>  tools/perf/util/machine.c      |  2 +-
>  6 files changed, 28 insertions(+), 13 deletions(-)
> 
> diff --git a/tools/perf/builtin-script.c b/tools/perf/builtin-script.c
> index 35ed8f69ed6a..5bdd1a393399 100644
> --- a/tools/perf/builtin-script.c
> +++ b/tools/perf/builtin-script.c
> @@ -2170,7 +2170,7 @@ static int print_event_with_time(struct perf_tool *tool,
>  					   event->header.type, stdout);
>  	}
>  
> -	perf_event__fprintf(event, stdout);
> +	perf_event__fprintf(machine, event, stdout);

Since this is for a perf event, the first arg should be event, not
machine, but I can fixup this when processing these patches, no need to
resend.

>  	thread__put(thread);
>  
> @@ -2305,7 +2305,7 @@ process_finished_round_event(struct perf_tool *tool __maybe_unused,
>  			     struct ordered_events *oe __maybe_unused)
>  
>  {
> -	perf_event__fprintf(event, stdout);
> +	perf_event__fprintf(NULL, event, stdout);
>  	return 0;
>  }
>  
> diff --git a/tools/perf/tests/perf-record.c b/tools/perf/tests/perf-record.c
> index 83adfd846ccd..ff6febf15269 100644
> --- a/tools/perf/tests/perf-record.c
> +++ b/tools/perf/tests/perf-record.c
> @@ -185,14 +185,14 @@ int test__PERF_RECORD(struct test *test __maybe_unused, int subtest __maybe_unus
>  				err = perf_evlist__parse_sample(evlist, event, &sample);
>  				if (err < 0) {
>  					if (verbose > 0)
> -						perf_event__fprintf(event, stderr);
> +						perf_event__fprintf(NULL, event, stderr);
>  					pr_debug("Couldn't parse sample\n");
>  					goto out_delete_evlist;
>  				}
>  
>  				if (verbose > 0) {
>  					pr_info("%" PRIu64" %d ", sample.time, sample.cpu);
> -					perf_event__fprintf(event, stderr);
> +					perf_event__fprintf(NULL, event, stderr);
>  				}
>  
>  				if (prev_time > sample.time) {
> diff --git a/tools/perf/util/event.c b/tools/perf/util/event.c
> index dde2f0147713..4b60cd57ad62 100644
> --- a/tools/perf/util/event.c
> +++ b/tools/perf/util/event.c
> @@ -440,14 +440,26 @@ static int text_poke_printer(enum binary_printer_ops op, unsigned int val,
>  	}
>  }
>  
> -size_t perf_event__fprintf_text_poke(union perf_event *event, FILE *fp)
> +size_t perf_event__fprintf_text_poke(struct machine *machine,
> +				     union perf_event *event, FILE *fp)

Ditto

>  {
>  	struct perf_record_text_poke_event *tp = &event->text_poke;
>  	size_t ret;
>  	bool old;
>  
> -	ret = fprintf(fp, " %" PRI_lx64 " old len %u new len %u\n",
> -		      tp->addr, tp->old_len, tp->new_len);
> +	ret = fprintf(fp, " %" PRI_lx64 " ", tp->addr);
> +	if (machine) {
> +		struct addr_location al;
> +
> +		al.map = maps__find(&machine->kmaps, tp->addr);
> +		if (al.map && map__load(al.map) >= 0) {
> +			al.addr = al.map->map_ip(al.map, tp->addr);
> +			al.sym = map__find_symbol(al.map, al.addr);
> +			if (al.sym)
> +				ret += symbol__fprintf_symname_offs(al.sym, &al, fp);
> +		}
> +	}
> +	ret += fprintf(fp, " old len %u new len %u\n", tp->old_len, tp->new_len);
>  	old = true;
>  	ret += binary__fprintf(tp->bytes, tp->old_len, 16, text_poke_printer,
>  			       &old, fp);
> @@ -457,7 +469,8 @@ size_t perf_event__fprintf_text_poke(union perf_event *event, FILE *fp)
>  	return ret;
>  }
>  
> -size_t perf_event__fprintf(union perf_event *event, FILE *fp)
> +size_t perf_event__fprintf(struct machine *machine, union perf_event *event,
> +			   FILE *fp)
>  {
>  	size_t ret = fprintf(fp, "PERF_RECORD_%s",
>  			     perf_event__name(event->header.type));
> @@ -502,7 +515,7 @@ size_t perf_event__fprintf(union perf_event *event, FILE *fp)
>  		ret += perf_event__fprintf_bpf(event, fp);
>  		break;
>  	case PERF_RECORD_TEXT_POKE:
> -		ret += perf_event__fprintf_text_poke(event, fp);
> +		ret += perf_event__fprintf_text_poke(machine, event, fp);
>  		break;
>  	default:
>  		ret += fprintf(fp, "\n");
> diff --git a/tools/perf/util/event.h b/tools/perf/util/event.h
> index dbbba6dea51d..611c8e5ad9d9 100644
> --- a/tools/perf/util/event.h
> +++ b/tools/perf/util/event.h
> @@ -389,8 +389,10 @@ size_t perf_event__fprintf_namespaces(union perf_event *event, FILE *fp);
>  size_t perf_event__fprintf_cgroup(union perf_event *event, FILE *fp);
>  size_t perf_event__fprintf_ksymbol(union perf_event *event, FILE *fp);
>  size_t perf_event__fprintf_bpf(union perf_event *event, FILE *fp);
> -size_t perf_event__fprintf_text_poke(union perf_event *event, FILE *fp);
> -size_t perf_event__fprintf(union perf_event *event, FILE *fp);
> +size_t perf_event__fprintf_text_poke(struct machine *machine,
> +				     union perf_event *event, FILE *fp);
> +size_t perf_event__fprintf(struct machine *machine, union perf_event *event,
> +			   FILE *fp);
>  
>  int kallsyms__get_function_start(const char *kallsyms_filename,
>  				 const char *symbol_name, u64 *addr);
> diff --git a/tools/perf/util/intel-pt.c b/tools/perf/util/intel-pt.c
> index e2e199a58222..4c6bb45d8200 100644
> --- a/tools/perf/util/intel-pt.c
> +++ b/tools/perf/util/intel-pt.c
> @@ -236,7 +236,7 @@ static void intel_pt_log_event(union perf_event *event)
>  	if (!intel_pt_enable_logging || !f)
>  		return;
>  
> -	perf_event__fprintf(event, f);
> +	perf_event__fprintf(NULL, event, f);
>  }
>  
>  static void intel_pt_dump_sample(struct perf_session *session,
> diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c
> index 9120d7dc274f..5f52b579b3fe 100644
> --- a/tools/perf/util/machine.c
> +++ b/tools/perf/util/machine.c
> @@ -821,7 +821,7 @@ int machine__process_text_poke(struct machine *machine, union perf_event *event,
>  	u8 cpumode = event->header.misc & PERF_RECORD_MISC_CPUMODE_MASK;
>  
>  	if (dump_trace)
> -		perf_event__fprintf_text_poke(event, stdout);
> +		perf_event__fprintf_text_poke(machine, event, stdout);
>  
>  	if (!event->text_poke.new_len)
>  		return 0;
> -- 
> 2.17.1
> 

-- 

- Arnaldo