Re: [igt-dev] [PATCH i-g-t 3/7] lib: Stop using assert() for runtime checks

[Petri Latvala <petri.latvala@intel.com>]

On Tue, Apr 16, 2019 at 04:10:57PM -0400, Lyude wrote:
> From: Lyude Paul <lyude@redhat.com>
> 
> In the process of trying to get an up to date version of igt packaged
> for Fedora I discovered that if igt was built with -Db_ndebug=true, a
> significant portion of the test infrastructure unit tests would start
> failing:
> 
>   1/265 lib: igt_assert                         OK       0.11 s
>   2/265 lib: igt_can_fail                       OK       0.08 s
>   3/265 lib: igt_can_fail_simple                OK       0.08 s
>   4/265 lib: igt_exit_handler                   OK       0.05 s
>   5/265 lib: igt_fork                           FAIL     0.05 s (killed by signal 9 SIGKILL)
>   6/265 lib: igt_fork_helper                    OK       0.42 s
>   7/265 lib: igt_hdmi_inject                    OK       0.05 s
>   8/265 lib: igt_list_only                      OK       0.01 s
>   9/265 lib: igt_invalid_subtest_name           OK       0.05 s
>  10/265 lib: igt_no_exit                        OK       0.04 s
>  11/265 lib: igt_segfault                       OK       0.38 s
>  12/265 lib: igt_simulation                     OK       0.02 s
>  13/265 lib: igt_stats                          OK       0.04 s
>  14/265 lib: igt_subtest_group                  OK       0.03 s
>  15/265 lib: igt_no_subtest                     SKIP     0.02 s
>  16/265 lib: igt_simple_test_subtests           UNEXPECTEDPASS 0.02 s
>  17/265 lib: igt_timeout                        EXPECTEDFAIL 1.02 s
> 
> Which appeared to stem from the fact that -Db_ndebug=true would strip
> assert() calls. While on a first glance of lib/tests/igt_tests_common.h
> one would assume that the only user of assert() was the test
> infrastructure unit tests themselves, it turns out we've actually been
> using this in multiple spots that seem to expect an unconditional
> runtime check.


Umm, yes. You've uncovered a bug, but not what you think. With meson,
we don't include lib/check-ndebug.h anymore, only with autotools so
you missed the important part: Do not attempt to build IGT with
b_ndebug=true.


> So in order to fix this, we introduce igt_internal_assert(). The purpose
> of this function is to provide simple runtime error-checking for
> conditions that warrant abort()ing IGT under any circumstances, and to
> provide a internal_assert() replacement for our test infrastructure
> tests to use. Then, remove all of the assert() usages in lib/tests/*
> along with any of the assert() calls in lib/igt_core.c that affect test
> behavior.

In a nutshell: You wanted to build without asserts, so you removed all
the asserts from the code, replacing them with the same code? Why?

However, it's the thought that matters, and this is slightly going off
on a tangent. Those uses of assert in lib/ are for places where

1) something is fatally wrong and we need to drop everything and stop
   executing

2) cannot use igt_assert for it. That's for places where we can say
   "you tried testing your kernel but it has a bug". The lib/ asserts
   are for "IGT has a bug", or in a couple of cases, "your IGT setup
   has a bug".

Chris has been tossing around mentions of having a clear and usable
igt_is_broken_not_your_kernel__assert() and this might be a good spot
to introduce such a thing. Plain assert() is fine to achieve that
outcome, but igt.cocci (albeit a bit dusty) nukes those from tests/. A
'CRASH' test result is a fine outcome when it's triggered and conveys
the message across, we just need a clear looking igt API for it.




> 
> Signed-off-by: Lyude Paul <lyude@redhat.com>
> ---
>  lib/igt_core.c                       | 24 +++++------
>  lib/igt_core.h                       | 18 +++++++++
>  lib/tests/igt_assert.c               |  2 +-
>  lib/tests/igt_can_fail.c             | 10 ++---
>  lib/tests/igt_can_fail_simple.c      |  2 +-
>  lib/tests/igt_exit_handler.c         | 18 ++++-----
>  lib/tests/igt_fork.c                 |  4 +-
>  lib/tests/igt_invalid_subtest_name.c |  2 +-
>  lib/tests/igt_no_exit.c              |  2 +-
>  lib/tests/igt_segfault.c             |  2 +-
>  lib/tests/igt_simulation.c           | 60 ++++++++++++++--------------
>  lib/tests/igt_subtest_group.c        | 16 ++++----
>  lib/tests/igt_tests_common.h         | 19 ++++-----
>  13 files changed, 97 insertions(+), 82 deletions(-)
> 
> diff --git a/lib/igt_core.c b/lib/igt_core.c
> index ae03e909..a637b94e 100644
> --- a/lib/igt_core.c
> +++ b/lib/igt_core.c
> @@ -521,7 +521,7 @@ static void common_exit_handler(int sig)
>  
>  	/* When not killed by a signal check that igt_exit() has been properly
>  	 * called. */
> -	assert(sig != 0 || igt_exit_called);
> +	igt_internal_assert(sig != 0 || igt_exit_called);
>  }
>  
>  static void print_test_description(void)
> @@ -611,7 +611,7 @@ static void common_init_config(void)
>  
>  	ret = g_key_file_get_integer(igt_key_file, "DUT", "SuspendResumeDelay",
>  				     &error);
> -	assert(!error || error->code != G_KEY_FILE_ERROR_INVALID_VALUE);
> +	igt_internal_assert(!error || error->code != G_KEY_FILE_ERROR_INVALID_VALUE);
>  
>  	g_clear_error(&error);
>  
> @@ -903,9 +903,9 @@ bool __igt_run_subtest(const char *subtest_name)
>  {
>  	int i;
>  
> -	assert(!in_subtest);
> -	assert(!in_fixture);
> -	assert(test_with_subtests);
> +	igt_internal_assert(!in_subtest);
> +	igt_internal_assert(!in_fixture);
> +	igt_internal_assert(test_with_subtests);
>  
>  	/* check the subtest name only contains a-z, A-Z, 0-9, '-' and '_' */
>  	for (i = 0; subtest_name[i] != '\0'; i++)
> @@ -978,7 +978,7 @@ bool igt_only_list_subtests(void)
>  
>  void __igt_subtest_group_save(int *save)
>  {
> -	assert(test_with_subtests);
> +	igt_internal_assert(test_with_subtests);
>  
>  	*save = skip_subtests_henceforth;
>  }
> @@ -1032,7 +1032,7 @@ void igt_skip(const char *f, ...)
>  	va_list args;
>  	skipped_one = true;
>  
> -	assert(!test_child);
> +	igt_internal_assert(!test_child);
>  
>  	if (!igt_only_list_subtests()) {
>  		va_start(args, f);
> @@ -1510,10 +1510,10 @@ void igt_exit(void)
>  		exit(IGT_EXIT_SUCCESS);
>  
>  	/* Calling this without calling one of the above is a failure */
> -	assert(!test_with_subtests ||
> -	       skipped_one ||
> -	       succeeded_one ||
> -	       failed_one);
> +	igt_internal_assert(!test_with_subtests ||
> +			    skipped_one ||
> +			    succeeded_one ||
> +			    failed_one);
>  
>  	if (test_with_subtests && !failed_one) {
>  		if (succeeded_one)
> @@ -1531,7 +1531,7 @@ void igt_exit(void)
>  		kill(test_children[c], SIGKILL);
>  	assert(!num_test_children);
>  
> -	assert(waitpid(-1, &tmp, WNOHANG) == -1 && errno == ECHILD);
> +	igt_internal_assert(waitpid(-1, &tmp, WNOHANG) == -1 && errno == ECHILD);
>  
>  	if (!test_with_subtests) {
>  		struct timespec now;
> diff --git a/lib/igt_core.h b/lib/igt_core.h
> index 47ffd9e7..eed39bbf 100644
> --- a/lib/igt_core.h
> +++ b/lib/igt_core.h
> @@ -747,6 +747,24 @@ static inline void igt_ignore_warn(bool value)
>  	else igt_debug("Test requirement passed: !(%s)\n", #expr); \
>  } while (0)
>  
> +/**
> + * igt_internal_assert:
> + * @expr: condition to test
> + *
> + * If the given expression fails, print an error to stderr and abort()
> + * immediately. This is intended for use only by the igt helpers in order to
> + * check for incorrect usages of the helpers or other extraordinary conditions.
> + *
> + * This should never be used within a normal test. Doing so will alert
> + * velociraptors to your location.
> + */


Aside: I like this documentation block.



-- 
Petri Latvala
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev