Set of fixes related to the comments here [1]. Mainly cleanups,
additional tests and refactoring since adding the new strcmp_cpuid_str()
metric expression.
I added the string replace function to the perf utils
rather than tools/lib/string.c because it didn't seem
easy to add tests for tools/lib.
[1]: https://lore.kernel.org/linux-arm-kernel/CAP-5=fVnUx0BnJC7X1rrm42OD7Bk=ZsHWNwAZMBYyB7yWhBfhQ@mail.gmail.com/
James Clark (7):
perf test: Check result of has_event(cycles) test
perf jevents: Remove unused keyword
perf util: Add a function for replacing characters in a string
perf test: Add a test for strcmp_cpuid_str() expression
perf pmu: Move pmu__find_core_pmu() to pmus.c
perf pmus: Simplify perf_pmus__find_core_pmu()
perf pmu: Remove unused function
tools/perf/arch/arm64/util/pmu.c | 20 ++++++-------
tools/perf/pmu-events/metric.py | 3 +-
tools/perf/tests/Build | 1 +
tools/perf/tests/builtin-test.c | 1 +
tools/perf/tests/expr.c | 33 ++++++++++++++++++----
tools/perf/tests/tests.h | 1 +
tools/perf/tests/util.c | 30 ++++++++++++++++++++
tools/perf/util/expr.c | 2 +-
tools/perf/util/pmu.c | 22 ---------------
tools/perf/util/pmu.h | 3 +-
tools/perf/util/pmus.c | 6 ++++
tools/perf/util/string.c | 48 ++++++++++++++++++++++++++++++++
tools/perf/util/string2.h | 1 +
13 files changed, 127 insertions(+), 44 deletions(-)
create mode 100644 tools/perf/tests/util.c
--
2.34.1
It finds all occurrences of a single character and replaces them with
a multi character string. This will be used in a test in a following
commit.
Signed-off-by: James Clark <[email protected]>
---
tools/perf/tests/Build | 1 +
tools/perf/tests/builtin-test.c | 1 +
tools/perf/tests/tests.h | 1 +
tools/perf/tests/util.c | 30 +++++++++++++++++++++
tools/perf/util/string.c | 48 +++++++++++++++++++++++++++++++++
tools/perf/util/string2.h | 1 +
6 files changed, 82 insertions(+)
create mode 100644 tools/perf/tests/util.c
diff --git a/tools/perf/tests/Build b/tools/perf/tests/Build
index 63d5e6d5f165..2b45ffa462a6 100644
--- a/tools/perf/tests/Build
+++ b/tools/perf/tests/Build
@@ -66,6 +66,7 @@ perf-y += dlfilter-test.o
perf-y += sigtrap.o
perf-y += event_groups.o
perf-y += symbols.o
+perf-y += util.o
ifeq ($(SRCARCH),$(filter $(SRCARCH),x86 arm arm64 powerpc))
perf-$(CONFIG_DWARF_UNWIND) += dwarf-unwind.o
diff --git a/tools/perf/tests/builtin-test.c b/tools/perf/tests/builtin-test.c
index 0ad18cf6dd22..cb6f1dd00dc4 100644
--- a/tools/perf/tests/builtin-test.c
+++ b/tools/perf/tests/builtin-test.c
@@ -123,6 +123,7 @@ static struct test_suite *generic_tests[] = {
&suite__sigtrap,
&suite__event_groups,
&suite__symbols,
+ &suite__util,
NULL,
};
diff --git a/tools/perf/tests/tests.h b/tools/perf/tests/tests.h
index f33cfc3c19a4..b394f3ac2d66 100644
--- a/tools/perf/tests/tests.h
+++ b/tools/perf/tests/tests.h
@@ -145,6 +145,7 @@ DECLARE_SUITE(dlfilter);
DECLARE_SUITE(sigtrap);
DECLARE_SUITE(event_groups);
DECLARE_SUITE(symbols);
+DECLARE_SUITE(util);
/*
* PowerPC and S390 do not support creation of instruction breakpoints using the
diff --git a/tools/perf/tests/util.c b/tools/perf/tests/util.c
new file mode 100644
index 000000000000..43e66a620b83
--- /dev/null
+++ b/tools/perf/tests/util.c
@@ -0,0 +1,30 @@
+// SPDX-License-Identifier: GPL-2.0
+#include "tests.h"
+#include "util/debug.h"
+
+#include <linux/compiler.h>
+#include <stdlib.h>
+#include <string2.h>
+
+static int test_strreplace(char find, const char *s, const char *replace, const char *expected)
+{
+ char *new = strreplace_chars(find, s, replace);
+ int ret = strcmp(new, expected);
+
+ free(new);
+ return ret == 0;
+}
+
+static int test__util(struct test_suite *t __maybe_unused, int subtest __maybe_unused)
+{
+ TEST_ASSERT_VAL("empty string", test_strreplace(' ', "", "123", ""));
+ TEST_ASSERT_VAL("no match", test_strreplace('5', "123", "4", "123"));
+ TEST_ASSERT_VAL("replace 1", test_strreplace('3', "123", "4", "124"));
+ TEST_ASSERT_VAL("replace 2", test_strreplace('a', "abcabc", "ef", "efbcefbc"));
+ TEST_ASSERT_VAL("replace long", test_strreplace('a', "abcabc", "longlong",
+ "longlongbclonglongbc"));
+
+ return 0;
+}
+
+DEFINE_SUITE("util", util);
diff --git a/tools/perf/util/string.c b/tools/perf/util/string.c
index cf05b0b56c57..6410a683183e 100644
--- a/tools/perf/util/string.c
+++ b/tools/perf/util/string.c
@@ -301,3 +301,51 @@ unsigned int hex(char c)
return c - 'a' + 10;
return c - 'A' + 10;
}
+
+
+/*
+ * Replace all occurrences of character 'find' in string s with string 'replace'
+ *
+ * The new string could be longer so a new string is returned which must
+ * be freed.
+ */
+char *strreplace_chars(char find, const char *s, const char *replace)
+{
+ int replace_len = strlen(replace);
+ char *new_s, *to;
+ const char *loc = strchr(s, find);
+ const char *from = s;
+ int num = 0;
+
+ /* Count occurrences */
+ while (loc) {
+ loc = strchr(loc + 1, find);
+ num++;
+ }
+
+ /* Allocate enough space for replacements and reset first location */
+ new_s = malloc(strlen(s) + (num * (replace_len - 1) + 1));
+ if (!new_s)
+ return NULL;
+ loc = strchr(s, find);
+ to = new_s;
+
+ while (loc) {
+ /* Copy original string up to found char and update positions */
+ memcpy(to, from, 1 + loc - from);
+ to += loc - from;
+ from = loc + 1;
+
+ /* Copy replacement string and update positions */
+ memcpy(to, replace, replace_len);
+ to += replace_len;
+
+ /* Find next occurrence or end of string */
+ loc = strchr(from, find);
+ }
+
+ /* Copy any remaining chars + null */
+ strcpy(to, from);
+
+ return new_s;
+}
diff --git a/tools/perf/util/string2.h b/tools/perf/util/string2.h
index 56c30fef9682..920488099214 100644
--- a/tools/perf/util/string2.h
+++ b/tools/perf/util/string2.h
@@ -39,5 +39,6 @@ char *strpbrk_esc(char *str, const char *stopset);
char *strdup_esc(const char *str);
unsigned int hex(char c);
+char *strreplace_chars(char find, const char *s, const char *replace);
#endif /* PERF_STRING_H */
--
2.34.1
Test that the new expression builtin returns a match when the current
escaped CPU ID is given, and that it doesn't match when "0x0" is given.
The CPU ID in test__expr() has to be changed to perf_pmu__getcpuid()
which returns the CPU ID string, rather than the raw CPU ID that
get_cpuid() returns because that can't be used with strcmp_cpuid_str().
It doesn't affect the is_intel test because both versions contain
"Intel".
Signed-off-by: James Clark <[email protected]>
---
tools/perf/tests/expr.c | 31 +++++++++++++++++++++++++++----
1 file changed, 27 insertions(+), 4 deletions(-)
diff --git a/tools/perf/tests/expr.c b/tools/perf/tests/expr.c
index 39be7f3b3a53..78544092ef0c 100644
--- a/tools/perf/tests/expr.c
+++ b/tools/perf/tests/expr.c
@@ -9,6 +9,7 @@
#include <math.h>
#include <stdlib.h>
#include <string.h>
+#include <string2.h>
#include <linux/zalloc.h>
static int test_ids_union(void)
@@ -74,10 +75,13 @@ static int test__expr(struct test_suite *t __maybe_unused, int subtest __maybe_u
int ret;
struct expr_parse_ctx *ctx;
bool is_intel = false;
- char buf[128];
+ char strcmp_cpuid_buf[256];
+ struct perf_pmu *pmu = pmu__find_core_pmu();
+ char *cpuid = perf_pmu__getcpuid(pmu);
+ char *escaped_cpuid1, *escaped_cpuid2;
- if (!get_cpuid(buf, sizeof(buf)))
- is_intel = strstr(buf, "Intel") != NULL;
+ TEST_ASSERT_VAL("get_cpuid", cpuid);
+ is_intel = strstr(cpuid, "Intel") != NULL;
TEST_ASSERT_EQUAL("ids_union", test_ids_union(), 0);
@@ -257,9 +261,28 @@ static int test__expr(struct test_suite *t __maybe_unused, int subtest __maybe_u
TEST_ASSERT_VAL("source count", hashmap__size(ctx->ids) == 1);
TEST_ASSERT_VAL("source count", hashmap__find(ctx->ids, "EVENT1", &val_ptr));
+
+ /* Test no cpuid match */
+ ret = test(ctx, "strcmp_cpuid_str(0x0)", 0);
+
+ /*
+ * Test cpuid match with current cpuid. Special chars have to be
+ * escaped.
+ */
+ escaped_cpuid1 = strreplace_chars('-', cpuid, "\\-");
+ free(cpuid);
+ escaped_cpuid2 = strreplace_chars(',', escaped_cpuid1, "\\,");
+ free(escaped_cpuid1);
+ escaped_cpuid1 = strreplace_chars('=', escaped_cpuid2, "\\=");
+ free(escaped_cpuid2);
+ scnprintf(strcmp_cpuid_buf, sizeof(strcmp_cpuid_buf),
+ "strcmp_cpuid_str(%s)", escaped_cpuid1);
+ free(escaped_cpuid1);
+ ret |= test(ctx, strcmp_cpuid_buf, 1);
+
/* has_event returns 1 when an event exists. */
expr__add_id_val(ctx, strdup("cycles"), 2);
- ret = test(ctx, "has_event(cycles)", 1);
+ ret |= test(ctx, "has_event(cycles)", 1);
expr__ctx_free(ctx);
--
2.34.1
pmu__find_core_pmu() more logically belongs in pmus.c because it
iterates over all PMUs, so move it to pmus.c
At the same time rename it to perf_pmus__find_core_pmu() to match the
naming convention in this file.
Signed-off-by: James Clark <[email protected]>
---
tools/perf/arch/arm64/util/pmu.c | 6 +++---
tools/perf/tests/expr.c | 2 +-
tools/perf/util/expr.c | 2 +-
tools/perf/util/pmu.c | 17 -----------------
tools/perf/util/pmu.h | 2 +-
tools/perf/util/pmus.c | 18 ++++++++++++++++++
6 files changed, 24 insertions(+), 23 deletions(-)
diff --git a/tools/perf/arch/arm64/util/pmu.c b/tools/perf/arch/arm64/util/pmu.c
index 615084eb88d8..3d9330feebd2 100644
--- a/tools/perf/arch/arm64/util/pmu.c
+++ b/tools/perf/arch/arm64/util/pmu.c
@@ -10,7 +10,7 @@
const struct pmu_metrics_table *pmu_metrics_table__find(void)
{
- struct perf_pmu *pmu = pmu__find_core_pmu();
+ struct perf_pmu *pmu = perf_pmus__find_core_pmu();
if (pmu)
return perf_pmu__find_metrics_table(pmu);
@@ -20,7 +20,7 @@ const struct pmu_metrics_table *pmu_metrics_table__find(void)
const struct pmu_events_table *pmu_events_table__find(void)
{
- struct perf_pmu *pmu = pmu__find_core_pmu();
+ struct perf_pmu *pmu = perf_pmus__find_core_pmu();
if (pmu)
return perf_pmu__find_events_table(pmu);
@@ -32,7 +32,7 @@ double perf_pmu__cpu_slots_per_cycle(void)
{
char path[PATH_MAX];
unsigned long long slots = 0;
- struct perf_pmu *pmu = pmu__find_core_pmu();
+ struct perf_pmu *pmu = perf_pmus__find_core_pmu();
if (pmu) {
perf_pmu__pathname_scnprintf(path, sizeof(path),
diff --git a/tools/perf/tests/expr.c b/tools/perf/tests/expr.c
index 78544092ef0c..e3aa9d4fcf3a 100644
--- a/tools/perf/tests/expr.c
+++ b/tools/perf/tests/expr.c
@@ -76,7 +76,7 @@ static int test__expr(struct test_suite *t __maybe_unused, int subtest __maybe_u
struct expr_parse_ctx *ctx;
bool is_intel = false;
char strcmp_cpuid_buf[256];
- struct perf_pmu *pmu = pmu__find_core_pmu();
+ struct perf_pmu *pmu = perf_pmus__find_core_pmu();
char *cpuid = perf_pmu__getcpuid(pmu);
char *escaped_cpuid1, *escaped_cpuid2;
diff --git a/tools/perf/util/expr.c b/tools/perf/util/expr.c
index 4488f306de78..7be23b3ac082 100644
--- a/tools/perf/util/expr.c
+++ b/tools/perf/util/expr.c
@@ -509,7 +509,7 @@ double expr__strcmp_cpuid_str(const struct expr_parse_ctx *ctx __maybe_unused,
bool compute_ids __maybe_unused, const char *test_id)
{
double ret;
- struct perf_pmu *pmu = pmu__find_core_pmu();
+ struct perf_pmu *pmu = perf_pmus__find_core_pmu();
char *cpuid = perf_pmu__getcpuid(pmu);
if (!cpuid)
diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c
index 152cda84f273..8213e26783a1 100644
--- a/tools/perf/util/pmu.c
+++ b/tools/perf/util/pmu.c
@@ -2049,20 +2049,3 @@ void perf_pmu__delete(struct perf_pmu *pmu)
zfree(&pmu->id);
free(pmu);
}
-
-struct perf_pmu *pmu__find_core_pmu(void)
-{
- struct perf_pmu *pmu = NULL;
-
- while ((pmu = perf_pmus__scan_core(pmu))) {
- /*
- * The cpumap should cover all CPUs. Otherwise, some CPUs may
- * not support some events or have different event IDs.
- */
- if (RC_CHK_ACCESS(pmu->cpus)->nr != cpu__max_cpu().cpu)
- return NULL;
-
- return pmu;
- }
- return NULL;
-}
diff --git a/tools/perf/util/pmu.h b/tools/perf/util/pmu.h
index 6a4e170c61d6..45079f26abf6 100644
--- a/tools/perf/util/pmu.h
+++ b/tools/perf/util/pmu.h
@@ -264,6 +264,6 @@ int perf_pmu__pathname_fd(int dirfd, const char *pmu_name, const char *filename,
struct perf_pmu *perf_pmu__lookup(struct list_head *pmus, int dirfd, const char *lookup_name);
struct perf_pmu *perf_pmu__create_placeholder_core_pmu(struct list_head *core_pmus);
void perf_pmu__delete(struct perf_pmu *pmu);
-struct perf_pmu *pmu__find_core_pmu(void);
+struct perf_pmu *perf_pmus__find_core_pmu(void);
#endif /* __PMU_H */
diff --git a/tools/perf/util/pmus.c b/tools/perf/util/pmus.c
index 6631367c756f..032ce57d2b8e 100644
--- a/tools/perf/util/pmus.c
+++ b/tools/perf/util/pmus.c
@@ -10,6 +10,7 @@
#include <pthread.h>
#include <string.h>
#include <unistd.h>
+#include "cpumap.h"
#include "debug.h"
#include "evsel.h"
#include "pmus.h"
@@ -592,3 +593,20 @@ struct perf_pmu *evsel__find_pmu(const struct evsel *evsel)
}
return pmu;
}
+
+struct perf_pmu *perf_pmus__find_core_pmu(void)
+{
+ struct perf_pmu *pmu = NULL;
+
+ while ((pmu = perf_pmus__scan_core(pmu))) {
+ /*
+ * The cpumap should cover all CPUs. Otherwise, some CPUs may
+ * not support some events or have different event IDs.
+ */
+ if (RC_CHK_ACCESS(pmu->cpus)->nr != cpu__max_cpu().cpu)
+ return NULL;
+
+ return pmu;
+ }
+ return NULL;
+}
--
2.34.1
Currently the while loop always either exits on the first iteration with
a core PMU, or exits with NULL on heterogeneous systems or when not all
CPUs are online.
Both of the latter behaviors are undesirable for platforms other than
Arm so simplify it to always return the first core PMU, or NULL if none
exist.
This behavior was depended on by the Arm version of
pmu_metrics_table__find(), so the logic has been moved there instead.
Suggested-by: Ian Rogers <[email protected]>
Signed-off-by: James Clark <[email protected]>
---
tools/perf/arch/arm64/util/pmu.c | 8 +++++++-
tools/perf/util/pmus.c | 14 +-------------
2 files changed, 8 insertions(+), 14 deletions(-)
diff --git a/tools/perf/arch/arm64/util/pmu.c b/tools/perf/arch/arm64/util/pmu.c
index 3d9330feebd2..3099f5f448ba 100644
--- a/tools/perf/arch/arm64/util/pmu.c
+++ b/tools/perf/arch/arm64/util/pmu.c
@@ -10,8 +10,14 @@
const struct pmu_metrics_table *pmu_metrics_table__find(void)
{
- struct perf_pmu *pmu = perf_pmus__find_core_pmu();
+ struct perf_pmu *pmu;
+
+ /* Metrics aren't currently supported on heterogeneous Arm systems */
+ if (perf_pmus__num_core_pmus() > 1)
+ return NULL;
+ /* Doesn't matter which one here because they'll all be the same */
+ pmu = perf_pmus__find_core_pmu();
if (pmu)
return perf_pmu__find_metrics_table(pmu);
diff --git a/tools/perf/util/pmus.c b/tools/perf/util/pmus.c
index 032ce57d2b8e..5ae41644ccda 100644
--- a/tools/perf/util/pmus.c
+++ b/tools/perf/util/pmus.c
@@ -596,17 +596,5 @@ struct perf_pmu *evsel__find_pmu(const struct evsel *evsel)
struct perf_pmu *perf_pmus__find_core_pmu(void)
{
- struct perf_pmu *pmu = NULL;
-
- while ((pmu = perf_pmus__scan_core(pmu))) {
- /*
- * The cpumap should cover all CPUs. Otherwise, some CPUs may
- * not support some events or have different event IDs.
- */
- if (RC_CHK_ACCESS(pmu->cpus)->nr != cpu__max_cpu().cpu)
- return NULL;
-
- return pmu;
- }
- return NULL;
+ return perf_pmus__scan_core(NULL);
}
--
2.34.1
Currently the function always returns 0, so even when the has_event()
test fails, the test still passes. Fix it by returning ret instead.
Signed-off-by: James Clark <[email protected]>
---
tools/perf/tests/expr.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tools/perf/tests/expr.c b/tools/perf/tests/expr.c
index 81229fa4f1e9..39be7f3b3a53 100644
--- a/tools/perf/tests/expr.c
+++ b/tools/perf/tests/expr.c
@@ -263,7 +263,7 @@ static int test__expr(struct test_suite *t __maybe_unused, int subtest __maybe_u
expr__ctx_free(ctx);
- return 0;
+ return ret;
}
DEFINE_SUITE("Simple expression parser", expr);
--
2.34.1
pmu_events_table__find() is no longer used so remove it and its Arm
specific version.
Signed-off-by: James Clark <[email protected]>
---
tools/perf/arch/arm64/util/pmu.c | 10 ----------
tools/perf/util/pmu.c | 5 -----
tools/perf/util/pmu.h | 1 -
3 files changed, 16 deletions(-)
diff --git a/tools/perf/arch/arm64/util/pmu.c b/tools/perf/arch/arm64/util/pmu.c
index 3099f5f448ba..2a4eab2d160e 100644
--- a/tools/perf/arch/arm64/util/pmu.c
+++ b/tools/perf/arch/arm64/util/pmu.c
@@ -24,16 +24,6 @@ const struct pmu_metrics_table *pmu_metrics_table__find(void)
return NULL;
}
-const struct pmu_events_table *pmu_events_table__find(void)
-{
- struct perf_pmu *pmu = perf_pmus__find_core_pmu();
-
- if (pmu)
- return perf_pmu__find_events_table(pmu);
-
- return NULL;
-}
-
double perf_pmu__cpu_slots_per_cycle(void)
{
char path[PATH_MAX];
diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c
index 8213e26783a1..f9d8525eec64 100644
--- a/tools/perf/util/pmu.c
+++ b/tools/perf/util/pmu.c
@@ -775,11 +775,6 @@ char *perf_pmu__getcpuid(struct perf_pmu *pmu)
return cpuid;
}
-__weak const struct pmu_events_table *pmu_events_table__find(void)
-{
- return perf_pmu__find_events_table(NULL);
-}
-
__weak const struct pmu_metrics_table *pmu_metrics_table__find(void)
{
return perf_pmu__find_metrics_table(NULL);
diff --git a/tools/perf/util/pmu.h b/tools/perf/util/pmu.h
index 45079f26abf6..4b9386534d2a 100644
--- a/tools/perf/util/pmu.h
+++ b/tools/perf/util/pmu.h
@@ -238,7 +238,6 @@ void pmu_add_cpu_aliases_table(struct perf_pmu *pmu,
const struct pmu_events_table *table);
char *perf_pmu__getcpuid(struct perf_pmu *pmu);
-const struct pmu_events_table *pmu_events_table__find(void);
const struct pmu_metrics_table *pmu_metrics_table__find(void);
int perf_pmu__convert_scale(const char *scale, char **end, double *sval);
--
2.34.1
'cpuid_not_more_than' was the working title of the new
'strcmp_cpuid_str' keyword and was accidentally left in. It was never
used so tidying it up has no effect.
Signed-off-by: James Clark <[email protected]>
---
tools/perf/pmu-events/metric.py | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/tools/perf/pmu-events/metric.py b/tools/perf/pmu-events/metric.py
index 0e9ec65d92ae..1bbb89102901 100644
--- a/tools/perf/pmu-events/metric.py
+++ b/tools/perf/pmu-events/metric.py
@@ -558,8 +558,7 @@ def ParsePerfJson(orig: str) -> Expression:
# Convert accidentally converted scientific notation constants back
py = re.sub(r'([0-9]+)Event\(r"(e[0-9]+)"\)', r'\1\2', py)
# Convert all the known keywords back from events to just the keyword
- keywords = ['if', 'else', 'min', 'max', 'd_ratio', 'source_count', 'has_event', 'strcmp_cpuid_str',
- 'cpuid_not_more_than']
+ keywords = ['if', 'else', 'min', 'max', 'd_ratio', 'source_count', 'has_event', 'strcmp_cpuid_str']
for kw in keywords:
py = re.sub(rf'Event\(r"{kw}"\)', kw, py)
try:
--
2.34.1
Em Thu, Aug 31, 2023 at 04:16:16PM +0100, James Clark escreveu:
> pmu__find_core_pmu() more logically belongs in pmus.c because it
> iterates over all PMUs, so move it to pmus.c
>
> At the same time rename it to perf_pmus__find_core_pmu() to match the
> naming convention in this file.
>
> Signed-off-by: James Clark <[email protected]>
So, this one is hitting this:
CC /tmp/build/perf-tools-next/util/expr.o
In file included from /var/home/acme/git/perf-tools-next/tools/include/linux/list.h:7,
from util/pmus.c:2:
In function ‘perf_pmus__scan_core’,
inlined from ‘perf_pmus__find_core_pmu’ at util/pmus.c:601:16:
/var/home/acme/git/perf-tools-next/tools/include/linux/kernel.h:36:45: error: array subscript 0 is outside array bounds of ‘struct list_head[1]’ [-Werror=array-bounds]
36 | const typeof(((type *)0)->member) * __mptr = (ptr); \
| ^~~~~~
/var/home/acme/git/perf-tools-next/tools/include/linux/list.h:352:9: note: in expansion of macro ‘container_of’
352 | container_of(ptr, type, member)
| ^~~~~~~~~~~~
/var/home/acme/git/perf-tools-next/tools/include/linux/list.h:404:9: note: in expansion of macro ‘list_entry’
404 | list_entry((pos)->member.next, typeof(*(pos)), member)
| ^~~~~~~~~~
/var/home/acme/git/perf-tools-next/tools/include/linux/list.h:494:20: note: in expansion of macro ‘list_next_entry’
494 | for (pos = list_next_entry(pos, member); \
| ^~~~~~~~~~~~~~~
util/pmus.c:274:9: note: in expansion of macro ‘list_for_each_entry_continue’
274 | list_for_each_entry_continue(pmu, &core_pmus, list)
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
util/pmus.c: In function ‘perf_pmus__find_core_pmu’:
util/pmus.c:35:18: note: at offset -128 into object ‘core_pmus’ of size 16
35 | static LIST_HEAD(core_pmus);
| ^~~~~~~~~
/var/home/acme/git/perf-tools-next/tools/include/linux/list.h:23:26: note: in definition of macro ‘LIST_HEAD’
23 | struct list_head name = LIST_HEAD_INIT(name)
| ^~~~
cc1: all warnings being treated as errors
make[4]: *** [/var/home/acme/git/perf-tools-next/tools/build/Makefile.build:97: /tmp/build/perf-tools-next/util/pmus.o] Error 1
make[4]: *** Waiting for unfinished jobs....
LD /tmp/build/perf-tools-next/ui/browsers/perf-in.o
So I applied up to 4/7
Please continue from what will be in tmp.perf-tools-next in some
jiffies.
- Arnaldo
On 12/09/2023 20:26, Arnaldo Carvalho de Melo wrote:
> Em Thu, Aug 31, 2023 at 04:16:16PM +0100, James Clark escreveu:
>> pmu__find_core_pmu() more logically belongs in pmus.c because it
>> iterates over all PMUs, so move it to pmus.c
>>
>> At the same time rename it to perf_pmus__find_core_pmu() to match the
>> naming convention in this file.
>>
>> Signed-off-by: James Clark <[email protected]>
>
> So, this one is hitting this:
>
> CC /tmp/build/perf-tools-next/util/expr.o
> In file included from /var/home/acme/git/perf-tools-next/tools/include/linux/list.h:7,
> from util/pmus.c:2:
> In function ‘perf_pmus__scan_core’,
> inlined from ‘perf_pmus__find_core_pmu’ at util/pmus.c:601:16:
> /var/home/acme/git/perf-tools-next/tools/include/linux/kernel.h:36:45: error: array subscript 0 is outside array bounds of ‘struct list_head[1]’ [-Werror=array-bounds]
> 36 | const typeof(((type *)0)->member) * __mptr = (ptr); \
> | ^~~~~~
> /var/home/acme/git/perf-tools-next/tools/include/linux/list.h:352:9: note: in expansion of macro ‘container_of’
> 352 | container_of(ptr, type, member)
> | ^~~~~~~~~~~~
> /var/home/acme/git/perf-tools-next/tools/include/linux/list.h:404:9: note: in expansion of macro ‘list_entry’
> 404 | list_entry((pos)->member.next, typeof(*(pos)), member)
> | ^~~~~~~~~~
> /var/home/acme/git/perf-tools-next/tools/include/linux/list.h:494:20: note: in expansion of macro ‘list_next_entry’
> 494 | for (pos = list_next_entry(pos, member); \
> | ^~~~~~~~~~~~~~~
> util/pmus.c:274:9: note: in expansion of macro ‘list_for_each_entry_continue’
> 274 | list_for_each_entry_continue(pmu, &core_pmus, list)
> | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
> util/pmus.c: In function ‘perf_pmus__find_core_pmu’:
> util/pmus.c:35:18: note: at offset -128 into object ‘core_pmus’ of size 16
> 35 | static LIST_HEAD(core_pmus);
> | ^~~~~~~~~
> /var/home/acme/git/perf-tools-next/tools/include/linux/list.h:23:26: note: in definition of macro ‘LIST_HEAD’
> 23 | struct list_head name = LIST_HEAD_INIT(name)
> | ^~~~
> cc1: all warnings being treated as errors
> make[4]: *** [/var/home/acme/git/perf-tools-next/tools/build/Makefile.build:97: /tmp/build/perf-tools-next/util/pmus.o] Error 1
> make[4]: *** Waiting for unfinished jobs....
> LD /tmp/build/perf-tools-next/ui/browsers/perf-in.o
>
>
> So I applied up to 4/7
>
> Please continue from what will be in tmp.perf-tools-next in some
> jiffies.
>
> - Arnaldo
I wasn't able to reproduce this on x86 or Arm, with either Clang or GCC.
That was with this patch applied onto 999b81b907e on tmp.perf-tools-next
and a pretty normal "make WERROR=1" command.
It seems like the 0 here is just to get the type rather than access
anything, if that's the 0 that the "array subscript 0" error is about,
so something seems a bit strange:
> /var/home/acme/git/perf-tools-next/tools/include/linux/kernel.h:36:45:
error: array subscript 0 is outside array bounds of ‘struct
list_head[1]’ [-Werror=array-bounds]
> 36 | const typeof(((type *)0)->member) * __mptr = (ptr); \
On 13/09/2023 11:20, James Clark wrote:
>
>
> On 12/09/2023 20:26, Arnaldo Carvalho de Melo wrote:
>> Em Thu, Aug 31, 2023 at 04:16:16PM +0100, James Clark escreveu:
>>> pmu__find_core_pmu() more logically belongs in pmus.c because it
>>> iterates over all PMUs, so move it to pmus.c
>>>
>>> At the same time rename it to perf_pmus__find_core_pmu() to match the
>>> naming convention in this file.
>>>
>>> Signed-off-by: James Clark <[email protected]>
>>
>> So, this one is hitting this:
>>
>> CC /tmp/build/perf-tools-next/util/expr.o
>> In file included from /var/home/acme/git/perf-tools-next/tools/include/linux/list.h:7,
>> from util/pmus.c:2:
>> In function ‘perf_pmus__scan_core’,
>> inlined from ‘perf_pmus__find_core_pmu’ at util/pmus.c:601:16:
>> /var/home/acme/git/perf-tools-next/tools/include/linux/kernel.h:36:45: error: array subscript 0 is outside array bounds of ‘struct list_head[1]’ [-Werror=array-bounds]
>> 36 | const typeof(((type *)0)->member) * __mptr = (ptr); \
>> | ^~~~~~
>> /var/home/acme/git/perf-tools-next/tools/include/linux/list.h:352:9: note: in expansion of macro ‘container_of’
>> 352 | container_of(ptr, type, member)
>> | ^~~~~~~~~~~~
>> /var/home/acme/git/perf-tools-next/tools/include/linux/list.h:404:9: note: in expansion of macro ‘list_entry’
>> 404 | list_entry((pos)->member.next, typeof(*(pos)), member)
>> | ^~~~~~~~~~
>> /var/home/acme/git/perf-tools-next/tools/include/linux/list.h:494:20: note: in expansion of macro ‘list_next_entry’
>> 494 | for (pos = list_next_entry(pos, member); \
>> | ^~~~~~~~~~~~~~~
>> util/pmus.c:274:9: note: in expansion of macro ‘list_for_each_entry_continue’
>> 274 | list_for_each_entry_continue(pmu, &core_pmus, list)
>> | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
>> util/pmus.c: In function ‘perf_pmus__find_core_pmu’:
>> util/pmus.c:35:18: note: at offset -128 into object ‘core_pmus’ of size 16
>> 35 | static LIST_HEAD(core_pmus);
>> | ^~~~~~~~~
>> /var/home/acme/git/perf-tools-next/tools/include/linux/list.h:23:26: note: in definition of macro ‘LIST_HEAD’
>> 23 | struct list_head name = LIST_HEAD_INIT(name)
>> | ^~~~
>> cc1: all warnings being treated as errors
>> make[4]: *** [/var/home/acme/git/perf-tools-next/tools/build/Makefile.build:97: /tmp/build/perf-tools-next/util/pmus.o] Error 1
>> make[4]: *** Waiting for unfinished jobs....
>> LD /tmp/build/perf-tools-next/ui/browsers/perf-in.o
>>
>>
>> So I applied up to 4/7
>>
>> Please continue from what will be in tmp.perf-tools-next in some
>> jiffies.
>>
>> - Arnaldo
>
> I wasn't able to reproduce this on x86 or Arm, with either Clang or GCC.
>
> That was with this patch applied onto 999b81b907e on tmp.perf-tools-next
> and a pretty normal "make WERROR=1" command.
>
> It seems like the 0 here is just to get the type rather than access
> anything, if that's the 0 that the "array subscript 0" error is about,
> so something seems a bit strange:
>
>> /var/home/acme/git/perf-tools-next/tools/include/linux/kernel.h:36:45:
> error: array subscript 0 is outside array bounds of ‘struct
> list_head[1]’ [-Werror=array-bounds]
>> 36 | const typeof(((type *)0)->member) * __mptr = (ptr); \
Nevermind, I managed to reproduce it. With a DEBUG=1 build on x86 there
is no error, it only happens with a non debug one.
I will look into it.
On 13/09/2023 11:32, James Clark wrote:
>
>
> On 13/09/2023 11:20, James Clark wrote:
>>
>>
>> On 12/09/2023 20:26, Arnaldo Carvalho de Melo wrote:
>>> Em Thu, Aug 31, 2023 at 04:16:16PM +0100, James Clark escreveu:
>>>> pmu__find_core_pmu() more logically belongs in pmus.c because it
>>>> iterates over all PMUs, so move it to pmus.c
>>>>
>>>> At the same time rename it to perf_pmus__find_core_pmu() to match the
>>>> naming convention in this file.
>>>>
>>>> Signed-off-by: James Clark <[email protected]>
>>>
>>> So, this one is hitting this:
>>>
>>> CC /tmp/build/perf-tools-next/util/expr.o
>>> In file included from /var/home/acme/git/perf-tools-next/tools/include/linux/list.h:7,
>>> from util/pmus.c:2:
>>> In function ‘perf_pmus__scan_core’,
>>> inlined from ‘perf_pmus__find_core_pmu’ at util/pmus.c:601:16:
>>> /var/home/acme/git/perf-tools-next/tools/include/linux/kernel.h:36:45: error: array subscript 0 is outside array bounds of ‘struct list_head[1]’ [-Werror=array-bounds]
>>> 36 | const typeof(((type *)0)->member) * __mptr = (ptr); \
>>> | ^~~~~~
>>> /var/home/acme/git/perf-tools-next/tools/include/linux/list.h:352:9: note: in expansion of macro ‘container_of’
>>> 352 | container_of(ptr, type, member)
>>> | ^~~~~~~~~~~~
>>> /var/home/acme/git/perf-tools-next/tools/include/linux/list.h:404:9: note: in expansion of macro ‘list_entry’
>>> 404 | list_entry((pos)->member.next, typeof(*(pos)), member)
>>> | ^~~~~~~~~~
>>> /var/home/acme/git/perf-tools-next/tools/include/linux/list.h:494:20: note: in expansion of macro ‘list_next_entry’
>>> 494 | for (pos = list_next_entry(pos, member); \
>>> | ^~~~~~~~~~~~~~~
>>> util/pmus.c:274:9: note: in expansion of macro ‘list_for_each_entry_continue’
>>> 274 | list_for_each_entry_continue(pmu, &core_pmus, list)
>>> | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>> util/pmus.c: In function ‘perf_pmus__find_core_pmu’:
>>> util/pmus.c:35:18: note: at offset -128 into object ‘core_pmus’ of size 16
>>> 35 | static LIST_HEAD(core_pmus);
>>> | ^~~~~~~~~
>>> /var/home/acme/git/perf-tools-next/tools/include/linux/list.h:23:26: note: in definition of macro ‘LIST_HEAD’
>>> 23 | struct list_head name = LIST_HEAD_INIT(name)
>>> | ^~~~
>>> cc1: all warnings being treated as errors
>>> make[4]: *** [/var/home/acme/git/perf-tools-next/tools/build/Makefile.build:97: /tmp/build/perf-tools-next/util/pmus.o] Error 1
>>> make[4]: *** Waiting for unfinished jobs....
>>> LD /tmp/build/perf-tools-next/ui/browsers/perf-in.o
>>>
>>>
>>> So I applied up to 4/7
>>>
>>> Please continue from what will be in tmp.perf-tools-next in some
>>> jiffies.
>>>
>>> - Arnaldo
>>
>> I wasn't able to reproduce this on x86 or Arm, with either Clang or GCC.
>>
>> That was with this patch applied onto 999b81b907e on tmp.perf-tools-next
>> and a pretty normal "make WERROR=1" command.
>>
>> It seems like the 0 here is just to get the type rather than access
>> anything, if that's the 0 that the "array subscript 0" error is about,
>> so something seems a bit strange:
>>
>>> /var/home/acme/git/perf-tools-next/tools/include/linux/kernel.h:36:45:
>> error: array subscript 0 is outside array bounds of ‘struct
>> list_head[1]’ [-Werror=array-bounds]
>>> 36 | const typeof(((type *)0)->member) * __mptr = (ptr); \
>
> Nevermind, I managed to reproduce it. With a DEBUG=1 build on x86 there
> is no error, it only happens with a non debug one.
>
> I will look into it.
Sent a v3 with the fix. It's some kind of awkward semi-undefined
behavior in the linked list implementation that was always there but the
compiler could only see when I moved that function so it was all in one
compilation unit. It also required -O2 and I always build with DEBUG=1
so I missed it.