[PATCH v2 1/3] perf tsc: Add missing newlines to debug statements

[PATCH v2 1/3] perf tsc: Add missing newlines to debug statements

[Ian Rogers]

It is assumed that debug statements always print a newline, fix two
missing ones.

Signed-off-by: Ian Rogers <irogers@google.com>
---
This patch was inspired by bad debug output in:
https://lore.kernel.org/linux-perf-users/CAGjhMsg_bVKJ_zfsLUR32+oZwGDr3OiBHV_BJ3QtFjyKAs7Sgg@mail.gmail.com/
---
 tools/perf/arch/x86/util/tsc.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/perf/arch/x86/util/tsc.c b/tools/perf/arch/x86/util/tsc.c
index 9b99f48b923c..e2d6cfe21057 100644
--- a/tools/perf/arch/x86/util/tsc.c
+++ b/tools/perf/arch/x86/util/tsc.c
@@ -33,7 +33,7 @@ static double cpuinfo_tsc_freq(void)
 
 	cpuinfo = fopen("/proc/cpuinfo", "r");
 	if (!cpuinfo) {
-		pr_err("Failed to read /proc/cpuinfo for TSC frequency");
+		pr_err("Failed to read /proc/cpuinfo for TSC frequency\n");
 		return NAN;
 	}
 	while (getline(&line, &len, cpuinfo) > 0) {
@@ -48,7 +48,7 @@ static double cpuinfo_tsc_freq(void)
 	}
 out:
 	if (fpclassify(result) == FP_ZERO)
-		pr_err("Failed to find TSC frequency in /proc/cpuinfo");
+		pr_err("Failed to find TSC frequency in /proc/cpuinfo\n");
 
 	free(line);
 	fclose(cpuinfo);
-- 
2.43.0.429.g432eaa2c6b-goog

[PATCH v2 2/3] perf parse-events: Improve error location of terms cloned from an event

[Ian Rogers]

A PMU event/alias will have a set of format terms that replace it when
an event is parsed. The location of the terms is their position when
parsed for the event/alias either from sysfs or json. This location is
of little use when an event fails to parse as the error will be given
in terms of the location in the string of events parsed not the json
or sysfs string. Fix this by making the cloned terms location that of
the event/alias.

If a cloned term from an event/alias is invalid the bad format is hard
to determine from the error string. Add the name of the bad format
into the error string.

Signed-off-by: Ian Rogers <irogers@google.com>
---
These fixes were inspired by the poor error output in:
https://lore.kernel.org/linux-perf-users/alpine.LRH.2.20.2401300733310.11354@Diego/
---
 tools/perf/util/pmu.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c
index 355f813f960d..437386dedd5c 100644
--- a/tools/perf/util/pmu.c
+++ b/tools/perf/util/pmu.c
@@ -657,7 +657,7 @@ static int pmu_aliases_parse(struct perf_pmu *pmu)
 	return 0;
 }
 
-static int pmu_alias_terms(struct perf_pmu_alias *alias, struct list_head *terms)
+static int pmu_alias_terms(struct perf_pmu_alias *alias, int err_loc, struct list_head *terms)
 {
 	struct parse_events_term *term, *cloned;
 	struct parse_events_terms clone_terms;
@@ -675,6 +675,7 @@ static int pmu_alias_terms(struct perf_pmu_alias *alias, struct list_head *terms
 		 * which we don't want for implicit terms in aliases.
 		 */
 		cloned->weak = true;
+		cloned->err_term = cloned->err_val = err_loc;
 		list_add_tail(&cloned->list, &clone_terms.terms);
 	}
 	list_splice_init(&clone_terms.terms, terms);
@@ -1363,8 +1364,8 @@ static int pmu_config_term(const struct perf_pmu *pmu,
 
 			parse_events_error__handle(err, term->err_val,
 				asprintf(&err_str,
-				    "value too big for format, maximum is %llu",
-				    (unsigned long long)max_val) < 0
+				    "value too big for format (%s), maximum is %llu",
+				    format->name, (unsigned long long)max_val) < 0
 				    ? strdup("value too big for format")
 				    : err_str,
 				    NULL);
@@ -1518,7 +1519,7 @@ int perf_pmu__check_alias(struct perf_pmu *pmu, struct parse_events_terms *head_
 		alias = pmu_find_alias(pmu, term);
 		if (!alias)
 			continue;
-		ret = pmu_alias_terms(alias, &term->list);
+		ret = pmu_alias_terms(alias, term->err_term, &term->list);
 		if (ret) {
 			parse_events_error__handle(err, term->err_term,
 						strdup("Failure to duplicate terms"),
-- 
2.43.0.429.g432eaa2c6b-goog

Re: [PATCH v2 2/3] perf parse-events: Improve error location of terms cloned from an event

[James Clark]

On 31/01/2024 13:49, Ian Rogers wrote:
> A PMU event/alias will have a set of format terms that replace it when
> an event is parsed. The location of the terms is their position when
> parsed for the event/alias either from sysfs or json. This location is
> of little use when an event fails to parse as the error will be given
> in terms of the location in the string of events parsed not the json
> or sysfs string. Fix this by making the cloned terms location that of
> the event/alias.
> 
> If a cloned term from an event/alias is invalid the bad format is hard
> to determine from the error string. Add the name of the bad format
> into the error string.
> 
> Signed-off-by: Ian Rogers <irogers@google.com>

Reviewed-by: James Clark <james.clark@arm.com>

> ---
> These fixes were inspired by the poor error output in:
> https://lore.kernel.org/linux-perf-users/alpine.LRH.2.20.2401300733310.11354@Diego/
> ---
>  tools/perf/util/pmu.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c
> index 355f813f960d..437386dedd5c 100644
> --- a/tools/perf/util/pmu.c
> +++ b/tools/perf/util/pmu.c
> @@ -657,7 +657,7 @@ static int pmu_aliases_parse(struct perf_pmu *pmu)
>  	return 0;
>  }
>  
> -static int pmu_alias_terms(struct perf_pmu_alias *alias, struct list_head *terms)
> +static int pmu_alias_terms(struct perf_pmu_alias *alias, int err_loc, struct list_head *terms)
>  {
>  	struct parse_events_term *term, *cloned;
>  	struct parse_events_terms clone_terms;
> @@ -675,6 +675,7 @@ static int pmu_alias_terms(struct perf_pmu_alias *alias, struct list_head *terms
>  		 * which we don't want for implicit terms in aliases.
>  		 */
>  		cloned->weak = true;
> +		cloned->err_term = cloned->err_val = err_loc;
>  		list_add_tail(&cloned->list, &clone_terms.terms);
>  	}
>  	list_splice_init(&clone_terms.terms, terms);
> @@ -1363,8 +1364,8 @@ static int pmu_config_term(const struct perf_pmu *pmu,
>  
>  			parse_events_error__handle(err, term->err_val,
>  				asprintf(&err_str,
> -				    "value too big for format, maximum is %llu",
> -				    (unsigned long long)max_val) < 0
> +				    "value too big for format (%s), maximum is %llu",
> +				    format->name, (unsigned long long)max_val) < 0
>  				    ? strdup("value too big for format")
>  				    : err_str,
>  				    NULL);
> @@ -1518,7 +1519,7 @@ int perf_pmu__check_alias(struct perf_pmu *pmu, struct parse_events_terms *head_
>  		alias = pmu_find_alias(pmu, term);
>  		if (!alias)
>  			continue;
> -		ret = pmu_alias_terms(alias, &term->list);
> +		ret = pmu_alias_terms(alias, term->err_term, &term->list);
>  		if (ret) {
>  			parse_events_error__handle(err, term->err_term,
>  						strdup("Failure to duplicate terms"),

[PATCH v2 3/3] perf parse-events: Print all errors

[Ian Rogers]

Prior to this patch the first and the last error encountered during
parsing are printed. To see other errors verbose needs
enabling. Unfortunately this can drop useful errors, in particular on
terms. This patch changes the errors so that instead of the first and
last all errors are recorded and printed, the underlying data
structure is changed to a list.

Before:
```
$ perf stat -e 'slots/edge=2/' true
event syntax error: 'slots/edge=2/'
                                \___ Bad event or PMU

Unable to find PMU or event on a PMU of 'slots'

Initial error:
event syntax error: 'slots/edge=2/'
                     \___ Cannot find PMU `slots'. Missing kernel support?
Run 'perf list' for a list of valid events

 Usage: perf stat [<options>] [<command>]

    -e, --event <event>   event selector. use 'perf list' to list available events
```

After:
```
$ perf stat -e 'slots/edge=2/' true
event syntax error: 'slots/edge=2/'
                     \___ Bad event or PMU

Unable to find PMU or event on a PMU of 'slots'

event syntax error: 'slots/edge=2/'
                                \___ value too big for format (edge), maximum is 1

event syntax error: 'slots/edge=2/'
                     \___ Cannot find PMU `slots'. Missing kernel support?
Run 'perf list' for a list of valid events

 Usage: perf stat [<options>] [<command>]

    -e, --event <event>   event selector. use 'perf list' to list available events
```

Signed-off-by: Ian Rogers <irogers@google.com>
---
Prompted by discussion:
https://lore.kernel.org/linux-perf-users/9dd303cb-0455-d8ac-ce0c-f4a8320b787b@arm.com/
---
 tools/perf/arch/x86/tests/hybrid.c |  5 +-
 tools/perf/tests/expand-cgroup.c   |  3 +-
 tools/perf/tests/parse-events.c    |  9 ++-
 tools/perf/util/parse-events.c     | 92 ++++++++++++++++++------------
 tools/perf/util/parse-events.h     | 14 ++---
 tools/perf/util/parse-events.y     |  2 -
 6 files changed, 67 insertions(+), 58 deletions(-)

diff --git a/tools/perf/arch/x86/tests/hybrid.c b/tools/perf/arch/x86/tests/hybrid.c
index 40f5d17fedab..e221ea104174 100644
--- a/tools/perf/arch/x86/tests/hybrid.c
+++ b/tools/perf/arch/x86/tests/hybrid.c
@@ -259,11 +259,10 @@ static int test_event(const struct evlist_test *e)
 	parse_events_error__init(&err);
 	ret = parse_events(evlist, e->name, &err);
 	if (ret) {
-		pr_debug("failed to parse event '%s', err %d, str '%s'\n",
-			 e->name, ret, err.str);
+		pr_debug("failed to parse event '%s', err %d\n", e->name, ret);
 		parse_events_error__print(&err, e->name);
 		ret = TEST_FAIL;
-		if (strstr(err.str, "can't access trace events"))
+		if (parse_events_error__contains(&err, "can't access trace events"))
 			ret = TEST_SKIP;
 	} else {
 		ret = e->check(evlist);
diff --git a/tools/perf/tests/expand-cgroup.c b/tools/perf/tests/expand-cgroup.c
index 9c1a1f18db75..31966ff856f8 100644
--- a/tools/perf/tests/expand-cgroup.c
+++ b/tools/perf/tests/expand-cgroup.c
@@ -127,8 +127,7 @@ static int expand_group_events(void)
 	parse_events_error__init(&err);
 	ret = parse_events(evlist, event_str, &err);
 	if (ret < 0) {
-		pr_debug("failed to parse event '%s', err %d, str '%s'\n",
-			 event_str, ret, err.str);
+		pr_debug("failed to parse event '%s', err %d\n", event_str, ret);
 		parse_events_error__print(&err, event_str);
 		goto out;
 	}
diff --git a/tools/perf/tests/parse-events.c b/tools/perf/tests/parse-events.c
index fbdf710d5eea..feb5727584d1 100644
--- a/tools/perf/tests/parse-events.c
+++ b/tools/perf/tests/parse-events.c
@@ -2506,11 +2506,10 @@ static int test_event(const struct evlist_test *e)
 	parse_events_error__init(&err);
 	ret = parse_events(evlist, e->name, &err);
 	if (ret) {
-		pr_debug("failed to parse event '%s', err %d, str '%s'\n",
-			 e->name, ret, err.str);
+		pr_debug("failed to parse event '%s', err %d\n", e->name, ret);
 		parse_events_error__print(&err, e->name);
 		ret = TEST_FAIL;
-		if (err.str && strstr(err.str, "can't access trace events"))
+		if (parse_events_error__contains(&err, "can't access trace events"))
 			ret = TEST_SKIP;
 	} else {
 		ret = e->check(evlist);
@@ -2535,8 +2534,8 @@ static int test_event_fake_pmu(const char *str)
 	ret = __parse_events(evlist, str, /*pmu_filter=*/NULL, &err,
 			     &perf_pmu__fake, /*warn_if_reordered=*/true);
 	if (ret) {
-		pr_debug("failed to parse event '%s', err %d, str '%s'\n",
-			 str, ret, err.str);
+		pr_debug("failed to parse event '%s', err %d\n",
+			 str, ret);
 		parse_events_error__print(&err, str);
 	}
 
diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
index 66eabcea4242..6f8b0fa17689 100644
--- a/tools/perf/util/parse-events.c
+++ b/tools/perf/util/parse-events.c
@@ -2181,50 +2181,53 @@ int parse_event(struct evlist *evlist, const char *str)
 	return ret;
 }
 
+struct parse_events_error_entry {
+	/** @list: The list the error is part of. */
+	struct list_head list;
+	/** @idx: index in the parsed string */
+	int   idx;
+	/** @str: string to display at the index */
+	char *str;
+	/** @help: optional help string */
+	char *help;
+};
+
 void parse_events_error__init(struct parse_events_error *err)
 {
-	bzero(err, sizeof(*err));
+	INIT_LIST_HEAD(&err->list);
 }
 
 void parse_events_error__exit(struct parse_events_error *err)
 {
-	zfree(&err->str);
-	zfree(&err->help);
-	zfree(&err->first_str);
-	zfree(&err->first_help);
+	struct parse_events_error_entry *pos, *tmp;
+
+	list_for_each_entry_safe(pos, tmp, &err->list, list) {
+		zfree(&pos->str);
+		zfree(&pos->help);
+		list_del_init(&pos->list);
+		free(pos);
+	}
 }
 
 void parse_events_error__handle(struct parse_events_error *err, int idx,
 				char *str, char *help)
 {
+	struct parse_events_error_entry *entry;
+
 	if (WARN(!str || !err, "WARNING: failed to provide error string or struct\n"))
 		goto out_free;
-	switch (err->num_errors) {
-	case 0:
-		err->idx = idx;
-		err->str = str;
-		err->help = help;
-		break;
-	case 1:
-		err->first_idx = err->idx;
-		err->idx = idx;
-		err->first_str = err->str;
-		err->str = str;
-		err->first_help = err->help;
-		err->help = help;
-		break;
-	default:
-		pr_debug("Multiple errors dropping message: %s (%s)\n",
-			err->str, err->help ?: "<no help>");
-		free(err->str);
-		err->str = str;
-		free(err->help);
-		err->help = help;
-		break;
+
+	entry = zalloc(sizeof(*entry));
+	if (!entry) {
+		pr_err("Failed to allocate memory for event parsing error: %s (%s)\n",
+			str, help ?: "<no help>");
+		goto out_free;
 	}
-	err->num_errors++;
+	entry->idx = idx;
+	entry->str = str;
+	entry->help = help;
+	list_add(&entry->list, &err->list);
 	return;
-
 out_free:
 	free(str);
 	free(help);
@@ -2294,19 +2297,34 @@ static void __parse_events_error__print(int err_idx, const char *err_str,
 	}
 }
 
-void parse_events_error__print(struct parse_events_error *err,
+void parse_events_error__print(const struct parse_events_error *err,
 			       const char *event)
 {
-	if (!err->num_errors)
-		return;
+	struct parse_events_error_entry *pos;
+	bool first = true;
+
+	list_for_each_entry(pos, &err->list, list) {
+		if (!first)
+			fputs("\n", stderr);
+		__parse_events_error__print(pos->idx, pos->str, pos->help, event);
+		first = false;
+	}
+}
 
-	__parse_events_error__print(err->idx, err->str, err->help, event);
+/*
+ * In the list of errors err, do any of the error strings (str) contain the
+ * given needle string?
+ */
+bool parse_events_error__contains(const struct parse_events_error *err,
+				  const char *needle)
+{
+	struct parse_events_error_entry *pos;
 
-	if (err->num_errors > 1) {
-		fputs("\nInitial error:\n", stderr);
-		__parse_events_error__print(err->first_idx, err->first_str,
-					err->first_help, event);
+	list_for_each_entry(pos, &err->list, list) {
+		if (strstr(pos->str, needle) != NULL)
+			return true;
 	}
+	return false;
 }
 
 #undef MAX_WIDTH
diff --git a/tools/perf/util/parse-events.h b/tools/perf/util/parse-events.h
index 63c0a36a4bf1..809359e8544e 100644
--- a/tools/perf/util/parse-events.h
+++ b/tools/perf/util/parse-events.h
@@ -130,13 +130,8 @@ struct parse_events_term {
 };
 
 struct parse_events_error {
-	int   num_errors;       /* number of errors encountered */
-	int   idx;	/* index in the parsed string */
-	char *str;      /* string to display at the index */
-	char *help;	/* optional help string */
-	int   first_idx;/* as above, but for the first encountered error */
-	char *first_str;
-	char *first_help;
+	/** @list: The head of a list of errors. */
+	struct list_head list;
 };
 
 /* A wrapper around a list of terms for the sake of better type safety. */
@@ -247,9 +242,10 @@ void parse_events_error__init(struct parse_events_error *err);
 void parse_events_error__exit(struct parse_events_error *err);
 void parse_events_error__handle(struct parse_events_error *err, int idx,
 				char *str, char *help);
-void parse_events_error__print(struct parse_events_error *err,
+void parse_events_error__print(const struct parse_events_error *err,
 			       const char *event);
-
+bool parse_events_error__contains(const struct parse_events_error *err,
+				  const char *needle);
 #ifdef HAVE_LIBELF_SUPPORT
 /*
  * If the probe point starts with '%',
diff --git a/tools/perf/util/parse-events.y b/tools/perf/util/parse-events.y
index de098caf0c1c..d70f5d84af92 100644
--- a/tools/perf/util/parse-events.y
+++ b/tools/perf/util/parse-events.y
@@ -536,8 +536,6 @@ tracepoint_name opt_event_config
 	list = alloc_list();
 	if (!list)
 		YYNOMEM;
-	if (error)
-		error->idx = @1.first_column;
 
 	err = parse_events_add_tracepoint(list, &parse_state->idx, $1.sys, $1.event,
 					error, $2, &@1);
-- 
2.43.0.429.g432eaa2c6b-goog

Re: [PATCH v2 3/3] perf parse-events: Print all errors

[James Clark]

On 31/01/2024 13:49, Ian Rogers wrote:
> Prior to this patch the first and the last error encountered during
> parsing are printed. To see other errors verbose needs
> enabling. Unfortunately this can drop useful errors, in particular on
> terms. This patch changes the errors so that instead of the first and
> last all errors are recorded and printed, the underlying data
> structure is changed to a list.
> 
> Before:
> ```
> $ perf stat -e 'slots/edge=2/' true
> event syntax error: 'slots/edge=2/'
>                                 \___ Bad event or PMU
> 
> Unable to find PMU or event on a PMU of 'slots'
> 
> Initial error:
> event syntax error: 'slots/edge=2/'
>                      \___ Cannot find PMU `slots'. Missing kernel support?
> Run 'perf list' for a list of valid events
> 
>  Usage: perf stat [<options>] [<command>]
> 
>     -e, --event <event>   event selector. use 'perf list' to list available events
> ```
> 
> After:
> ```
> $ perf stat -e 'slots/edge=2/' true
> event syntax error: 'slots/edge=2/'
>                      \___ Bad event or PMU
> 
> Unable to find PMU or event on a PMU of 'slots'
> 
> event syntax error: 'slots/edge=2/'
>                                 \___ value too big for format (edge), maximum is 1
> 
> event syntax error: 'slots/edge=2/'
>                      \___ Cannot find PMU `slots'. Missing kernel support?
> Run 'perf list' for a list of valid events
> 
>  Usage: perf stat [<options>] [<command>]
> 
>     -e, --event <event>   event selector. use 'perf list' to list available events
> ```
> 
> Signed-off-by: Ian Rogers <irogers@google.com>

Reviewed-by: James Clark <james.clark@arm.com>

> ---
> Prompted by discussion:
> https://lore.kernel.org/linux-perf-users/9dd303cb-0455-d8ac-ce0c-f4a8320b787b@arm.com/
> ---
>  tools/perf/arch/x86/tests/hybrid.c |  5 +-
>  tools/perf/tests/expand-cgroup.c   |  3 +-
>  tools/perf/tests/parse-events.c    |  9 ++-
>  tools/perf/util/parse-events.c     | 92 ++++++++++++++++++------------
>  tools/perf/util/parse-events.h     | 14 ++---
>  tools/perf/util/parse-events.y     |  2 -
>  6 files changed, 67 insertions(+), 58 deletions(-)
> 
> diff --git a/tools/perf/arch/x86/tests/hybrid.c b/tools/perf/arch/x86/tests/hybrid.c
> index 40f5d17fedab..e221ea104174 100644
> --- a/tools/perf/arch/x86/tests/hybrid.c
> +++ b/tools/perf/arch/x86/tests/hybrid.c
> @@ -259,11 +259,10 @@ static int test_event(const struct evlist_test *e)
>  	parse_events_error__init(&err);
>  	ret = parse_events(evlist, e->name, &err);
>  	if (ret) {
> -		pr_debug("failed to parse event '%s', err %d, str '%s'\n",
> -			 e->name, ret, err.str);
> +		pr_debug("failed to parse event '%s', err %d\n", e->name, ret);
>  		parse_events_error__print(&err, e->name);
>  		ret = TEST_FAIL;
> -		if (strstr(err.str, "can't access trace events"))
> +		if (parse_events_error__contains(&err, "can't access trace events"))
>  			ret = TEST_SKIP;
>  	} else {
>  		ret = e->check(evlist);
> diff --git a/tools/perf/tests/expand-cgroup.c b/tools/perf/tests/expand-cgroup.c
> index 9c1a1f18db75..31966ff856f8 100644
> --- a/tools/perf/tests/expand-cgroup.c
> +++ b/tools/perf/tests/expand-cgroup.c
> @@ -127,8 +127,7 @@ static int expand_group_events(void)
>  	parse_events_error__init(&err);
>  	ret = parse_events(evlist, event_str, &err);
>  	if (ret < 0) {
> -		pr_debug("failed to parse event '%s', err %d, str '%s'\n",
> -			 event_str, ret, err.str);
> +		pr_debug("failed to parse event '%s', err %d\n", event_str, ret);
>  		parse_events_error__print(&err, event_str);
>  		goto out;
>  	}
> diff --git a/tools/perf/tests/parse-events.c b/tools/perf/tests/parse-events.c
> index fbdf710d5eea..feb5727584d1 100644
> --- a/tools/perf/tests/parse-events.c
> +++ b/tools/perf/tests/parse-events.c
> @@ -2506,11 +2506,10 @@ static int test_event(const struct evlist_test *e)
>  	parse_events_error__init(&err);
>  	ret = parse_events(evlist, e->name, &err);
>  	if (ret) {
> -		pr_debug("failed to parse event '%s', err %d, str '%s'\n",
> -			 e->name, ret, err.str);
> +		pr_debug("failed to parse event '%s', err %d\n", e->name, ret);
>  		parse_events_error__print(&err, e->name);
>  		ret = TEST_FAIL;
> -		if (err.str && strstr(err.str, "can't access trace events"))
> +		if (parse_events_error__contains(&err, "can't access trace events"))
>  			ret = TEST_SKIP;
>  	} else {
>  		ret = e->check(evlist);
> @@ -2535,8 +2534,8 @@ static int test_event_fake_pmu(const char *str)
>  	ret = __parse_events(evlist, str, /*pmu_filter=*/NULL, &err,
>  			     &perf_pmu__fake, /*warn_if_reordered=*/true);
>  	if (ret) {
> -		pr_debug("failed to parse event '%s', err %d, str '%s'\n",
> -			 str, ret, err.str);
> +		pr_debug("failed to parse event '%s', err %d\n",
> +			 str, ret);
>  		parse_events_error__print(&err, str);
>  	}
>  
> diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
> index 66eabcea4242..6f8b0fa17689 100644
> --- a/tools/perf/util/parse-events.c
> +++ b/tools/perf/util/parse-events.c
> @@ -2181,50 +2181,53 @@ int parse_event(struct evlist *evlist, const char *str)
>  	return ret;
>  }
>  
> +struct parse_events_error_entry {
> +	/** @list: The list the error is part of. */
> +	struct list_head list;
> +	/** @idx: index in the parsed string */
> +	int   idx;
> +	/** @str: string to display at the index */
> +	char *str;
> +	/** @help: optional help string */
> +	char *help;
> +};
> +
>  void parse_events_error__init(struct parse_events_error *err)
>  {
> -	bzero(err, sizeof(*err));
> +	INIT_LIST_HEAD(&err->list);
>  }
>  
>  void parse_events_error__exit(struct parse_events_error *err)
>  {
> -	zfree(&err->str);
> -	zfree(&err->help);
> -	zfree(&err->first_str);
> -	zfree(&err->first_help);
> +	struct parse_events_error_entry *pos, *tmp;
> +
> +	list_for_each_entry_safe(pos, tmp, &err->list, list) {
> +		zfree(&pos->str);
> +		zfree(&pos->help);
> +		list_del_init(&pos->list);
> +		free(pos);
> +	}
>  }
>  
>  void parse_events_error__handle(struct parse_events_error *err, int idx,
>  				char *str, char *help)
>  {
> +	struct parse_events_error_entry *entry;
> +
>  	if (WARN(!str || !err, "WARNING: failed to provide error string or struct\n"))
>  		goto out_free;
> -	switch (err->num_errors) {
> -	case 0:
> -		err->idx = idx;
> -		err->str = str;
> -		err->help = help;
> -		break;
> -	case 1:
> -		err->first_idx = err->idx;
> -		err->idx = idx;
> -		err->first_str = err->str;
> -		err->str = str;
> -		err->first_help = err->help;
> -		err->help = help;
> -		break;
> -	default:
> -		pr_debug("Multiple errors dropping message: %s (%s)\n",
> -			err->str, err->help ?: "<no help>");
> -		free(err->str);
> -		err->str = str;
> -		free(err->help);
> -		err->help = help;
> -		break;
> +
> +	entry = zalloc(sizeof(*entry));
> +	if (!entry) {
> +		pr_err("Failed to allocate memory for event parsing error: %s (%s)\n",
> +			str, help ?: "<no help>");
> +		goto out_free;
>  	}
> -	err->num_errors++;
> +	entry->idx = idx;
> +	entry->str = str;
> +	entry->help = help;
> +	list_add(&entry->list, &err->list);
>  	return;
> -
>  out_free:
>  	free(str);
>  	free(help);
> @@ -2294,19 +2297,34 @@ static void __parse_events_error__print(int err_idx, const char *err_str,
>  	}
>  }
>  
> -void parse_events_error__print(struct parse_events_error *err,
> +void parse_events_error__print(const struct parse_events_error *err,
>  			       const char *event)
>  {
> -	if (!err->num_errors)
> -		return;
> +	struct parse_events_error_entry *pos;
> +	bool first = true;
> +
> +	list_for_each_entry(pos, &err->list, list) {
> +		if (!first)
> +			fputs("\n", stderr);
> +		__parse_events_error__print(pos->idx, pos->str, pos->help, event);
> +		first = false;
> +	}
> +}
>  
> -	__parse_events_error__print(err->idx, err->str, err->help, event);
> +/*
> + * In the list of errors err, do any of the error strings (str) contain the
> + * given needle string?
> + */
> +bool parse_events_error__contains(const struct parse_events_error *err,
> +				  const char *needle)
> +{
> +	struct parse_events_error_entry *pos;
>  
> -	if (err->num_errors > 1) {
> -		fputs("\nInitial error:\n", stderr);
> -		__parse_events_error__print(err->first_idx, err->first_str,
> -					err->first_help, event);
> +	list_for_each_entry(pos, &err->list, list) {
> +		if (strstr(pos->str, needle) != NULL)
> +			return true;
>  	}
> +	return false;
>  }
>  
>  #undef MAX_WIDTH
> diff --git a/tools/perf/util/parse-events.h b/tools/perf/util/parse-events.h
> index 63c0a36a4bf1..809359e8544e 100644
> --- a/tools/perf/util/parse-events.h
> +++ b/tools/perf/util/parse-events.h
> @@ -130,13 +130,8 @@ struct parse_events_term {
>  };
>  
>  struct parse_events_error {
> -	int   num_errors;       /* number of errors encountered */
> -	int   idx;	/* index in the parsed string */
> -	char *str;      /* string to display at the index */
> -	char *help;	/* optional help string */
> -	int   first_idx;/* as above, but for the first encountered error */
> -	char *first_str;
> -	char *first_help;
> +	/** @list: The head of a list of errors. */
> +	struct list_head list;
>  };
>  
>  /* A wrapper around a list of terms for the sake of better type safety. */
> @@ -247,9 +242,10 @@ void parse_events_error__init(struct parse_events_error *err);
>  void parse_events_error__exit(struct parse_events_error *err);
>  void parse_events_error__handle(struct parse_events_error *err, int idx,
>  				char *str, char *help);
> -void parse_events_error__print(struct parse_events_error *err,
> +void parse_events_error__print(const struct parse_events_error *err,
>  			       const char *event);
> -
> +bool parse_events_error__contains(const struct parse_events_error *err,
> +				  const char *needle);
>  #ifdef HAVE_LIBELF_SUPPORT
>  /*
>   * If the probe point starts with '%',
> diff --git a/tools/perf/util/parse-events.y b/tools/perf/util/parse-events.y
> index de098caf0c1c..d70f5d84af92 100644
> --- a/tools/perf/util/parse-events.y
> +++ b/tools/perf/util/parse-events.y
> @@ -536,8 +536,6 @@ tracepoint_name opt_event_config
>  	list = alloc_list();
>  	if (!list)
>  		YYNOMEM;
> -	if (error)
> -		error->idx = @1.first_column;
>  
>  	err = parse_events_add_tracepoint(list, &parse_state->idx, $1.sys, $1.event,
>  					error, $2, &@1);

Re: [PATCH v2 1/3] perf tsc: Add missing newlines to debug statements

[Ian Rogers]

On Wed, Jan 31, 2024 at 5:49 AM Ian Rogers <irogers@google.com> wrote:
>
> It is assumed that debug statements always print a newline, fix two
> missing ones.
>
> Signed-off-by: Ian Rogers <irogers@google.com>

Sorry forgot to add:
Reviewed-by: James Clark <james.clark@arm.com>
To this one.

Thanks,
Ian

> ---
> This patch was inspired by bad debug output in:
> https://lore.kernel.org/linux-perf-users/CAGjhMsg_bVKJ_zfsLUR32+oZwGDr3OiBHV_BJ3QtFjyKAs7Sgg@mail.gmail.com/
> ---
>  tools/perf/arch/x86/util/tsc.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/tools/perf/arch/x86/util/tsc.c b/tools/perf/arch/x86/util/tsc.c
> index 9b99f48b923c..e2d6cfe21057 100644
> --- a/tools/perf/arch/x86/util/tsc.c
> +++ b/tools/perf/arch/x86/util/tsc.c
> @@ -33,7 +33,7 @@ static double cpuinfo_tsc_freq(void)
>
>         cpuinfo = fopen("/proc/cpuinfo", "r");
>         if (!cpuinfo) {
> -               pr_err("Failed to read /proc/cpuinfo for TSC frequency");
> +               pr_err("Failed to read /proc/cpuinfo for TSC frequency\n");
>                 return NAN;
>         }
>         while (getline(&line, &len, cpuinfo) > 0) {
> @@ -48,7 +48,7 @@ static double cpuinfo_tsc_freq(void)
>         }
>  out:
>         if (fpclassify(result) == FP_ZERO)
> -               pr_err("Failed to find TSC frequency in /proc/cpuinfo");
> +               pr_err("Failed to find TSC frequency in /proc/cpuinfo\n");
>
>         free(line);
>         fclose(cpuinfo);
> --
> 2.43.0.429.g432eaa2c6b-goog
>

Re: [PATCH v2 1/3] perf tsc: Add missing newlines to debug statements

[Namhyung Kim]

On Wed, 31 Jan 2024 05:49:38 -0800, Ian Rogers wrote:
> It is assumed that debug statements always print a newline, fix two
> missing ones.
> 
> 

Applied to perf-tools-next, thanks!

[1/3] perf tsc: Add missing newlines to debug statements
      commit: 2882358b8b83b417a9d82205ae6aae5be00bd989
[2/3] perf parse-events: Improve error location of terms cloned from an event
      commit: f5144ecad74101f87843aa08070df26a3937102a
[3/3] perf parse-events: Print all errors
      commit: fd7b8e8fb20f51d60dfee7792806548f3c6a4c2c

Best regards,
-- 
Namhyung Kim <namhyung@kernel.org>