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

[Lyude <lyude@redhat.com>]

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.

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@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(); \
+	} \
+} while (0)
+
 /* 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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev