LinuxLists.cc - [PATCH] perf tests: Fix false TEST

2015-12-15 04:19:40

Subject: [PATCH] perf tests: Fix false TEST_OK result for 'perf test hist'

Commit 71d6de64feddd4b455555326fba2111b3006d9e0 ('perf test: Fix hist
testcases when kptr_restrict is on') solves a double free problem when
'perf test hist' calling setup_fake_machine(). However, the result is
still incorrect. For example:

$ ./perf test -v 'filtering hist entries'
25: Test filtering hist entries :
--- start ---
test child forked, pid 4186
Cannot create kernel maps
test child finished with 0
---- end ----
Test filtering hist entries: Ok

In this case the body of this test is not get executed at all, but the
result is 'Ok'. I think 'Skip' should be better.

The reason is that *ALL* hists test cases forget to reset err after
using it to hold an error code.

This patch ensure err is reset.

In case when kptr_restrict prevent normal user get kernel address, this
test should be skipped, not fail. This patch uses linux/err.h to store
error code in return value of setup_fake_machine(), and let 'EACCES' to
indicate this problem.

Signed-off-by: Wang Nan <[email protected]>
Cc: Arnaldo Carvalho de Melo <[email protected]>
Cc: Jiri Olsa <[email protected]>
Cc: Masami Hiramatsu <[email protected]>
Cc: Namhyung Kim <[email protected]>
---
tools/perf/tests/hists_common.c | 12 +++++++++---
tools/perf/tests/hists_common.h | 1 +
tools/perf/tests/hists_cumulate.c | 6 +++++-
tools/perf/tests/hists_filter.c | 6 +++++-
tools/perf/tests/hists_link.c | 6 +++++-
tools/perf/tests/hists_output.c | 6 +++++-
6 files changed, 30 insertions(+), 7 deletions(-)

diff --git a/tools/perf/tests/hists_common.c b/tools/perf/tests/hists_common.c
index bcfd081..2eca1b2 100644
--- a/tools/perf/tests/hists_common.c
+++ b/tools/perf/tests/hists_common.c
@@ -81,15 +81,20 @@ struct machine *setup_fake_machine(struct machines *machines)
{
struct machine *machine = machines__find(machines, HOST_KERNEL_ID);
size_t i;
+ int err = -ENOMEM;

if (machine == NULL) {
pr_debug("Not enough memory for machine setup\n");
- return NULL;
+ return ERR_PTR(-ENOMEM);
}

if (machine__create_kernel_maps(machine)) {
pr_debug("Cannot create kernel maps\n");
- return NULL;
+ if (symbol_conf.kptr_restrict) {
+ pr_debug("Hint: Check /proc/sys/kernel/kptr_restrict.\n");
+ err = -EACCES;
+ }
+ return ERR_PTR(err);
}

for (i = 0; i < ARRAY_SIZE(fake_threads); i++) {
@@ -155,7 +160,8 @@ struct machine *setup_fake_machine(struct machines *machines)
out:
pr_debug("Not enough memory for machine setup\n");
machine__delete_threads(machine);
- return NULL;
+ machine__exit(machine);
+ return ERR_PTR(err);
}

void print_hists_in(struct hists *hists)
diff --git a/tools/perf/tests/hists_common.h b/tools/perf/tests/hists_common.h
index 888254e..0252eae 100644
--- a/tools/perf/tests/hists_common.h
+++ b/tools/perf/tests/hists_common.h
@@ -1,5 +1,6 @@
#ifndef __PERF_TESTS__HISTS_COMMON_H__
#define __PERF_TESTS__HISTS_COMMON_H__
+#include <linux/err.h>

struct machine;
struct machines;
diff --git a/tools/perf/tests/hists_cumulate.c b/tools/perf/tests/hists_cumulate.c
index 8292948..c211075 100644
--- a/tools/perf/tests/hists_cumulate.c
+++ b/tools/perf/tests/hists_cumulate.c
@@ -706,13 +706,17 @@ int test__hists_cumulate(int subtest __maybe_unused)
err = parse_events(evlist, "cpu-clock", NULL);
if (err)
goto out;
+ err = TEST_FAIL;

machines__init(&machines);

/* setup threads/dso/map/symbols also */
machine = setup_fake_machine(&machines);
- if (!machine)
+ if (IS_ERR(machine)) {
+ if (PTR_ERR(machine) == -EACCES)
+ err = TEST_SKIP;
goto out;
+ }

if (verbose > 1)
machine__fprintf(machine, stderr);
diff --git a/tools/perf/tests/hists_filter.c b/tools/perf/tests/hists_filter.c
index ccb5b49..795a04d 100644
--- a/tools/perf/tests/hists_filter.c
+++ b/tools/perf/tests/hists_filter.c
@@ -120,6 +120,7 @@ int test__hists_filter(int subtest __maybe_unused)
err = parse_events(evlist, "task-clock", NULL);
if (err)
goto out;
+ err = TEST_FAIL;

/* default sort order (comm,dso,sym) will be used */
if (setup_sorting() < 0)
@@ -129,8 +130,11 @@ int test__hists_filter(int subtest __maybe_unused)

/* setup threads/dso/map/symbols also */
machine = setup_fake_machine(&machines);
- if (!machine)
+ if (IS_ERR(machine)) {
+ if (PTR_ERR(machine) == -EACCES)
+ err = TEST_SKIP;
goto out;
+ }

if (verbose > 1)
machine__fprintf(machine, stderr);
diff --git a/tools/perf/tests/hists_link.c b/tools/perf/tests/hists_link.c
index 6243e2b..f5f37ef9 100644
--- a/tools/perf/tests/hists_link.c
+++ b/tools/perf/tests/hists_link.c
@@ -293,6 +293,7 @@ int test__hists_link(int subtest __maybe_unused)
if (err)
goto out;

+ err = TEST_FAIL;
/* default sort order (comm,dso,sym) will be used */
if (setup_sorting() < 0)
goto out;
@@ -301,8 +302,11 @@ int test__hists_link(int subtest __maybe_unused)

/* setup threads/dso/map/symbols also */
machine = setup_fake_machine(&machines);
- if (!machine)
+ if (IS_ERR(machine)) {
+ if (PTR_ERR(machine) == -EACCES)
+ err = TEST_SKIP;
goto out;
+ }

if (verbose > 1)
machine__fprintf(machine, stderr);
diff --git a/tools/perf/tests/hists_output.c b/tools/perf/tests/hists_output.c
index 248beec..cc570cf 100644
--- a/tools/perf/tests/hists_output.c
+++ b/tools/perf/tests/hists_output.c
@@ -597,13 +597,17 @@ int test__hists_output(int subtest __maybe_unused)
err = parse_events(evlist, "cpu-clock", NULL);
if (err)
goto out;
+ err = TEST_FAIL;

machines__init(&machines);

/* setup threads/dso/map/symbols also */
machine = setup_fake_machine(&machines);
- if (!machine)
+ if (IS_ERR(machine)) {
+ if (PTR_ERR(machine) == -EACCES)
+ err = TEST_SKIP;
goto out;
+ }

if (verbose > 1)
machine__fprintf(machine, stderr);
--
1.8.3.4

2015-12-15 07:53:45

by Namhyung Kim

[permalink] [raw]

Subject: Re: [PATCH] perf tests: Fix false TEST_OK result for 'perf test hist'

Hi,

On Tue, Dec 15, 2015 at 04:18:44AM +0000, Wang Nan wrote:
> Commit 71d6de64feddd4b455555326fba2111b3006d9e0 ('perf test: Fix hist
> testcases when kptr_restrict is on') solves a double free problem when
> 'perf test hist' calling setup_fake_machine(). However, the result is
> still incorrect. For example:
>
> $ ./perf test -v 'filtering hist entries'
> 25: Test filtering hist entries :
> --- start ---
> test child forked, pid 4186
> Cannot create kernel maps
> test child finished with 0
> ---- end ----
> Test filtering hist entries: Ok
>
> In this case the body of this test is not get executed at all, but the
> result is 'Ok'. I think 'Skip' should be better.

Right, I missed this part.

But on second thought, I think it should not create real kernel maps.
What setup_fake_machine() does is to create/insert fake
dso/mmap/symbols for testing hist functions. So it has to use the
predefined info not real ones.

>
> The reason is that *ALL* hists test cases forget to reset err after
> using it to hold an error code.
>
> This patch ensure err is reset.
>
> In case when kptr_restrict prevent normal user get kernel address, this
> test should be skipped, not fail. This patch uses linux/err.h to store
> error code in return value of setup_fake_machine(), and let 'EACCES' to
> indicate this problem.

Please just remove the machine__create_kernel_maps() in
setup_fake_machine() and simply return TEST_FAIL if there's an error.

Thanks,
Namhyung

>
> Signed-off-by: Wang Nan <[email protected]>
> Cc: Arnaldo Carvalho de Melo <[email protected]>
> Cc: Jiri Olsa <[email protected]>
> Cc: Masami Hiramatsu <[email protected]>
> Cc: Namhyung Kim <[email protected]>
> ---
> tools/perf/tests/hists_common.c | 12 +++++++++---
> tools/perf/tests/hists_common.h | 1 +
> tools/perf/tests/hists_cumulate.c | 6 +++++-
> tools/perf/tests/hists_filter.c | 6 +++++-
> tools/perf/tests/hists_link.c | 6 +++++-
> tools/perf/tests/hists_output.c | 6 +++++-
> 6 files changed, 30 insertions(+), 7 deletions(-)
>
> diff --git a/tools/perf/tests/hists_common.c b/tools/perf/tests/hists_common.c
> index bcfd081..2eca1b2 100644
> --- a/tools/perf/tests/hists_common.c
> +++ b/tools/perf/tests/hists_common.c
> @@ -81,15 +81,20 @@ struct machine *setup_fake_machine(struct machines *machines)
> {
> struct machine *machine = machines__find(machines, HOST_KERNEL_ID);
> size_t i;
> + int err = -ENOMEM;
>
> if (machine == NULL) {
> pr_debug("Not enough memory for machine setup\n");
> - return NULL;
> + return ERR_PTR(-ENOMEM);
> }
>
> if (machine__create_kernel_maps(machine)) {
> pr_debug("Cannot create kernel maps\n");
> - return NULL;
> + if (symbol_conf.kptr_restrict) {
> + pr_debug("Hint: Check /proc/sys/kernel/kptr_restrict.\n");
> + err = -EACCES;
> + }
> + return ERR_PTR(err);
> }
>
> for (i = 0; i < ARRAY_SIZE(fake_threads); i++) {
> @@ -155,7 +160,8 @@ struct machine *setup_fake_machine(struct machines *machines)
> out:
> pr_debug("Not enough memory for machine setup\n");
> machine__delete_threads(machine);
> - return NULL;
> + machine__exit(machine);
> + return ERR_PTR(err);
> }
>
> void print_hists_in(struct hists *hists)
> diff --git a/tools/perf/tests/hists_common.h b/tools/perf/tests/hists_common.h
> index 888254e..0252eae 100644
> --- a/tools/perf/tests/hists_common.h
> +++ b/tools/perf/tests/hists_common.h
> @@ -1,5 +1,6 @@
> #ifndef __PERF_TESTS__HISTS_COMMON_H__
> #define __PERF_TESTS__HISTS_COMMON_H__
> +#include <linux/err.h>
>
> struct machine;
> struct machines;
> diff --git a/tools/perf/tests/hists_cumulate.c b/tools/perf/tests/hists_cumulate.c
> index 8292948..c211075 100644
> --- a/tools/perf/tests/hists_cumulate.c
> +++ b/tools/perf/tests/hists_cumulate.c
> @@ -706,13 +706,17 @@ int test__hists_cumulate(int subtest __maybe_unused)
> err = parse_events(evlist, "cpu-clock", NULL);
> if (err)
> goto out;
> + err = TEST_FAIL;
>
> machines__init(&machines);
>
> /* setup threads/dso/map/symbols also */
> machine = setup_fake_machine(&machines);
> - if (!machine)
> + if (IS_ERR(machine)) {
> + if (PTR_ERR(machine) == -EACCES)
> + err = TEST_SKIP;
> goto out;
> + }
>
> if (verbose > 1)
> machine__fprintf(machine, stderr);
> diff --git a/tools/perf/tests/hists_filter.c b/tools/perf/tests/hists_filter.c
> index ccb5b49..795a04d 100644
> --- a/tools/perf/tests/hists_filter.c
> +++ b/tools/perf/tests/hists_filter.c
> @@ -120,6 +120,7 @@ int test__hists_filter(int subtest __maybe_unused)
> err = parse_events(evlist, "task-clock", NULL);
> if (err)
> goto out;
> + err = TEST_FAIL;
>
> /* default sort order (comm,dso,sym) will be used */
> if (setup_sorting() < 0)
> @@ -129,8 +130,11 @@ int test__hists_filter(int subtest __maybe_unused)
>
> /* setup threads/dso/map/symbols also */
> machine = setup_fake_machine(&machines);
> - if (!machine)
> + if (IS_ERR(machine)) {
> + if (PTR_ERR(machine) == -EACCES)
> + err = TEST_SKIP;
> goto out;
> + }
>
> if (verbose > 1)
> machine__fprintf(machine, stderr);
> diff --git a/tools/perf/tests/hists_link.c b/tools/perf/tests/hists_link.c
> index 6243e2b..f5f37ef9 100644
> --- a/tools/perf/tests/hists_link.c
> +++ b/tools/perf/tests/hists_link.c
> @@ -293,6 +293,7 @@ int test__hists_link(int subtest __maybe_unused)
> if (err)
> goto out;
>
> + err = TEST_FAIL;
> /* default sort order (comm,dso,sym) will be used */
> if (setup_sorting() < 0)
> goto out;
> @@ -301,8 +302,11 @@ int test__hists_link(int subtest __maybe_unused)
>
> /* setup threads/dso/map/symbols also */
> machine = setup_fake_machine(&machines);
> - if (!machine)
> + if (IS_ERR(machine)) {
> + if (PTR_ERR(machine) == -EACCES)
> + err = TEST_SKIP;
> goto out;
> + }
>
> if (verbose > 1)
> machine__fprintf(machine, stderr);
> diff --git a/tools/perf/tests/hists_output.c b/tools/perf/tests/hists_output.c
> index 248beec..cc570cf 100644
> --- a/tools/perf/tests/hists_output.c
> +++ b/tools/perf/tests/hists_output.c
> @@ -597,13 +597,17 @@ int test__hists_output(int subtest __maybe_unused)
> err = parse_events(evlist, "cpu-clock", NULL);
> if (err)
> goto out;
> + err = TEST_FAIL;
>
> machines__init(&machines);
>
> /* setup threads/dso/map/symbols also */
> machine = setup_fake_machine(&machines);
> - if (!machine)
> + if (IS_ERR(machine)) {
> + if (PTR_ERR(machine) == -EACCES)
> + err = TEST_SKIP;
> goto out;
> + }
>
> if (verbose > 1)
> machine__fprintf(machine, stderr);
> --
> 1.8.3.4
>

2015-12-15 08:01:12

by Jiri Olsa

[permalink] [raw]

Subject: Re: [PATCH] perf tests: Fix false TEST_OK result for 'perf test hist'

On Tue, Dec 15, 2015 at 04:18:44AM +0000, Wang Nan wrote:
> Commit 71d6de64feddd4b455555326fba2111b3006d9e0 ('perf test: Fix hist
> testcases when kptr_restrict is on') solves a double free problem when
> 'perf test hist' calling setup_fake_machine(). However, the result is
> still incorrect. For example:
>
> $ ./perf test -v 'filtering hist entries'
> 25: Test filtering hist entries :
> --- start ---
> test child forked, pid 4186
> Cannot create kernel maps
> test child finished with 0
> ---- end ----
> Test filtering hist entries: Ok
>
> In this case the body of this test is not get executed at all, but the
> result is 'Ok'. I think 'Skip' should be better.
>
> The reason is that *ALL* hists test cases forget to reset err after
> using it to hold an error code.
>
> This patch ensure err is reset.
>
> In case when kptr_restrict prevent normal user get kernel address, this
> test should be skipped, not fail. This patch uses linux/err.h to store
> error code in return value of setup_fake_machine(), and let 'EACCES' to
> indicate this problem.
>
> Signed-off-by: Wang Nan <[email protected]>
> Cc: Arnaldo Carvalho de Melo <[email protected]>
> Cc: Jiri Olsa <[email protected]>
> Cc: Masami Hiramatsu <[email protected]>
> Cc: Namhyung Kim <[email protected]>

Acked-by: Jiri Olsa <[email protected]>

thanks,
jirka

2015-12-15 08:09:53

by Wang Nan

[permalink] [raw]

Subject: [PATCH v2] perf test: Fix false TEST_OK result for 'perf test hist'

2015-12-15 08:21:24

by Wang Nan

[permalink] [raw]

Subject: [PATCH v2] perf test: Reset err after using it hold errcode in hist testcases

All hists test cases forget to reset err after using it to hold an
error code. If error occure in setup_fake_machine() it incorrectly
return TEST_OK.

This patch fixes it.

Signed-off-by: Wang Nan <[email protected]>
Suggested-by: Namhyung Kim <[email protected]>
Cc: Arnaldo Carvalho de Melo <[email protected]>
Cc: Jiri Olsa <[email protected]>
Cc: Masami Hiramatsu <[email protected]>
Cc: Namhyung Kim <[email protected]>
---
tools/perf/tests/hists_cumulate.c | 1 +
tools/perf/tests/hists_filter.c | 1 +
tools/perf/tests/hists_link.c | 1 +
tools/perf/tests/hists_output.c | 1 +
4 files changed, 4 insertions(+)

diff --git a/tools/perf/tests/hists_cumulate.c b/tools/perf/tests/hists_cumulate.c
index 8292948..76f8922 100644
--- a/tools/perf/tests/hists_cumulate.c
+++ b/tools/perf/tests/hists_cumulate.c
@@ -706,6 +706,7 @@ int test__hists_cumulate(int subtest __maybe_unused)
err = parse_events(evlist, "cpu-clock", NULL);
if (err)
goto out;
+ err = TEST_FAIL;

machines__init(&machines);

diff --git a/tools/perf/tests/hists_filter.c b/tools/perf/tests/hists_filter.c
index ccb5b49..2cd91df 100644
--- a/tools/perf/tests/hists_filter.c
+++ b/tools/perf/tests/hists_filter.c
@@ -120,6 +120,7 @@ int test__hists_filter(int subtest __maybe_unused)
err = parse_events(evlist, "task-clock", NULL);
if (err)
goto out;
+ err = TEST_FAIL;

/* default sort order (comm,dso,sym) will be used */
if (setup_sorting() < 0)
diff --git a/tools/perf/tests/hists_link.c b/tools/perf/tests/hists_link.c
index 6243e2b..4dd8f4e 100644
--- a/tools/perf/tests/hists_link.c
+++ b/tools/perf/tests/hists_link.c
@@ -293,6 +293,7 @@ int test__hists_link(int subtest __maybe_unused)
if (err)
goto out;

+ err = TEST_FAIL;
/* default sort order (comm,dso,sym) will be used */
if (setup_sorting() < 0)
goto out;
diff --git a/tools/perf/tests/hists_output.c b/tools/perf/tests/hists_output.c
index 248beec..dc7bf56 100644
--- a/tools/perf/tests/hists_output.c
+++ b/tools/perf/tests/hists_output.c
@@ -597,6 +597,7 @@ int test__hists_output(int subtest __maybe_unused)
err = parse_events(evlist, "cpu-clock", NULL);
if (err)
goto out;
+ err = TEST_FAIL;

machines__init(&machines);

--
1.8.3.4

2015-12-15 09:04:15

by Namhyung Kim

[permalink] [raw]

Subject: Re: [PATCH v2] perf test: Fix false TEST_OK result for 'perf test hist'

On Tue, Dec 15, 2015 at 08:09:26AM +0000, Wang Nan wrote:
> Commit 71d6de64feddd4b455555326fba2111b3006d9e0 ('perf test: Fix hist
> testcases when kptr_restrict is on') solves a double free problem when
> 'perf test hist' calling setup_fake_machine(). However, the result is
> still incorrect. For example:
>
> $ ./perf test -v 'filtering hist entries'
> 25: Test filtering hist entries :
> --- start ---
> test child forked, pid 4186
> Cannot create kernel maps
> test child finished with 0
> ---- end ----
> Test filtering hist entries: Ok
>
> In this case the body of this test is not get executed at all, but the
> result is 'Ok'.
>
> Actually, in setup_fake_machine() there's no need to create real kernel
> maps. What we want is the fake maps. This patch removes the
> machine__create_kernel_maps() in setup_fake_machine(), so it won't be
> affected by kptr_restrict setting.
>
> Test result:
>
> $ cat /proc/sys/kernel/kptr_restrict
> 1
> $ ~/perf test -v hist
> 15: Test matching and linking multiple hists :
> --- start ---
> test child forked, pid 24031
> test child finished with 0
> ---- end ----
> Test matching and linking multiple hists: Ok
> [SNIP]
>
> Signed-off-by: Wang Nan <[email protected]>
> Suggested-by: Namhyung Kim <[email protected]>
> Cc: Arnaldo Carvalho de Melo <[email protected]>
> Cc: Jiri Olsa <[email protected]>
> Cc: Masami Hiramatsu <[email protected]>

Acked-by: Namhyung Kim <[email protected]>

Thanks,
Namhyung

> ---
> tools/perf/tests/hists_common.c | 5 -----
> 1 file changed, 5 deletions(-)
>
> diff --git a/tools/perf/tests/hists_common.c b/tools/perf/tests/hists_common.c
> index bcfd081..071a8b5 100644
> --- a/tools/perf/tests/hists_common.c
> +++ b/tools/perf/tests/hists_common.c
> @@ -87,11 +87,6 @@ struct machine *setup_fake_machine(struct machines *machines)
> return NULL;
> }
>
> - if (machine__create_kernel_maps(machine)) {
> - pr_debug("Cannot create kernel maps\n");
> - return NULL;
> - }
> -
> for (i = 0; i < ARRAY_SIZE(fake_threads); i++) {
> struct thread *thread;
>
> --
> 1.8.3.4
>

2015-12-15 09:05:32

by Namhyung Kim

[permalink] [raw]

Subject: Re: [PATCH v2] perf test: Reset err after using it hold errcode in hist testcases

On Tue, Dec 15, 2015 at 08:20:50AM +0000, Wang Nan wrote:
> All hists test cases forget to reset err after using it to hold an
> error code. If error occure in setup_fake_machine() it incorrectly
> return TEST_OK.
>
> This patch fixes it.
>
> Signed-off-by: Wang Nan <[email protected]>
> Suggested-by: Namhyung Kim <[email protected]>
> Cc: Arnaldo Carvalho de Melo <[email protected]>
> Cc: Jiri Olsa <[email protected]>
> Cc: Masami Hiramatsu <[email protected]>

Acked-by: Namhyung Kim <[email protected]>

Thanks,
Namhyung

> ---
> tools/perf/tests/hists_cumulate.c | 1 +
> tools/perf/tests/hists_filter.c | 1 +
> tools/perf/tests/hists_link.c | 1 +
> tools/perf/tests/hists_output.c | 1 +
> 4 files changed, 4 insertions(+)
>
> diff --git a/tools/perf/tests/hists_cumulate.c b/tools/perf/tests/hists_cumulate.c
> index 8292948..76f8922 100644
> --- a/tools/perf/tests/hists_cumulate.c
> +++ b/tools/perf/tests/hists_cumulate.c
> @@ -706,6 +706,7 @@ int test__hists_cumulate(int subtest __maybe_unused)
> err = parse_events(evlist, "cpu-clock", NULL);
> if (err)
> goto out;
> + err = TEST_FAIL;
>
> machines__init(&machines);
>
> diff --git a/tools/perf/tests/hists_filter.c b/tools/perf/tests/hists_filter.c
> index ccb5b49..2cd91df 100644
> --- a/tools/perf/tests/hists_filter.c
> +++ b/tools/perf/tests/hists_filter.c
> @@ -120,6 +120,7 @@ int test__hists_filter(int subtest __maybe_unused)
> err = parse_events(evlist, "task-clock", NULL);
> if (err)
> goto out;
> + err = TEST_FAIL;
>
> /* default sort order (comm,dso,sym) will be used */
> if (setup_sorting() < 0)
> diff --git a/tools/perf/tests/hists_link.c b/tools/perf/tests/hists_link.c
> index 6243e2b..4dd8f4e 100644
> --- a/tools/perf/tests/hists_link.c
> +++ b/tools/perf/tests/hists_link.c
> @@ -293,6 +293,7 @@ int test__hists_link(int subtest __maybe_unused)
> if (err)
> goto out;
>
> + err = TEST_FAIL;
> /* default sort order (comm,dso,sym) will be used */
> if (setup_sorting() < 0)
> goto out;
> diff --git a/tools/perf/tests/hists_output.c b/tools/perf/tests/hists_output.c
> index 248beec..dc7bf56 100644
> --- a/tools/perf/tests/hists_output.c
> +++ b/tools/perf/tests/hists_output.c
> @@ -597,6 +597,7 @@ int test__hists_output(int subtest __maybe_unused)
> err = parse_events(evlist, "cpu-clock", NULL);
> if (err)
> goto out;
> + err = TEST_FAIL;
>
> machines__init(&machines);
>
> --
> 1.8.3.4
>