[igt-dev] [PATCH i-g-t v2 3/8] lib: Stop using assert() for runtime checks
Lyude
lyude at redhat.com
Tue Apr 16 23:25:43 UTC 2019
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(); \
+ } \
+} 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
More information about the igt-dev
mailing list