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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
Currently WARN_ONCE prints to stderr and corrupts the TUI. Add
equivalent methods for UI warnings.
Signed-off-by: James Clark <[email protected]>
---
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
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 <[email protected]>
Signed-off-by: James Clark <[email protected]>
---
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
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 <[email protected]>
> ---
> 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
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 <[email protected]>
> ---
> 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
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 <[email protected]>
> ---
> 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 <[email protected]>
> return 0;
> + }
>
> return len;
> }
> --
> 2.28.0
>
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 <[email protected]>
> > ---
> > 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
HI James,
On Mon, 2 Aug 2021 at 18:03, Mathieu Poirier <[email protected]> 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 <[email protected]>
> > > ---
> > > 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
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 <[email protected]>
> > ---
> > 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 <[email protected]>
Does this apply to the other 5 patches in this series?
- Arnaldo
> > return 0;
> > + }
> >
> > return len;
> > }
> > --
> > 2.28.0
> >
--
- Arnaldo
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 <[email protected]>
> > Signed-off-by: James Clark <[email protected]>
>
> Reviewed-by: Leo Yan <[email protected]>
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
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 <[email protected]>
> > > ---
> > > 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 <[email protected]>
>
> 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
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 <[email protected]>
>> 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
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 <[email protected]>
> >> 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