perf test: Test 6 dumps core on s390

Perf test case 6 "Parse event definition strings"
dumps core when executed on s390.
Root case is a NULL pointer supplied in function

test_event()
+---> parse_events()

The third parameter is set to NULL:
(gdb) where
#0 parse_events (evlist=0x149dc90, str=0x133242a "intel_pt//u", err=0x0)

	at util/parse-events.c:1835
 #1  0x00000000010d3d1c in test_event (e=0x14330e0 <test.events+1272>)
	at tests/parse-events.c:1696
 #2  0x00000000010d3e88 in test_events (events=0x1432be8 <test.events>, cnt=54)
	at tests/parse-events.c:1718
 #3  0x00000000010d44c0 in test__parse_events (test=0x142b500
		 <generic_tests+280>, subtest=-1) at tests/parse-events.c:1838

Function parse_events(xx, xx, struct parse_events_error *err) dives into a bison generated scanner and creates
parser state information for it first:

   struct parse_events_state parse_state = {
      .list   = LIST_HEAD_INIT(parse_state.list),
      .idx    = evlist->nr_entries,
      .error  = err,   <--- NULL POINTER !!!
      .evlist = evlist,
   };

Now various functions inside the bison scanner are called to end up in __parse_events_add_pmu(struct parse_events_state *parse_state, ..) with first parameter being a pointer to above structure definition.

Now the event name is not found (because being executed on s390) and this function tries to create an error message with

asprintf(&parse_state->error.str, ....)

which references above NULL pointer and dumps core.

Fix this by providing a pointer to the necessary error information instead of NULL.
Please note that the test still fails on non x86 platforms but for different and valid reason.

Output with this fix:
[root@s35lp76 perf]# ./perf test -vvvvv -F 6
6: Parse event definition strings :
--- start ---
running test 0 'syscalls:sys_enter_openat' Using CPUID IBM,3906,703,M03,3.5,002f running test 1 'syscalls:*'
running test 2 'r1a'
running test 3 '1:1'
running test 4 'instructions'
...
running test 51 'L1-dcache-misses/name=cachepmu/' running test 52 'intel_pt//u'
failed to parse event 'intel_pt//u', err 1 omitting PMU cpu tests
omitting PMU cpu tests
running test 0 'config=10,config1,config2=3,umask=1'
---- end ----
Parse event definition strings: FAILED!

---
 tools/perf/tests/parse-events.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

index b9ebe15afb13..f1012d7aea7a 100644
@@ -1686,6 +1686,7 @@ static struct terms_test test__terms[] = {
 
 static int test_event(struct evlist_test *e)
 {
+	struct parse_events_error errinfo;
 	struct perf_evlist *evlist;
 	int ret;
 

@@ -1693,7 +1694,7 @@ static int test_event(struct evlist_test *e)
 	if (evlist == NULL)
 		return -ENOMEM;
 
-	ret = parse_events(evlist, e->name, NULL);
+	ret = parse_events(evlist, e->name, &errinfo);
 	if (ret) {
 		pr_debug("failed to parse event '%s', err %d\n",
 			 e->name, ret);

2.14.3

I reported it actually fails on any $ARCH system without Intel Processor Trace (PT) h/w:

https://www.spinics.net/lists/linux-perf-users/msg06020.html

There was a follow-up patch sent here:

https://www.spinics.net/lists/linux-perf-users/msg06029.html

which worked for me, but I don't know what has/has-not transpired since, other than it is still broken, as you found.

Kim

Looks like this is still broken. I also can not find this patch on git clone -b perf/core git://git.kernel.org/pub/scm/linux/kernel/git/acme/linux

We actually need 2 patches
- one to fix the core dump
- one to test for x86 platform

Thomas Richter, Dept 3303, IBM s390 Linux Development, Boeblingen, Germany
--
Vorsitzende des Aufsichtsrats: Martina Koederitz 
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen / Registergericht: Amtsgericht Stuttgart, HRB 243294

right.. I forgot about those, will rebase/repost it

jirka

Add missing error handling for parse_events calls in test_event function that led to following segfault on s390:

running test 52 'intel_pt//u'
perf: Segmentation fault
...
/lib64/libc.so.6(vasprintf+0xe6) [0x3fffca3f106] /lib64/libc.so.6(asprintf+0x46) [0x3fffca1aa96] ./perf(parse_events_add_pmu+0xb8) [0x80132088] ./perf(parse_events_parse+0xc62) [0x8019529a] ./perf(parse_events+0x98) [0x801341c0] ./perf(test__parse_events+0x48) [0x800cd140] ./perf(cmd_test+0x26a) [0x800bd44a]
test child interrupted

Adding the struct parse_events_error argument to parse_events call. Also adding parse_events_print_error to get more details on the parsing failures, like:

  # perf test 6 -v
  running test 52 'intel_pt//u'failed to parse event 'intel_pt//u', err 1, str 'Cannot find PMU `intel_pt'. Missing kernel support?'
  event syntax error: 'intel_pt//u'
                       \___ Cannot find PMU `intel_pt'. Missing kernel support?

---
 tools/perf/tests/parse-events.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

index 7d4077068454..3b0bfdf5a594 100644
@@ -1686,6 +1686,7 @@ static struct terms_test test__terms[] = {
 
 static int test_event(struct evlist_test *e)
 {
+	struct parse_events_error err = { 0 };
 	struct perf_evlist *evlist;
 	int ret;
 

@@ -1693,10 +1694,11 @@ static int test_event(struct evlist_test *e)
 	if (evlist == NULL)
 		return -ENOMEM;
 
-	ret = parse_events(evlist, e->name, NULL);
+	ret = parse_events(evlist, e->name, &err);
 	if (ret) {
-		pr_debug("failed to parse event '%s', err %d\n",
-			 e->name, ret);
+		pr_debug("failed to parse event '%s', err %d, str '%s'\n",
+			 e->name, ret, err.str);
+		parse_events_print_error(&err, e->name);
 	} else {
 		ret = e->check(evlist);
 	}

2.13.6

Adding optional 'valid' callback for events tests in parse-events object, so we don't try to parse PMUs, which are not supported.

Following line is displayed for skipped test:

running test 52 'intel_pt//u'... SKIP

---
 tools/perf/tests/parse-events.c | 17 +++++++++++++++--
 1 file changed, 15 insertions(+), 2 deletions(-)

index 3b0bfdf5a594..347378ddaa8b 100644
@@ -1309,6 +1309,11 @@ static int test__checkevent_config_cache(struct perf_evlist *evlist)
 	return 0;
 }
 
+static bool test__intel_pt_valid(void)
+{
+	return !!perf_pmu__find("intel_pt");
+}
+
 static int test__intel_pt(struct perf_evlist *evlist)
 {
 	struct perf_evsel *evsel = perf_evlist__first(evlist);

@@ -1375,6 +1380,7 @@ struct evlist_test {
 	const char *name;
 	__u32 type;
 	const int id;
+	bool (*valid)(void);
 	int (*check)(struct perf_evlist *evlist);
 };
 

@@ -1648,6 +1654,7 @@ static struct evlist_test test__events[] = {
 	},
 	{
 		.name  = "intel_pt//u",
+		.valid = test__intel_pt_valid,
 		.check = test__intel_pt,
 		.id    = 52,
 	},

@@ -1690,6 +1697,11 @@ static int test_event(struct evlist_test *e)
 	struct perf_evlist *evlist;
 	int ret;
 
+	if (e->valid && !e->valid()) {
+		pr_debug("... SKIP");
+		return 0;
+	}
+
 	evlist = perf_evlist__new();
 	if (evlist == NULL)
 		return -ENOMEM;

@@ -1716,10 +1728,11 @@ static int test_events(struct evlist_test *events, unsigned cnt)
 	for (i = 0; i < cnt; i++) {
 		struct evlist_test *e = &events[i];
 
-		pr_debug("running test %d '%s'\n", e->id, e->name);
+		pr_debug("running test %d '%s'", e->id, e->name);
 		ret1 = test_event(e);
 		if (ret1)
 			ret2 = ret1;
+		pr_debug("\n");
 	}
 
 	return ret2;

@@ -1801,7 +1814,7 @@ static int test_pmu_events(void)
 	}
 
 	while (!ret && (ent = readdir(dir))) {
-		struct evlist_test e;
+		struct evlist_test e = { 0 };
 		char name[2 * NAME_MAX + 1 + 12 + 3];
 
 		/* Names containing . are special and cannot be used directly */

2.13.6

Em Mon, Jun 11, 2018 at 11:34:22AM +0200, Jiri Olsa escreveu:

Would be nice to test the _parsing_ of this even on machines where this PMU is not present, as developers using these machines as workstations may end up breaking this.

Anyway, intel_pt now is in tons of machines, so should be caught easily, if not by anyone, by me ;-\

- Arnaldo

like I said, this happens on any $ARCH machine without intel_pt hardware.

Other than that, for both 1 & 2 in this series:

[Reported-and-]Tested-by: Kim Phillips <[email protected]>

Thanks,

Kim