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(-)
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, ...);
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
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;
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);
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;
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
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
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
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; }
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 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 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 len; } -- 2.28.0
--
- Arnaldo
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
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
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
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
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
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
-
Arnaldo Carvalho de Melo -
James Clark -
Leo Yan -
Mathieu Poirier -
Mike Leach