[igt-dev] [PATCH i-g-t v2 3/8] lib: Stop using assert() for runtime checks
Daniel Vetter
daniel at ffwll.ch
Wed Apr 17 11:40:18 UTC 2019
On Tue, Apr 16, 2019 at 07:25:43PM -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.
>
> 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.
>
> 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.
> + */
> +#define igt_internal_assert(expr) do { \
> + if (!(expr)) { \
> + igt_critical("Internal assertion failed: `%s`\n", #expr); \
> + abort(); \
I think this should _exit(), to avoid hilarity with our exit handlers
(which are abort handlers too).
> + } \
> +} while (0)
Hm, kinda awkward if we put this into a header tests have to use. I'd like
to have an easy check that nothing in tests/ uses plain assert(). Maybe
add a new igt_internal.h. Up to you I'd say.
Aside from these nits lgtm, with them addressed somehow:
Reviewed-by: Daniel Vetter <daniel.vetter at ffwll.ch>
> +
> /* fork support code */
> bool __igt_fork(void);
>
> diff --git a/lib/tests/igt_assert.c b/lib/tests/igt_assert.c
> index 1caf5d88..a8ca1a31 100644
> --- a/lib/tests/igt_assert.c
> +++ b/lib/tests/igt_assert.c
> @@ -62,7 +62,7 @@ static int do_fork(void)
>
> switch (pid = fork()) {
> case -1:
> - internal_assert(0);
> + igt_internal_assert(0);
> case 0:
> argc = ARRAY_SIZE(argv_run);
> igt_simple_init(argc, argv_run);
> diff --git a/lib/tests/igt_can_fail.c b/lib/tests/igt_can_fail.c
> index 1e3d9558..ae546644 100644
> --- a/lib/tests/igt_can_fail.c
> +++ b/lib/tests/igt_can_fail.c
> @@ -28,17 +28,17 @@
>
> igt_main
> {
> - internal_assert(igt_can_fail() == false);
> + igt_internal_assert(igt_can_fail() == false);
>
> igt_fixture {
> - internal_assert(igt_can_fail());
> + igt_internal_assert(igt_can_fail());
> }
>
> - internal_assert(igt_can_fail() == false);
> + igt_internal_assert(igt_can_fail() == false);
>
> igt_subtest("subtest") {
> - internal_assert(igt_can_fail());
> + igt_internal_assert(igt_can_fail());
> }
>
> - internal_assert(igt_can_fail() == false);
> + igt_internal_assert(igt_can_fail() == false);
> }
> diff --git a/lib/tests/igt_can_fail_simple.c b/lib/tests/igt_can_fail_simple.c
> index 8ff43d15..4f2e4a78 100644
> --- a/lib/tests/igt_can_fail_simple.c
> +++ b/lib/tests/igt_can_fail_simple.c
> @@ -28,5 +28,5 @@
>
> igt_simple_main
> {
> - internal_assert(igt_can_fail());
> + igt_internal_assert(igt_can_fail());
> }
> diff --git a/lib/tests/igt_exit_handler.c b/lib/tests/igt_exit_handler.c
> index 892a7f14..220c9085 100644
> --- a/lib/tests/igt_exit_handler.c
> +++ b/lib/tests/igt_exit_handler.c
> @@ -35,7 +35,7 @@ int pipes[2];
>
> static void exit_handler1(int sig)
> {
> - internal_assert(test == 1);
> + igt_internal_assert(test == 1);
> test++;
> }
>
> @@ -44,12 +44,12 @@ static void exit_handler2(int sig)
> char tmp = 1;
>
> /* ensure exit handlers are called in reverse */
> - internal_assert(test == 0);
> + igt_internal_assert(test == 0);
> test++;
>
> /* we need to get a side effect to the parent to make sure exit handlers
> * actually run. */
> - internal_assert(write(pipes[1], &tmp, 1) == 1);
> + igt_internal_assert(write(pipes[1], &tmp, 1) == 1);
> }
>
> enum test_type {
> @@ -69,7 +69,7 @@ static int testfunc(enum test_type test_type)
> int status;
> char tmp = 0;
>
> - internal_assert(pipe2(pipes, O_NONBLOCK) == 0);
> + igt_internal_assert(pipe2(pipes, O_NONBLOCK) == 0);
>
> pid = fork();
>
> @@ -102,10 +102,10 @@ static int testfunc(enum test_type test_type)
> igt_exit();
> }
>
> - internal_assert(waitpid(pid, &status, 0) != -1);
> + igt_internal_assert(waitpid(pid, &status, 0) != -1);
>
> - internal_assert(read(pipes[0], &tmp, 1) == 1);
> - internal_assert(tmp == 1);
> + igt_internal_assert(read(pipes[0], &tmp, 1) == 1);
> + igt_internal_assert(tmp == 1);
>
> return status;
> }
> @@ -114,9 +114,9 @@ int main(int argc, char **argv)
> {
> int status;
>
> - internal_assert(testfunc(SUC) == 0);
> + igt_internal_assert(testfunc(SUC) == 0);
>
> - internal_assert(testfunc(NORMAL) == 0);
> + igt_internal_assert(testfunc(NORMAL) == 0);
>
> status = testfunc(FAIL);
> internal_assert_wexited(status, 1);
> diff --git a/lib/tests/igt_fork.c b/lib/tests/igt_fork.c
> index 7e8b4f9b..b8e437e0 100644
> --- a/lib/tests/igt_fork.c
> +++ b/lib/tests/igt_fork.c
> @@ -68,7 +68,7 @@ static void plain_fork_leak(void)
>
> switch (pid = fork()) {
> case -1:
> - internal_assert(0);
> + igt_internal_assert(0);
> case 0:
> sleep(1);
> default:
> @@ -92,7 +92,7 @@ static int do_fork(void (*test_to_run)(void))
>
> switch (pid = fork()) {
> case -1:
> - internal_assert(0);
> + igt_internal_assert(0);
> case 0:
> argc = ARRAY_SIZE(argv_run);
> igt_simple_init(argc, argv_run);
> diff --git a/lib/tests/igt_invalid_subtest_name.c b/lib/tests/igt_invalid_subtest_name.c
> index 92e767ab..696687fc 100644
> --- a/lib/tests/igt_invalid_subtest_name.c
> +++ b/lib/tests/igt_invalid_subtest_name.c
> @@ -66,7 +66,7 @@ static int do_fork(void (*test_to_run)(void))
>
> switch (pid = fork()) {
> case -1:
> - internal_assert(0);
> + igt_internal_assert(0);
> case 0:
> test_to_run();
> default:
> diff --git a/lib/tests/igt_no_exit.c b/lib/tests/igt_no_exit.c
> index 82f00b52..3eeef5a4 100644
> --- a/lib/tests/igt_no_exit.c
> +++ b/lib/tests/igt_no_exit.c
> @@ -62,7 +62,7 @@ static int do_fork(void (*test_to_run)(void))
>
> switch (pid = fork()) {
> case -1:
> - internal_assert(0);
> + igt_internal_assert(0);
> case 0:
> test_to_run();
> default:
> diff --git a/lib/tests/igt_segfault.c b/lib/tests/igt_segfault.c
> index 0d872f67..b4c5bcbd 100644
> --- a/lib/tests/igt_segfault.c
> +++ b/lib/tests/igt_segfault.c
> @@ -63,7 +63,7 @@ static int do_fork(void)
>
> switch (pid = fork()) {
> case -1:
> - internal_assert(0);
> + igt_internal_assert(0);
> case 0:
> argc = ARRAY_SIZE(argv_run);
> if (simple) {
> diff --git a/lib/tests/igt_simulation.c b/lib/tests/igt_simulation.c
> index 3f3cd88f..899665d3 100644
> --- a/lib/tests/igt_simulation.c
> +++ b/lib/tests/igt_simulation.c
> @@ -52,7 +52,7 @@ static int do_fork(void)
>
> switch (pid = fork()) {
> case -1:
> - internal_assert(0);
> + igt_internal_assert(0);
> case 0:
> if (simple) {
> argc = 1;
> @@ -90,7 +90,7 @@ static int do_fork(void)
> errno == EINTR)
> ;
>
> - internal_assert(WIFEXITED(status));
> + igt_internal_assert(WIFEXITED(status));
>
> return status;
> }
> @@ -100,40 +100,40 @@ int main(int argc, char **argv)
> {
> /* simple tests */
> simple = true;
> - internal_assert(setenv("INTEL_SIMULATION", "1", 1) == 0);
> - internal_assert(WEXITSTATUS(do_fork()) == IGT_EXIT_SKIP);
> + igt_internal_assert(setenv("INTEL_SIMULATION", "1", 1) == 0);
> + igt_internal_assert(WEXITSTATUS(do_fork()) == IGT_EXIT_SKIP);
>
> - internal_assert(setenv("INTEL_SIMULATION", "0", 1) == 0);
> - internal_assert(WEXITSTATUS(do_fork()) == IGT_EXIT_SUCCESS);
> + igt_internal_assert(setenv("INTEL_SIMULATION", "0", 1) == 0);
> + igt_internal_assert(WEXITSTATUS(do_fork()) == IGT_EXIT_SUCCESS);
>
> /* subtests, list mode */
> simple = false;
> list_subtests = true;
>
> in_fixture = false;
> - internal_assert(setenv("INTEL_SIMULATION", "1", 1) == 0);
> - internal_assert(WEXITSTATUS(do_fork()) == IGT_EXIT_SUCCESS);
> + igt_internal_assert(setenv("INTEL_SIMULATION", "1", 1) == 0);
> + igt_internal_assert(WEXITSTATUS(do_fork()) == IGT_EXIT_SUCCESS);
>
> - internal_assert(setenv("INTEL_SIMULATION", "0", 1) == 0);
> - internal_assert(WEXITSTATUS(do_fork()) == IGT_EXIT_SUCCESS);
> + igt_internal_assert(setenv("INTEL_SIMULATION", "0", 1) == 0);
> + igt_internal_assert(WEXITSTATUS(do_fork()) == IGT_EXIT_SUCCESS);
>
> in_fixture = true;
> - internal_assert(setenv("INTEL_SIMULATION", "1", 1) == 0);
> - internal_assert(WEXITSTATUS(do_fork()) == IGT_EXIT_SUCCESS);
> + igt_internal_assert(setenv("INTEL_SIMULATION", "1", 1) == 0);
> + igt_internal_assert(WEXITSTATUS(do_fork()) == IGT_EXIT_SUCCESS);
>
>
> - internal_assert(setenv("INTEL_SIMULATION", "0", 1) == 0);
> - internal_assert(WEXITSTATUS(do_fork()) == IGT_EXIT_SUCCESS);
> + igt_internal_assert(setenv("INTEL_SIMULATION", "0", 1) == 0);
> + igt_internal_assert(WEXITSTATUS(do_fork()) == IGT_EXIT_SUCCESS);
>
>
> in_fixture = false;
> in_subtest = true;
> - internal_assert(setenv("INTEL_SIMULATION", "1", 1) == 0);
> - internal_assert(WEXITSTATUS(do_fork()) == IGT_EXIT_SUCCESS);
> + igt_internal_assert(setenv("INTEL_SIMULATION", "1", 1) == 0);
> + igt_internal_assert(WEXITSTATUS(do_fork()) == IGT_EXIT_SUCCESS);
>
>
> - internal_assert(setenv("INTEL_SIMULATION", "0", 1) == 0);
> - internal_assert(WEXITSTATUS(do_fork()) == IGT_EXIT_SUCCESS);
> + igt_internal_assert(setenv("INTEL_SIMULATION", "0", 1) == 0);
> + igt_internal_assert(WEXITSTATUS(do_fork()) == IGT_EXIT_SUCCESS);
>
>
> /* subtest, run mode */
> @@ -141,30 +141,30 @@ int main(int argc, char **argv)
> list_subtests = false;
>
> in_fixture = false;
> - internal_assert(setenv("INTEL_SIMULATION", "1", 1) == 0);
> - internal_assert(WEXITSTATUS(do_fork()) == IGT_EXIT_SKIP);
> + igt_internal_assert(setenv("INTEL_SIMULATION", "1", 1) == 0);
> + igt_internal_assert(WEXITSTATUS(do_fork()) == IGT_EXIT_SKIP);
>
> - internal_assert(setenv("INTEL_SIMULATION", "0", 1) == 0);
> - internal_assert(WEXITSTATUS(do_fork()) == IGT_EXIT_SUCCESS);
> + igt_internal_assert(setenv("INTEL_SIMULATION", "0", 1) == 0);
> + igt_internal_assert(WEXITSTATUS(do_fork()) == IGT_EXIT_SUCCESS);
>
>
> in_fixture = true;
> - internal_assert(setenv("INTEL_SIMULATION", "1", 1) == 0);
> - internal_assert(WEXITSTATUS(do_fork()) == IGT_EXIT_SKIP);
> + igt_internal_assert(setenv("INTEL_SIMULATION", "1", 1) == 0);
> + igt_internal_assert(WEXITSTATUS(do_fork()) == IGT_EXIT_SKIP);
>
>
> - internal_assert(setenv("INTEL_SIMULATION", "0", 1) == 0);
> - internal_assert(WEXITSTATUS(do_fork()) == IGT_EXIT_SUCCESS);
> + igt_internal_assert(setenv("INTEL_SIMULATION", "0", 1) == 0);
> + igt_internal_assert(WEXITSTATUS(do_fork()) == IGT_EXIT_SUCCESS);
>
>
> in_fixture = false;
> in_subtest = true;
> - internal_assert(setenv("INTEL_SIMULATION", "1", 1) == 0);
> - internal_assert(WEXITSTATUS(do_fork()) == IGT_EXIT_SKIP);
> + igt_internal_assert(setenv("INTEL_SIMULATION", "1", 1) == 0);
> + igt_internal_assert(WEXITSTATUS(do_fork()) == IGT_EXIT_SKIP);
>
>
> - internal_assert(setenv("INTEL_SIMULATION", "0", 1) == 0);
> - internal_assert(WEXITSTATUS(do_fork()) == IGT_EXIT_SUCCESS);
> + igt_internal_assert(setenv("INTEL_SIMULATION", "0", 1) == 0);
> + igt_internal_assert(WEXITSTATUS(do_fork()) == IGT_EXIT_SUCCESS);
>
>
> return 0;
> diff --git a/lib/tests/igt_subtest_group.c b/lib/tests/igt_subtest_group.c
> index 7783d021..d41ac5fb 100644
> --- a/lib/tests/igt_subtest_group.c
> +++ b/lib/tests/igt_subtest_group.c
> @@ -42,7 +42,7 @@ igt_main
> }
>
> igt_subtest("not-run") {
> - internal_assert(0);
> + igt_internal_assert(0);
> }
>
> igt_subtest_group {
> @@ -50,35 +50,35 @@ igt_main
> * restore to "run testcases" when an outer
> * group is already in SKIP state. */
> igt_subtest("still-not-run") {
> - internal_assert(0);
> + igt_internal_assert(0);
> }
> }
> }
>
> igt_subtest("run") {
> t1 = true;
> - internal_assert(1);
> + igt_internal_assert(1);
> }
> }
>
> igt_subtest_group {
> igt_fixture {
> - internal_assert(t2 == 0);
> + igt_internal_assert(t2 == 0);
> t2 = 1;
> }
>
> igt_subtest("run-again") {
> - internal_assert(t2 == 1);
> + igt_internal_assert(t2 == 1);
> t2 = 2;
> }
>
> igt_fixture {
> - internal_assert(t2 == 2);
> + igt_internal_assert(t2 == 2);
> t2 = 3;
>
> }
> }
>
> - internal_assert(t1);
> - internal_assert(t2 == 3);
> + igt_internal_assert(t1);
> + igt_internal_assert(t2 == 3);
> }
> diff --git a/lib/tests/igt_tests_common.h b/lib/tests/igt_tests_common.h
> index e66ee37c..c44bfc3c 100644
> --- a/lib/tests/igt_tests_common.h
> +++ b/lib/tests/igt_tests_common.h
> @@ -25,25 +25,22 @@
> #ifndef IGT_LIB_TESTS_COMMON_H
> #define IGT_LIB_TESTS_COMMON_H
>
> -#include <assert.h>
> +#include "igt_core.h"
>
> /*
> - * We need to hide assert from the cocci igt test refactor spatch.
> - *
> - * IMPORTANT: Test infrastructure tests are the only valid places where using
> - * assert is allowed.
> + * IMPORTANT: We used to define a wrapper around assert() here, but since this
> + * breaks all of our tests if debug assertions are turned off this turned out to
> + * be a really bad idea. Don't add it back!
> */
> -#define internal_assert assert
> -
> static inline void internal_assert_wexited(int wstatus, int exitcode)
> {
> - internal_assert(WIFEXITED(wstatus) &&
> - WEXITSTATUS(wstatus) == exitcode);
> + igt_internal_assert(WIFEXITED(wstatus) &&
> + WEXITSTATUS(wstatus) == exitcode);
> }
>
> static inline void internal_assert_wsignaled(int wstatus, int signal)
> {
> - internal_assert(WIFSIGNALED(wstatus) &&
> - WTERMSIG(wstatus) == signal);
> + igt_internal_assert(WIFSIGNALED(wstatus) &&
> + WTERMSIG(wstatus) == signal);
> }
> #endif
> --
> 2.20.1
>
> _______________________________________________
> igt-dev mailing list
> igt-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/igt-dev
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
More information about the igt-dev
mailing list