[igt-dev] [PATCH i-g-t 3/7] lib: Stop using assert() for runtime checks
Lyude Paul
lyude at redhat.com
Wed Apr 24 00:36:43 UTC 2019
On Wed, 2019-04-17 at 11:52 +0300, Petri Latvala wrote:
> 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.
If igt really isn't supposed to be built with -Db_ndebug=true we really should
add explicit checks for this in our meson.build file, e.g.
if get_option("b_ndebug") == true
error("Building with -Db_ndebug=true is not supported")
endif
Especially since the default in meson upstream is going to be changing to having
-Db_ndebug=true on release builds pretty soon. I'll include a patch for this in
the v3 respin.
>
>
> > 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.
>
>
>
More information about the igt-dev
mailing list