[PATCH] perf bench: fix assert when NDEBUG is defined

[PATCH] perf bench: fix assert when NDEBUG is defined

[Irina Tirdea]

From: Irina Tirdea <irina.tirdea@intel.com>

When NDEBUG is defined, the assert macro will be expanded to nothing.
Some assert calls used in perf are also including some functionality
(e.g. system calls), not only validity checks. Therefore, if NDEBUG is
defined, these functionality will be removed along with the assert.

The functionality of the program needs to be separated from the assert checks.
In perf, BUG_ON is also defined on assert, so we need to fix these statements
too.

Signed-off-by: Irina Tirdea <irina.tirdea@intel.com>
---
 tools/perf/bench/mem-memcpy.c |    8 +++++---
 tools/perf/bench/mem-memset.c |    8 +++++---
 tools/perf/bench/sched-pipe.c |    6 ++++--
 3 files changed, 14 insertions(+), 8 deletions(-)

diff --git a/tools/perf/bench/mem-memcpy.c b/tools/perf/bench/mem-memcpy.c
index 02dad5d..bccb783 100644
--- a/tools/perf/bench/mem-memcpy.c
+++ b/tools/perf/bench/mem-memcpy.c
@@ -144,17 +144,19 @@ static double do_memcpy_gettimeofday(memcpy_t
fn, size_t len, bool prefault)
 {
 	struct timeval tv_start, tv_end, tv_diff;
 	void *src = NULL, *dst = NULL;
-	int i;
+	int i, ret;

 	alloc_mem(&src, &dst, len);

 	if (prefault)
 		fn(dst, src, len);

-	BUG_ON(gettimeofday(&tv_start, NULL));
+	ret = gettimeofday(&tv_start, NULL);
+	BUG_ON(ret);
 	for (i = 0; i < iterations; ++i)
 		fn(dst, src, len);
-	BUG_ON(gettimeofday(&tv_end, NULL));
+	ret = gettimeofday(&tv_end, NULL);
+	BUG_ON(ret);

 	timersub(&tv_end, &tv_start, &tv_diff);

diff --git a/tools/perf/bench/mem-memset.c b/tools/perf/bench/mem-memset.c
index 350cc95..e0702d2 100644
--- a/tools/perf/bench/mem-memset.c
+++ b/tools/perf/bench/mem-memset.c
@@ -139,17 +139,19 @@ static double do_memset_gettimeofday(memset_t
fn, size_t len, bool prefault)
 {
 	struct timeval tv_start, tv_end, tv_diff;
 	void *dst = NULL;
-	int i;
+	int i, ret;

 	alloc_mem(&dst, len);

 	if (prefault)
 		fn(dst, -1, len);

-	BUG_ON(gettimeofday(&tv_start, NULL));
+	ret = gettimeofday(&tv_start, NULL);
+	BUG_ON(ret);
 	for (i = 0; i < iterations; ++i)
 		fn(dst, i, len);
-	BUG_ON(gettimeofday(&tv_end, NULL));
+	ret = gettimeofday(&tv_end, NULL);
+	BUG_ON(ret);

 	timersub(&tv_end, &tv_start, &tv_diff);

diff --git a/tools/perf/bench/sched-pipe.c b/tools/perf/bench/sched-pipe.c
index 0c7454f..b35c94b 100644
--- a/tools/perf/bench/sched-pipe.c
+++ b/tools/perf/bench/sched-pipe.c
@@ -61,8 +61,10 @@ int bench_sched_pipe(int argc, const char **argv,
 	argc = parse_options(argc, argv, options,
 			     bench_sched_pipe_usage, 0);

-	assert(!pipe(pipe_1));
-	assert(!pipe(pipe_2));
+	ret = pipe(pipe_1);
+	assert(!ret);
+	ret = !pipe(pipe_2);
+	assert(!ret);

 	pid = fork();
 	assert(pid >= 0);
-- 
1.7.9.5

Re: [PATCH] perf bench: fix assert when NDEBUG is defined

[Namhyung Kim]

On Mon, 3 Sep 2012 03:04:32 +0300, Irina Tirdea wrote:
> From: Irina Tirdea <irina.tirdea@intel.com>
>
> When NDEBUG is defined, the assert macro will be expanded to nothing.
> Some assert calls used in perf are also including some functionality
> (e.g. system calls), not only validity checks. Therefore, if NDEBUG is
> defined, these functionality will be removed along with the assert.
>
> The functionality of the program needs to be separated from the assert checks.
> In perf, BUG_ON is also defined on assert, so we need to fix these statements
> too.
>
> Signed-off-by: Irina Tirdea <irina.tirdea@intel.com>
> ---
>  tools/perf/bench/mem-memcpy.c |    8 +++++---
>  tools/perf/bench/mem-memset.c |    8 +++++---
>  tools/perf/bench/sched-pipe.c |    6 ++++--
>  3 files changed, 14 insertions(+), 8 deletions(-)
>
> diff --git a/tools/perf/bench/mem-memcpy.c b/tools/perf/bench/mem-memcpy.c
> index 02dad5d..bccb783 100644
> --- a/tools/perf/bench/mem-memcpy.c
> +++ b/tools/perf/bench/mem-memcpy.c
> @@ -144,17 +144,19 @@ static double do_memcpy_gettimeofday(memcpy_t
> fn, size_t len, bool prefault)
>  {
>  	struct timeval tv_start, tv_end, tv_diff;
>  	void *src = NULL, *dst = NULL;
> -	int i;
> +	int i, ret;
>
>  	alloc_mem(&src, &dst, len);
>
>  	if (prefault)
>  		fn(dst, src, len);
>
> -	BUG_ON(gettimeofday(&tv_start, NULL));
> +	ret = gettimeofday(&tv_start, NULL);
> +	BUG_ON(ret);

I think one of good thing of assert is that it outputs the exact failure
condition when it fails.  So with patch, it will convert

  Assertion `gettimeofday(&tv_start, NULL)' failed.

into

  Assertion `ret' failed.

which is not so informative.

So I'd rather suggest using more descriptive names like ret_gtod ?


>  	for (i = 0; i < iterations; ++i)
>  		fn(dst, src, len);
> -	BUG_ON(gettimeofday(&tv_end, NULL));
> +	ret = gettimeofday(&tv_end, NULL);
> +	BUG_ON(ret);
>
>  	timersub(&tv_end, &tv_start, &tv_diff);
>
> diff --git a/tools/perf/bench/mem-memset.c b/tools/perf/bench/mem-memset.c
> index 350cc95..e0702d2 100644
> --- a/tools/perf/bench/mem-memset.c
> +++ b/tools/perf/bench/mem-memset.c
> @@ -139,17 +139,19 @@ static double do_memset_gettimeofday(memset_t
> fn, size_t len, bool prefault)
>  {
>  	struct timeval tv_start, tv_end, tv_diff;
>  	void *dst = NULL;
> -	int i;
> +	int i, ret;
>
>  	alloc_mem(&dst, len);
>
>  	if (prefault)
>  		fn(dst, -1, len);
>
> -	BUG_ON(gettimeofday(&tv_start, NULL));
> +	ret = gettimeofday(&tv_start, NULL);
> +	BUG_ON(ret);
>  	for (i = 0; i < iterations; ++i)
>  		fn(dst, i, len);
> -	BUG_ON(gettimeofday(&tv_end, NULL));
> +	ret = gettimeofday(&tv_end, NULL);
> +	BUG_ON(ret);

Ditto.


>
>  	timersub(&tv_end, &tv_start, &tv_diff);
>
> diff --git a/tools/perf/bench/sched-pipe.c b/tools/perf/bench/sched-pipe.c
> index 0c7454f..b35c94b 100644
> --- a/tools/perf/bench/sched-pipe.c
> +++ b/tools/perf/bench/sched-pipe.c
> @@ -61,8 +61,10 @@ int bench_sched_pipe(int argc, const char **argv,
>  	argc = parse_options(argc, argv, options,
>  			     bench_sched_pipe_usage, 0);
>
> -	assert(!pipe(pipe_1));
> -	assert(!pipe(pipe_2));
> +	ret = pipe(pipe_1);
> +	assert(!ret);
> +	ret = !pipe(pipe_2);
> +	assert(!ret);

What about converting these raw assert's to BUG_ONs (with negating the
conditions) for consistency?

Thanks,
Namhyung


>
>  	pid = fork();
>  	assert(pid >= 0);

Re: [PATCH] perf bench: fix assert when NDEBUG is defined

[Pekka Enberg]

On Mon, Sep 3, 2012 at 4:45 AM, Namhyung Kim <namhyung@kernel.org> wrote:
> On Mon, 3 Sep 2012 03:04:32 +0300, Irina Tirdea wrote:
>> From: Irina Tirdea <irina.tirdea@intel.com>
>>
>> When NDEBUG is defined, the assert macro will be expanded to nothing.
>> Some assert calls used in perf are also including some functionality
>> (e.g. system calls), not only validity checks. Therefore, if NDEBUG is
>> defined, these functionality will be removed along with the assert.
>>
>> The functionality of the program needs to be separated from the assert checks.
>> In perf, BUG_ON is also defined on assert, so we need to fix these statements
>> too.
>>
>> Signed-off-by: Irina Tirdea <irina.tirdea@intel.com>
>> ---
>>  tools/perf/bench/mem-memcpy.c |    8 +++++---
>>  tools/perf/bench/mem-memset.c |    8 +++++---
>>  tools/perf/bench/sched-pipe.c |    6 ++++--
>>  3 files changed, 14 insertions(+), 8 deletions(-)
>>
>> diff --git a/tools/perf/bench/mem-memcpy.c b/tools/perf/bench/mem-memcpy.c
>> index 02dad5d..bccb783 100644
>> --- a/tools/perf/bench/mem-memcpy.c
>> +++ b/tools/perf/bench/mem-memcpy.c
>> @@ -144,17 +144,19 @@ static double do_memcpy_gettimeofday(memcpy_t
>> fn, size_t len, bool prefault)
>>  {
>>       struct timeval tv_start, tv_end, tv_diff;
>>       void *src = NULL, *dst = NULL;
>> -     int i;
>> +     int i, ret;
>>
>>       alloc_mem(&src, &dst, len);
>>
>>       if (prefault)
>>               fn(dst, src, len);
>>
>> -     BUG_ON(gettimeofday(&tv_start, NULL));
>> +     ret = gettimeofday(&tv_start, NULL);
>> +     BUG_ON(ret);
>
> I think one of good thing of assert is that it outputs the exact failure
> condition when it fails.  So with patch, it will convert
>
>   Assertion `gettimeofday(&tv_start, NULL)' failed.
>
> into
>
>   Assertion `ret' failed.
>
> which is not so informative.
>
> So I'd rather suggest using more descriptive names like ret_gtod ?

No, please don't do that. That'll make the code ugly and it's really
just papering over the fact that the assertions should be converted to
proper error handling.

Re: [PATCH] perf bench: fix assert when NDEBUG is defined

[Peter Zijlstra]

On Mon, 2012-09-03 at 03:04 +0300, Irina Tirdea wrote:
> -       BUG_ON(gettimeofday(&tv_start, NULL));
> +       ret = gettimeofday(&tv_start, NULL);
> +       BUG_ON(ret); 

Its valid (although admittedly dubious) to have BUG_ON with
side-effects.

The 'right' fix would be something like:

---
 tools/perf/util/include/linux/kernel.h |    4 ++++
 1 file changed, 4 insertions(+)

diff --git a/tools/perf/util/include/linux/kernel.h b/tools/perf/util/include/linux/kernel.h
index b6842c1..a5efd924 100644
--- a/tools/perf/util/include/linux/kernel.h
+++ b/tools/perf/util/include/linux/kernel.h
@@ -47,8 +47,12 @@
 #endif
 
 #ifndef BUG_ON
+#ifdef NDEBUG
+#define BUG_ON(cond) do { if (cond) ; } while (0)
+#else
 #define BUG_ON(cond) assert(!(cond))
 #endif
+#endif
 
 /*
  * Both need more care to handle endianness

Re: [PATCH] perf bench: fix assert when NDEBUG is defined

[Irina Tirdea]

> Its valid (although admittedly dubious) to have BUG_ON with
> side-effects.
>
> The 'right' fix would be something like:
>
> ---
>  #ifndef BUG_ON
> +#ifdef NDEBUG
> +#define BUG_ON(cond) do { if (cond) ; } while (0)
> +#else
>  #define BUG_ON(cond) assert(!(cond))
>  #endif
> +#endif
>

This is indeed the right fix. I will use this approach, test it and resubmit.

Thanks,
Irina