[igt-dev] [PATCH i-g-t 3/7] lib: Stop using assert() for runtime checks
Petri Latvala
petri.latvala at intel.com
Wed Apr 17 08:52:16 UTC 2019
On Tue, Apr 16, 2019 at 04:10:57PM -0400, Lyude wrote:
> From: Lyude Paul <lyude at 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 at 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
More information about the igt-dev
mailing list