[RFC PATCH 0/6] perf tools: Warning fixes

[RFC PATCH 0/6] perf tools: Warning fixes

[James Clark]

I'm submitting this as an RFC because there are a few changes I'd
like to get feedback on. The two changes I wanted to make were the
last two for Coresight warnings, but I ended up making some perf-wide
changes along the way.

For #1 (perf tools: Add WARN_ONCE equivalent for UI warnings)
 * Does it make sense to add warn once equivalents at all, or
   should the once be re-done for each usage?
 * Or should there be some kind of generic 'once' wrapper?

For #3 (perf tools: Add disassembly warnings for annotate --stdio)
 * If the output is interpreted by any other tools, then adding
   these warnings could be an issue, so maybe this change could
   be dropped, but no error output at all isn't ideal.

For #4 (perf tools: Add flag for tracking warnings of missing DSOs)
 * In theory I could re-use 'annotate_warned', but it might make sense
   to rename it in that case, or just leave the new auxtrace_warned and
   remove any confusion.

This set applies to perf/core e73f0f0ee7541

Thanks
James

James Clark (6):
  perf tools: Add WARN_ONCE equivalent for UI warnings
  perf tools: Re-add annotate_warned functionality
  perf tools: Add disassembly warnings for annotate --stdio
  perf tools: Add flag for tracking warnings of missing DSOs
  perf cs-etm: Improve Coresight zero timestamp warning
  perf cs-etm: Add warnings for missing DSOs

 tools/perf/ui/browsers/annotate.c             |  1 +
 tools/perf/ui/gtk/annotate.c                  |  1 +
 tools/perf/util/annotate.c                    | 20 +++++++++++++++++--
 .../perf/util/cs-etm-decoder/cs-etm-decoder.c |  7 +++++--
 tools/perf/util/cs-etm.c                      | 10 +++++++++-
 tools/perf/util/debug.h                       | 14 +++++++++++++
 tools/perf/util/dso.h                         |  1 +
 7 files changed, 49 insertions(+), 5 deletions(-)

-- 
2.28.0

[RFC PATCH 1/6] perf tools: Add WARN_ONCE equivalent for UI warnings

[James Clark]

Currently WARN_ONCE prints to stderr and corrupts the TUI. Add
equivalent methods for UI warnings.

Signed-off-by: James Clark <james.clark@arm.com>
---
 tools/perf/util/debug.h | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/tools/perf/util/debug.h b/tools/perf/util/debug.h
index 48f631966067..f99468a7f681 100644
--- a/tools/perf/util/debug.h
+++ b/tools/perf/util/debug.h
@@ -22,6 +22,13 @@ extern int debug_data_convert;
 	eprintf(0, verbose, pr_fmt(fmt), ##__VA_ARGS__)
 #define pr_warning(fmt, ...) \
 	eprintf(0, verbose, pr_fmt(fmt), ##__VA_ARGS__)
+#define pr_warning_once(fmt, ...) ({		\
+	static int __warned;			\
+	if (unlikely(!__warned)) {		\
+		pr_warning(fmt, ##__VA_ARGS__); \
+		__warned = 1;			\
+	}					\
+})
 #define pr_info(fmt, ...) \
 	eprintf(0, verbose, pr_fmt(fmt), ##__VA_ARGS__)
 #define pr_debug(fmt, ...) \
@@ -55,6 +62,13 @@ void trace_event(union perf_event *event);
 
 int ui__error(const char *format, ...) __printf(1, 2);
 int ui__warning(const char *format, ...) __printf(1, 2);
+#define ui__warning_once(format, ...) ({		\
+	static int __warned;				\
+	if (unlikely(!__warned)) {			\
+		ui__warning(format, ##__VA_ARGS__);	\
+		__warned = 1;				\
+	}						\
+})
 
 void pr_stat(const char *fmt, ...);
 
-- 
2.28.0

[RFC PATCH 2/6] perf tools: Re-add annotate_warned functionality

[James Clark]

Setting annotate_warned to true on errors was removed in
commit ee51d851392e ("perf annotate: Introduce strerror for handling
symbol__disassemble() errors") which means when 'perf annotate
--skip-missing' is used warnings are shown multiple times for the same
DSO.

Setting this again restores the original behavior of only one warning
each.

Signed-off-by: James Clark <james.clark@arm.com>
---
 tools/perf/ui/browsers/annotate.c | 1 +
 tools/perf/ui/gtk/annotate.c      | 1 +
 2 files changed, 2 insertions(+)

diff --git a/tools/perf/ui/browsers/annotate.c b/tools/perf/ui/browsers/annotate.c
index 701130ad43a2..ef4da4295bf7 100644
--- a/tools/perf/ui/browsers/annotate.c
+++ b/tools/perf/ui/browsers/annotate.c
@@ -966,6 +966,7 @@ int symbol__tui_annotate(struct map_symbol *ms, struct evsel *evsel,
 	err = symbol__annotate2(ms, evsel, opts, &browser.arch);
 	if (err) {
 		char msg[BUFSIZ];
+		ms->map->dso->annotate_warned = true;
 		symbol__strerror_disassemble(ms, err, msg, sizeof(msg));
 		ui__error("Couldn't annotate %s:\n%s", sym->name, msg);
 		goto out_free_offsets;
diff --git a/tools/perf/ui/gtk/annotate.c b/tools/perf/ui/gtk/annotate.c
index 94167bfed722..0a50e962f9a3 100644
--- a/tools/perf/ui/gtk/annotate.c
+++ b/tools/perf/ui/gtk/annotate.c
@@ -177,6 +177,7 @@ static int symbol__gtk_annotate(struct map_symbol *ms, struct evsel *evsel,
 	err = symbol__annotate(ms, evsel, &annotation__default_options, NULL);
 	if (err) {
 		char msg[BUFSIZ];
+		ms->map->dso->annotate_warned = true;
 		symbol__strerror_disassemble(ms, err, msg, sizeof(msg));
 		ui__error("Couldn't annotate %s: %s\n", sym->name, msg);
 		return -1;
-- 
2.28.0

[RFC PATCH 3/6] perf tools: Add disassembly warnings for annotate --stdio

[James Clark]

Currently 'perf annotate --stdio' (and --stdio2) will exit without
printing anything if there are disassembly errors. Apply the same
error handler that's used for TUI and GTK modes. This makes comparing
disassembly across the different modes more consistent.

Signed-off-by: James Clark <james.clark@arm.com>
---
 tools/perf/util/annotate.c | 20 ++++++++++++++++++--
 1 file changed, 18 insertions(+), 2 deletions(-)

diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
index aa04a3655236..1ed097bcb78a 100644
--- a/tools/perf/util/annotate.c
+++ b/tools/perf/util/annotate.c
@@ -2787,9 +2787,17 @@ int symbol__tty_annotate2(struct map_symbol *ms, struct evsel *evsel,
 	struct rb_root source_line = RB_ROOT;
 	struct hists *hists = evsel__hists(evsel);
 	char buf[1024];
+	int err;
+
+	err = symbol__annotate2(ms, evsel, opts, NULL);
+	if (err) {
+		char msg[BUFSIZ];
 
-	if (symbol__annotate2(ms, evsel, opts, NULL) < 0)
+		dso->annotate_warned = true;
+		symbol__strerror_disassemble(ms, err, msg, sizeof(msg));
+		ui__error("Couldn't annotate %s:\n%s", sym->name, msg);
 		return -1;
+	}
 
 	if (opts->print_lines) {
 		srcline_full_filename = opts->full_path;
@@ -2813,9 +2821,17 @@ int symbol__tty_annotate(struct map_symbol *ms, struct evsel *evsel,
 	struct dso *dso = ms->map->dso;
 	struct symbol *sym = ms->sym;
 	struct rb_root source_line = RB_ROOT;
+	int err;
+
+	err = symbol__annotate(ms, evsel, opts, NULL);
+	if (err) {
+		char msg[BUFSIZ];
 
-	if (symbol__annotate(ms, evsel, opts, NULL) < 0)
+		dso->annotate_warned = true;
+		symbol__strerror_disassemble(ms, err, msg, sizeof(msg));
+		ui__error("Couldn't annotate %s:\n%s", sym->name, msg);
 		return -1;
+	}
 
 	symbol__calc_percent(sym, evsel);
 
-- 
2.28.0

[RFC PATCH 4/6] perf tools: Add flag for tracking warnings of missing DSOs

[James Clark]

Auxtrace support may need DSOs for decoding (for example Arm
Coresight). If one of these is missing it would make sense to
warn once for each one that's missing, but not flood the output
with every address as there could be thousands of lookups.

This flag will allow tracking whether a warning was shown for
each DSO.

Signed-off-by: James Clark <james.clark@arm.com>
---
 tools/perf/util/dso.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/tools/perf/util/dso.h b/tools/perf/util/dso.h
index 52e7101c5609..83723ba11dc8 100644
--- a/tools/perf/util/dso.h
+++ b/tools/perf/util/dso.h
@@ -170,6 +170,7 @@ struct dso {
 	u8		 has_srcline:1;
 	u8		 hit:1;
 	u8		 annotate_warned:1;
+	u8		 auxtrace_warned:1;
 	u8		 short_name_allocated:1;
 	u8		 long_name_allocated:1;
 	u8		 is_64_bit:1;
-- 
2.28.0

[RFC PATCH 5/6] perf cs-etm: Improve Coresight zero timestamp warning

[James Clark]

Only show the warning if the user hasn't already set timeless mode and
improve the text because there was ambiguity around the meaning of '...'

Change the warning to a UI warning instead of printing straight to
stderr because this corrupts the UI when perf report TUI is used. The UI
warning function also handles printing to stderr when in perf script
mode.

Suggested-by: Leo Yan <leo.yan@linaro.org>
Signed-off-by: James Clark <james.clark@arm.com>
---
 tools/perf/util/cs-etm-decoder/cs-etm-decoder.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/tools/perf/util/cs-etm-decoder/cs-etm-decoder.c b/tools/perf/util/cs-etm-decoder/cs-etm-decoder.c
index 3e1a05bc82cc..5084bd2ca6eb 100644
--- a/tools/perf/util/cs-etm-decoder/cs-etm-decoder.c
+++ b/tools/perf/util/cs-etm-decoder/cs-etm-decoder.c
@@ -324,8 +324,11 @@ cs_etm_decoder__do_hard_timestamp(struct cs_etm_queue *etmq,
 		 * underflow.
 		 */
 		packet_queue->cs_timestamp = 0;
-		WARN_ONCE(true, "Zero Coresight timestamp found at Idx:%" OCSD_TRC_IDX_STR
-				". Decoding may be improved with --itrace=Z...\n", indx);
+		if (!cs_etm__etmq_is_timeless(etmq))
+			pr_warning_once("Zero Coresight timestamp found at Idx:%" OCSD_TRC_IDX_STR
+					". Decoding may be improved by prepending 'Z' to your current --itrace arguments.\n",
+					indx);
+
 	} else if (packet_queue->instr_count > elem->timestamp) {
 		/*
 		 * Sanity check that the elem->timestamp - packet_queue->instr_count would not
-- 
2.28.0

[RFC PATCH 6/6] perf cs-etm: Add warnings for missing DSOs

[James Clark]

Currently decode will silently fail if no binary data is available for
the decode. This is made worse if only partial data is available because
the decode will appear to work, but any trace from that missing DSO will
silently not be generated.

Add a UI popup once if there is any data missing, and then warn in the
bottom left for each individual DSO that's missing.

Signed-off-by: James Clark <james.clark@arm.com>
---
 tools/perf/util/cs-etm.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/tools/perf/util/cs-etm.c b/tools/perf/util/cs-etm.c
index 32ad92d3e454..e6851260d059 100644
--- a/tools/perf/util/cs-etm.c
+++ b/tools/perf/util/cs-etm.c
@@ -746,8 +746,16 @@ static u32 cs_etm__mem_access(struct cs_etm_queue *etmq, u8 trace_chan_id,
 
 	len = dso__data_read_offset(al.map->dso, machine, offset, buffer, size);
 
-	if (len <= 0)
+	if (len <= 0) {
+		ui__warning_once("CS ETM Trace: Missing DSO. Use 'perf archive' to export data from the traced system.\n");
+		if (!al.map->dso->auxtrace_warned) {
+			pr_err("CS ETM Trace: Debug data not found for address %#"PRIx64" in %s\n",
+				    address,
+				    al.map->dso->long_name ? al.map->dso->long_name : "Unknown");
+			al.map->dso->auxtrace_warned = true;
+		}
 		return 0;
+	}
 
 	return len;
 }
-- 
2.28.0

Re: [RFC PATCH 1/6] perf tools: Add WARN_ONCE equivalent for UI warnings

[Arnaldo Carvalho de Melo]

Em Thu, Jul 29, 2021 at 04:58:00PM +0100, James Clark escreveu:
> Currently WARN_ONCE prints to stderr and corrupts the TUI. Add
> equivalent methods for UI warnings.

This one and the the next 3 seem clean and useful, applying and then
waiting for people to comment on the coresight specific ones.

- Arnaldo
 
> Signed-off-by: James Clark <james.clark@arm.com>
> ---
>  tools/perf/util/debug.h | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/tools/perf/util/debug.h b/tools/perf/util/debug.h
> index 48f631966067..f99468a7f681 100644
> --- a/tools/perf/util/debug.h
> +++ b/tools/perf/util/debug.h
> @@ -22,6 +22,13 @@ extern int debug_data_convert;
>  	eprintf(0, verbose, pr_fmt(fmt), ##__VA_ARGS__)
>  #define pr_warning(fmt, ...) \
>  	eprintf(0, verbose, pr_fmt(fmt), ##__VA_ARGS__)
> +#define pr_warning_once(fmt, ...) ({		\
> +	static int __warned;			\
> +	if (unlikely(!__warned)) {		\
> +		pr_warning(fmt, ##__VA_ARGS__); \
> +		__warned = 1;			\
> +	}					\
> +})
>  #define pr_info(fmt, ...) \
>  	eprintf(0, verbose, pr_fmt(fmt), ##__VA_ARGS__)
>  #define pr_debug(fmt, ...) \
> @@ -55,6 +62,13 @@ void trace_event(union perf_event *event);
>  
>  int ui__error(const char *format, ...) __printf(1, 2);
>  int ui__warning(const char *format, ...) __printf(1, 2);
> +#define ui__warning_once(format, ...) ({		\
> +	static int __warned;				\
> +	if (unlikely(!__warned)) {			\
> +		ui__warning(format, ##__VA_ARGS__);	\
> +		__warned = 1;				\
> +	}						\
> +})
>  
>  void pr_stat(const char *fmt, ...);
>  
> -- 
> 2.28.0
> 

-- 

- Arnaldo

Re: [RFC PATCH 6/6] perf cs-etm: Add warnings for missing DSOs

[Arnaldo Carvalho de Melo]

Em Thu, Jul 29, 2021 at 04:58:05PM +0100, James Clark escreveu:
> Currently decode will silently fail if no binary data is available for
> the decode. This is made worse if only partial data is available because
> the decode will appear to work, but any trace from that missing DSO will
> silently not be generated.
> 
> Add a UI popup once if there is any data missing, and then warn in the
> bottom left for each individual DSO that's missing.

Looks ok to me (the last 3 patches in this series, the rest I applied
already), can I get some Acked-by/Reviewed-by from the CoreSight people?

Thanks,

- Arnaldo
 
> Signed-off-by: James Clark <james.clark@arm.com>
> ---
>  tools/perf/util/cs-etm.c | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/perf/util/cs-etm.c b/tools/perf/util/cs-etm.c
> index 32ad92d3e454..e6851260d059 100644
> --- a/tools/perf/util/cs-etm.c
> +++ b/tools/perf/util/cs-etm.c
> @@ -746,8 +746,16 @@ static u32 cs_etm__mem_access(struct cs_etm_queue *etmq, u8 trace_chan_id,
>  
>  	len = dso__data_read_offset(al.map->dso, machine, offset, buffer, size);
>  
> -	if (len <= 0)
> +	if (len <= 0) {
> +		ui__warning_once("CS ETM Trace: Missing DSO. Use 'perf archive' to export data from the traced system.\n");
> +		if (!al.map->dso->auxtrace_warned) {
> +			pr_err("CS ETM Trace: Debug data not found for address %#"PRIx64" in %s\n",
> +				    address,
> +				    al.map->dso->long_name ? al.map->dso->long_name : "Unknown");
> +			al.map->dso->auxtrace_warned = true;
> +		}
>  		return 0;
> +	}
>  
>  	return len;
>  }
> -- 
> 2.28.0
> 

-- 

- Arnaldo

Re: [RFC PATCH 5/6] perf cs-etm: Improve Coresight zero timestamp warning

[Leo Yan]

On Thu, Jul 29, 2021 at 04:58:04PM +0100, James Clark wrote:
> Only show the warning if the user hasn't already set timeless mode and
> improve the text because there was ambiguity around the meaning of '...'
> 
> Change the warning to a UI warning instead of printing straight to
> stderr because this corrupts the UI when perf report TUI is used. The UI
> warning function also handles printing to stderr when in perf script
> mode.
> 
> Suggested-by: Leo Yan <leo.yan@linaro.org>
> Signed-off-by: James Clark <james.clark@arm.com>

Reviewed-by: Leo Yan <leo.yan@linaro.org>

> ---
>  tools/perf/util/cs-etm-decoder/cs-etm-decoder.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/perf/util/cs-etm-decoder/cs-etm-decoder.c b/tools/perf/util/cs-etm-decoder/cs-etm-decoder.c
> index 3e1a05bc82cc..5084bd2ca6eb 100644
> --- a/tools/perf/util/cs-etm-decoder/cs-etm-decoder.c
> +++ b/tools/perf/util/cs-etm-decoder/cs-etm-decoder.c
> @@ -324,8 +324,11 @@ cs_etm_decoder__do_hard_timestamp(struct cs_etm_queue *etmq,
>  		 * underflow.
>  		 */
>  		packet_queue->cs_timestamp = 0;
> -		WARN_ONCE(true, "Zero Coresight timestamp found at Idx:%" OCSD_TRC_IDX_STR
> -				". Decoding may be improved with --itrace=Z...\n", indx);
> +		if (!cs_etm__etmq_is_timeless(etmq))
> +			pr_warning_once("Zero Coresight timestamp found at Idx:%" OCSD_TRC_IDX_STR
> +					". Decoding may be improved by prepending 'Z' to your current --itrace arguments.\n",
> +					indx);
> +
>  	} else if (packet_queue->instr_count > elem->timestamp) {
>  		/*
>  		 * Sanity check that the elem->timestamp - packet_queue->instr_count would not
> -- 
> 2.28.0
> 

Re: [RFC PATCH 6/6] perf cs-etm: Add warnings for missing DSOs

[Leo Yan]

On Thu, Jul 29, 2021 at 04:58:05PM +0100, James Clark wrote:
> Currently decode will silently fail if no binary data is available for
> the decode. This is made worse if only partial data is available because
> the decode will appear to work, but any trace from that missing DSO will
> silently not be generated.
> 
> Add a UI popup once if there is any data missing, and then warn in the
> bottom left for each individual DSO that's missing.
> 
> Signed-off-by: James Clark <james.clark@arm.com>
> ---
>  tools/perf/util/cs-etm.c | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/perf/util/cs-etm.c b/tools/perf/util/cs-etm.c
> index 32ad92d3e454..e6851260d059 100644
> --- a/tools/perf/util/cs-etm.c
> +++ b/tools/perf/util/cs-etm.c
> @@ -746,8 +746,16 @@ static u32 cs_etm__mem_access(struct cs_etm_queue *etmq, u8 trace_chan_id,
>  
>  	len = dso__data_read_offset(al.map->dso, machine, offset, buffer, size);
>  
> -	if (len <= 0)
> +	if (len <= 0) {
> +		ui__warning_once("CS ETM Trace: Missing DSO. Use 'perf archive' to export data from the traced system.\n");
> +		if (!al.map->dso->auxtrace_warned) {
> +			pr_err("CS ETM Trace: Debug data not found for address %#"PRIx64" in %s\n",
> +				    address,
> +				    al.map->dso->long_name ? al.map->dso->long_name : "Unknown");
> +			al.map->dso->auxtrace_warned = true;
> +		}

This is very useful.

Just one comment: in particularly if the perf fails to find the kernel
symbols, the user needs to enable config "CONFIG_PROC_KCORE=y" or
specify option "-k /path/to/vmlinux".  In this case, using 'perf
archive' is not helpful.  So I think the UI warning can be imporved
like:

        ui__warning_once("CS ETM Trace: Missing DSO. Use 'perf archive' to export data from the traced system.\n"
                         " Enable CONFIG_PROC_KCORE or use option '-k /path/to/vmlinux' for kernel symbols\n");

With this improvement, the patch looks good to me:

Reviewed-by: Leo Yan <leo.yan@linaro.org>

>  		return 0;
> +	}
>  
>  	return len;
>  }
> -- 
> 2.28.0
> 

Re: [RFC PATCH 6/6] perf cs-etm: Add warnings for missing DSOs

[Mathieu Poirier]

On Mon, Aug 02, 2021 at 11:51:58AM -0300, Arnaldo Carvalho de Melo wrote:
> Em Thu, Jul 29, 2021 at 04:58:05PM +0100, James Clark escreveu:
> > Currently decode will silently fail if no binary data is available for
> > the decode. This is made worse if only partial data is available because
> > the decode will appear to work, but any trace from that missing DSO will
> > silently not be generated.
> > 
> > Add a UI popup once if there is any data missing, and then warn in the
> > bottom left for each individual DSO that's missing.
> 
> Looks ok to me (the last 3 patches in this series, the rest I applied
> already), can I get some Acked-by/Reviewed-by from the CoreSight people?
>

I have a substantial backlog of patches to go through and I will be away for the
next two weeks.  As such it will be some time before I get to review this set.

Regards,
Mathieu

> Thanks,
> 
> - Arnaldo
>  
> > Signed-off-by: James Clark <james.clark@arm.com>
> > ---
> >  tools/perf/util/cs-etm.c | 10 +++++++++-
> >  1 file changed, 9 insertions(+), 1 deletion(-)
> > 
> > diff --git a/tools/perf/util/cs-etm.c b/tools/perf/util/cs-etm.c
> > index 32ad92d3e454..e6851260d059 100644
> > --- a/tools/perf/util/cs-etm.c
> > +++ b/tools/perf/util/cs-etm.c
> > @@ -746,8 +746,16 @@ static u32 cs_etm__mem_access(struct cs_etm_queue *etmq, u8 trace_chan_id,
> >  
> >  	len = dso__data_read_offset(al.map->dso, machine, offset, buffer, size);
> >  
> > -	if (len <= 0)
> > +	if (len <= 0) {
> > +		ui__warning_once("CS ETM Trace: Missing DSO. Use 'perf archive' to export data from the traced system.\n");
> > +		if (!al.map->dso->auxtrace_warned) {
> > +			pr_err("CS ETM Trace: Debug data not found for address %#"PRIx64" in %s\n",
> > +				    address,
> > +				    al.map->dso->long_name ? al.map->dso->long_name : "Unknown");
> > +			al.map->dso->auxtrace_warned = true;
> > +		}
> >  		return 0;
> > +	}
> >  
> >  	return len;
> >  }
> > -- 
> > 2.28.0
> > 
> 
> -- 
> 
> - Arnaldo

Re: [RFC PATCH 6/6] perf cs-etm: Add warnings for missing DSOs

[Mike Leach]

HI James,

On Mon, 2 Aug 2021 at 18:03, Mathieu Poirier <mathieu.poirier@linaro.org> wrote:
>
> On Mon, Aug 02, 2021 at 11:51:58AM -0300, Arnaldo Carvalho de Melo wrote:
> > Em Thu, Jul 29, 2021 at 04:58:05PM +0100, James Clark escreveu:
> > > Currently decode will silently fail if no binary data is available for
> > > the decode. This is made worse if only partial data is available because
> > > the decode will appear to work, but any trace from that missing DSO will
> > > silently not be generated.
> > >

The decoder actually outputs a OCSD_GEN_TRC_ELEM_ADDR_NACC packet if
binary memory data is missing.
These packets are currently ignored by perf / cs-etm-decoder.c.

I think this per DSO warning is fine, but perhaps at some point add in
handling for OCSD_GEN_TRC_ELEM_ADDR_NACC - which perhaps is only
active when dumping trace packets.

Regards

Mike


> > > Add a UI popup once if there is any data missing, and then warn in the
> > > bottom left for each individual DSO that's missing.
> >
> > Looks ok to me (the last 3 patches in this series, the rest I applied
> > already), can I get some Acked-by/Reviewed-by from the CoreSight people?
> >
>
> I have a substantial backlog of patches to go through and I will be away for the
> next two weeks.  As such it will be some time before I get to review this set.
>
> Regards,
> Mathieu
>
> > Thanks,
> >
> > - Arnaldo
> >
> > > Signed-off-by: James Clark <james.clark@arm.com>
> > > ---
> > >  tools/perf/util/cs-etm.c | 10 +++++++++-
> > >  1 file changed, 9 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/tools/perf/util/cs-etm.c b/tools/perf/util/cs-etm.c
> > > index 32ad92d3e454..e6851260d059 100644
> > > --- a/tools/perf/util/cs-etm.c
> > > +++ b/tools/perf/util/cs-etm.c
> > > @@ -746,8 +746,16 @@ static u32 cs_etm__mem_access(struct cs_etm_queue *etmq, u8 trace_chan_id,
> > >
> > >     len = dso__data_read_offset(al.map->dso, machine, offset, buffer, size);
> > >
> > > -   if (len <= 0)
> > > +   if (len <= 0) {
> > > +           ui__warning_once("CS ETM Trace: Missing DSO. Use 'perf archive' to export data from the traced system.\n");
> > > +           if (!al.map->dso->auxtrace_warned) {
> > > +                   pr_err("CS ETM Trace: Debug data not found for address %#"PRIx64" in %s\n",
> > > +                               address,
> > > +                               al.map->dso->long_name ? al.map->dso->long_name : "Unknown");
> > > +                   al.map->dso->auxtrace_warned = true;
> > > +           }
> > >             return 0;
> > > +   }
> > >
> > >     return len;
> > >  }
> > > --
> > > 2.28.0
> > >
> >
> > --
> >
> > - Arnaldo



-- 
Mike Leach
Principal Engineer, ARM Ltd.
Manchester Design Centre. UK

Re: [RFC PATCH 6/6] perf cs-etm: Add warnings for missing DSOs

[Arnaldo Carvalho de Melo]

Em Mon, Aug 02, 2021 at 11:41:45PM +0800, Leo Yan escreveu:
> On Thu, Jul 29, 2021 at 04:58:05PM +0100, James Clark wrote:
> > Currently decode will silently fail if no binary data is available for
> > the decode. This is made worse if only partial data is available because
> > the decode will appear to work, but any trace from that missing DSO will
> > silently not be generated.
> > 
> > Add a UI popup once if there is any data missing, and then warn in the
> > bottom left for each individual DSO that's missing.
> > 
> > Signed-off-by: James Clark <james.clark@arm.com>
> > ---
> >  tools/perf/util/cs-etm.c | 10 +++++++++-
> >  1 file changed, 9 insertions(+), 1 deletion(-)
> > 
> > diff --git a/tools/perf/util/cs-etm.c b/tools/perf/util/cs-etm.c
> > index 32ad92d3e454..e6851260d059 100644
> > --- a/tools/perf/util/cs-etm.c
> > +++ b/tools/perf/util/cs-etm.c
> > @@ -746,8 +746,16 @@ static u32 cs_etm__mem_access(struct cs_etm_queue *etmq, u8 trace_chan_id,
> >  
> >  	len = dso__data_read_offset(al.map->dso, machine, offset, buffer, size);
> >  
> > -	if (len <= 0)
> > +	if (len <= 0) {
> > +		ui__warning_once("CS ETM Trace: Missing DSO. Use 'perf archive' to export data from the traced system.\n");
> > +		if (!al.map->dso->auxtrace_warned) {
> > +			pr_err("CS ETM Trace: Debug data not found for address %#"PRIx64" in %s\n",
> > +				    address,
> > +				    al.map->dso->long_name ? al.map->dso->long_name : "Unknown");
> > +			al.map->dso->auxtrace_warned = true;
> > +		}
> 
> This is very useful.
> 
> Just one comment: in particularly if the perf fails to find the kernel
> symbols, the user needs to enable config "CONFIG_PROC_KCORE=y" or
> specify option "-k /path/to/vmlinux".  In this case, using 'perf
> archive' is not helpful.  So I think the UI warning can be imporved
> like:
> 
>         ui__warning_once("CS ETM Trace: Missing DSO. Use 'perf archive' to export data from the traced system.\n"
>                          " Enable CONFIG_PROC_KCORE or use option '-k /path/to/vmlinux' for kernel symbols\n");

one can also use debuginfod-client, which, as time passes, probably will
be the main way of finding DSOs now that we have build-ids in
PERF_RECORD_MMAP2 and debuginfod servers such as:

export DEBUGINFOD_URLS=https://debuginfod.fedoraproject.org/

https://fedoraproject.org/wiki/Debuginfod
 
> With this improvement, the patch looks good to me:
> 
> Reviewed-by: Leo Yan <leo.yan@linaro.org>

Does this apply to the other 5 patches in this series?

- Arnaldo
 
> >  		return 0;
> > +	}
> >  
> >  	return len;
> >  }
> > -- 
> > 2.28.0
> > 

-- 

- Arnaldo

Re: [RFC PATCH 5/6] perf cs-etm: Improve Coresight zero timestamp warning

[Arnaldo Carvalho de Melo]

Em Mon, Aug 02, 2021 at 11:17:10PM +0800, Leo Yan escreveu:
> On Thu, Jul 29, 2021 at 04:58:04PM +0100, James Clark wrote:
> > Only show the warning if the user hasn't already set timeless mode and
> > improve the text because there was ambiguity around the meaning of '...'
> > 
> > Change the warning to a UI warning instead of printing straight to
> > stderr because this corrupts the UI when perf report TUI is used. The UI
> > warning function also handles printing to stderr when in perf script
> > mode.
> > 
> > Suggested-by: Leo Yan <leo.yan@linaro.org>
> > Signed-off-by: James Clark <james.clark@arm.com>
> 
> Reviewed-by: Leo Yan <leo.yan@linaro.org>

Thanks, applied.

- Arnaldo

 
> > ---
> >  tools/perf/util/cs-etm-decoder/cs-etm-decoder.c | 7 +++++--
> >  1 file changed, 5 insertions(+), 2 deletions(-)
> > 
> > diff --git a/tools/perf/util/cs-etm-decoder/cs-etm-decoder.c b/tools/perf/util/cs-etm-decoder/cs-etm-decoder.c
> > index 3e1a05bc82cc..5084bd2ca6eb 100644
> > --- a/tools/perf/util/cs-etm-decoder/cs-etm-decoder.c
> > +++ b/tools/perf/util/cs-etm-decoder/cs-etm-decoder.c
> > @@ -324,8 +324,11 @@ cs_etm_decoder__do_hard_timestamp(struct cs_etm_queue *etmq,
> >  		 * underflow.
> >  		 */
> >  		packet_queue->cs_timestamp = 0;
> > -		WARN_ONCE(true, "Zero Coresight timestamp found at Idx:%" OCSD_TRC_IDX_STR
> > -				". Decoding may be improved with --itrace=Z...\n", indx);
> > +		if (!cs_etm__etmq_is_timeless(etmq))
> > +			pr_warning_once("Zero Coresight timestamp found at Idx:%" OCSD_TRC_IDX_STR
> > +					". Decoding may be improved by prepending 'Z' to your current --itrace arguments.\n",
> > +					indx);
> > +
> >  	} else if (packet_queue->instr_count > elem->timestamp) {
> >  		/*
> >  		 * Sanity check that the elem->timestamp - packet_queue->instr_count would not
> > -- 
> > 2.28.0
> > 

-- 

- Arnaldo

Re: [RFC PATCH 6/6] perf cs-etm: Add warnings for missing DSOs

[Leo Yan]

On Tue, Aug 03, 2021 at 10:24:09AM -0300, Arnaldo Carvalho de Melo wrote:
> Em Mon, Aug 02, 2021 at 11:41:45PM +0800, Leo Yan escreveu:
> > On Thu, Jul 29, 2021 at 04:58:05PM +0100, James Clark wrote:
> > > Currently decode will silently fail if no binary data is available for
> > > the decode. This is made worse if only partial data is available because
> > > the decode will appear to work, but any trace from that missing DSO will
> > > silently not be generated.
> > > 
> > > Add a UI popup once if there is any data missing, and then warn in the
> > > bottom left for each individual DSO that's missing.
> > > 
> > > Signed-off-by: James Clark <james.clark@arm.com>
> > > ---
> > >  tools/perf/util/cs-etm.c | 10 +++++++++-
> > >  1 file changed, 9 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/tools/perf/util/cs-etm.c b/tools/perf/util/cs-etm.c
> > > index 32ad92d3e454..e6851260d059 100644
> > > --- a/tools/perf/util/cs-etm.c
> > > +++ b/tools/perf/util/cs-etm.c
> > > @@ -746,8 +746,16 @@ static u32 cs_etm__mem_access(struct cs_etm_queue *etmq, u8 trace_chan_id,
> > >  
> > >  	len = dso__data_read_offset(al.map->dso, machine, offset, buffer, size);
> > >  
> > > -	if (len <= 0)
> > > +	if (len <= 0) {
> > > +		ui__warning_once("CS ETM Trace: Missing DSO. Use 'perf archive' to export data from the traced system.\n");
> > > +		if (!al.map->dso->auxtrace_warned) {
> > > +			pr_err("CS ETM Trace: Debug data not found for address %#"PRIx64" in %s\n",
> > > +				    address,
> > > +				    al.map->dso->long_name ? al.map->dso->long_name : "Unknown");
> > > +			al.map->dso->auxtrace_warned = true;
> > > +		}
> > 
> > This is very useful.
> > 
> > Just one comment: in particularly if the perf fails to find the kernel
> > symbols, the user needs to enable config "CONFIG_PROC_KCORE=y" or
> > specify option "-k /path/to/vmlinux".  In this case, using 'perf
> > archive' is not helpful.  So I think the UI warning can be imporved
> > like:
> > 
> >         ui__warning_once("CS ETM Trace: Missing DSO. Use 'perf archive' to export data from the traced system.\n"
> >                          " Enable CONFIG_PROC_KCORE or use option '-k /path/to/vmlinux' for kernel symbols\n");
> 
> one can also use debuginfod-client, which, as time passes, probably will
> be the main way of finding DSOs now that we have build-ids in
> PERF_RECORD_MMAP2 and debuginfod servers such as:
> 
> export DEBUGINFOD_URLS=https://debuginfod.fedoraproject.org/

If so, maybe should use more general description for missing DSO.

> https://fedoraproject.org/wiki/Debuginfod
>  
> > With this improvement, the patch looks good to me:
> > 
> > Reviewed-by: Leo Yan <leo.yan@linaro.org>
> 
> Does this apply to the other 5 patches in this series?

I finished to reivew patches 01, 04, 05, 06/06, so my review tag can
apply on these patches.  Current patch 06/06 needs James to improve for
the comments.

Thanks,
Leo

Re: [RFC PATCH 6/6] perf cs-etm: Add warnings for missing DSOs

[James Clark]

On 03/08/2021 15:01, Leo Yan wrote:
>> one can also use debuginfod-client, which, as time passes, probably will
>> be the main way of finding DSOs now that we have build-ids in
>> PERF_RECORD_MMAP2 and debuginfod servers such as:
>>
>> export DEBUGINFOD_URLS=https://debuginfod.fedoraproject.org/
> If so, maybe should use more general description for missing DSO.
> 
>> https://fedoraproject.org/wiki/Debuginfod
>>  
>>> With this improvement, the patch looks good to me:
>>>
>>> Reviewed-by: Leo Yan <leo.yan@linaro.org>
>> Does this apply to the other 5 patches in this series?
> I finished to reivew patches 01, 04, 05, 06/06, so my review tag can
> apply on these patches.  Current patch 06/06 needs James to improve for
> the comments.

Thanks for the reviews, I've submitted v2.

I also had a play around with debuginfod, it looks promising.
Especially if it's integrated with perf report, which I assume is the plan?

Thanks
James

Re: [RFC PATCH 6/6] perf cs-etm: Add warnings for missing DSOs

[Arnaldo Carvalho de Melo]

Em Thu, Aug 05, 2021 at 01:59:49PM +0100, James Clark escreveu:
> 
> 
> On 03/08/2021 15:01, Leo Yan wrote:
> >> one can also use debuginfod-client, which, as time passes, probably will
> >> be the main way of finding DSOs now that we have build-ids in
> >> PERF_RECORD_MMAP2 and debuginfod servers such as:
> >>
> >> export DEBUGINFOD_URLS=https://debuginfod.fedoraproject.org/
> > If so, maybe should use more general description for missing DSO.
> > 
> >> https://fedoraproject.org/wiki/Debuginfod
> >>  
> >>> With this improvement, the patch looks good to me:
> >>>
> >>> Reviewed-by: Leo Yan <leo.yan@linaro.org>
> >> Does this apply to the other 5 patches in this series?
> > I finished to reivew patches 01, 04, 05, 06/06, so my review tag can
> > apply on these patches.  Current patch 06/06 needs James to improve for
> > the comments.
> 
> Thanks for the reviews, I've submitted v2.
> 
> I also had a play around with debuginfod, it looks promising.
> Especially if it's integrated with perf report, which I assume is the plan?

Yeah, code is needed for that in the underlying symbols library so that
we can tune its usage, i.e. the threshold of samples to trigger asking
for the auto-download of debuginfo files.

- Arnaldo