[igt-dev] [PATCH 01/76] lib: Introduce typed cleanups
Janusz Krzysztofik
janusz.krzysztofik at linux.intel.com
Tue Oct 4 10:17:20 UTC 2022
On Friday, 30 September 2022 17:16:27 CEST Kamil Konieczny wrote:
> Hi Mauro,
>
> On 2022-09-26 at 08:17:06 +0200, Mauro Carvalho Chehab wrote:
> > From: Chris Wilson <chris at chris-wilson.co.uk>
> >
> > Start introducing standard types with automatic cleanup courtesy of
> > gcc's __attribute__((cleanup)). As an example, we start with an fd
> > that will automatically call close() on going out of scope, and
> > crucially before atexit where we will want to check for resource leaks.
>
> This is "what is changing" part, please also put here "why part"
> from cover letter, so it will stay in git history.
>
> >
> > Suggested-by: Andrzej Hajda <andrzej.hajda at intel.com>
> > Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> > Cc: Andrzej Hajda <andrzej.hajda at intel.com>
> > Cc: Petri Latvala <petri.latvala at intel.com>
> > Acked-by: Nirmoy Das <nirmoy.das at linux.intel.com>
> > Signed-off-by: Mauro Carvalho Chehab <mchehab at kernel.org>
>
> I will send cc also to Zbigniew.
>
> > ---
> >
> > To avoid mailbombing on a large number of people, only mailing lists were C/C on the cover.
> > See [PATCH 00/76] at: https://lore.kernel.org/all/cover.1664173031.git.mchehab@kernel.org/
> >
> > lib/igt_core.c | 6 ++
> > lib/igt_core.h | 2 +
> > lib/igt_types.c | 17 +++++
> > lib/igt_types.h | 47 ++++++++++++
> > lib/meson.build | 1 +
> > lib/tests/bad_subtest_type.c | 19 +++++
> > lib/tests/igt_types.c | 136 +++++++++++++++++++++++++++++++++++
> > lib/tests/meson.build | 2 +
> > 8 files changed, 230 insertions(+)
> > create mode 100644 lib/igt_types.c
> > create mode 100644 lib/igt_types.h
> > create mode 100644 lib/tests/bad_subtest_type.c
> > create mode 100644 lib/tests/igt_types.c
> >
> > diff --git a/lib/igt_core.c b/lib/igt_core.c
> > index e7425326b7f0..dc6486c841f0 100644
> > --- a/lib/igt_core.c
> > +++ b/lib/igt_core.c
> > @@ -624,6 +624,12 @@ uint64_t igt_nsec_elapsed(struct timespec *start)
> > (uint64_t)NSEC_PER_SEC*(now.tv_sec - start->tv_sec));
> > }
> >
> > +void __igt_assert_in_outer_scope(void)
> > +{
> > + internal_assert(!in_subtest,
> > + "must only be called outside of a subtest");
> > +}
> > +
> > bool __igt_fixture(void)
> > {
> > internal_assert(!in_fixture,
> > diff --git a/lib/igt_core.h b/lib/igt_core.h
> > index aa98e8ed8deb..f21723dec4bc 100644
> > --- a/lib/igt_core.h
> > +++ b/lib/igt_core.h
> > @@ -135,6 +135,8 @@ struct _GKeyFile *igt_load_igtrc(void);
> > */
> > #define IGT_EXIT_ABORT 112
> >
> > +void __igt_assert_in_outer_scope(void);
> > +
> > bool __igt_fixture(void);
> > void __igt_fixture_complete(void);
> > __noreturn void __igt_fixture_end(void);
> > diff --git a/lib/igt_types.c b/lib/igt_types.c
> > new file mode 100644
> > index 000000000000..392f30fcab23
> > --- /dev/null
> > +++ b/lib/igt_types.c
> > @@ -0,0 +1,17 @@
> > +// SPDX-License-Identifier: MIT
> > +/*
> > +* Copyright © 2022 Intel Corporation
> > +*/
> > +
> > +#include <unistd.h>
> > +
> > +#include "igt_types.h"
> > +
> > +void igt_cleanup_fd(volatile int *fd)
> > +{
> > + if (!fd || *fd < 0)
> ---------------------- ^
> imho here should be != -1
> Consider also (*fd >= -1 && *fd <= 2) as these are
> stdout/in/error file descriptors.
No, the code looks correct to me. Why would you like to return without
closing if fd != -1?
>
> > + return;
> > +
> > + close(*fd);
> > + *fd = -1;
> > +}
> > diff --git a/lib/igt_types.h b/lib/igt_types.h
> > new file mode 100644
> > index 000000000000..c4bc01ecdb3b
> > --- /dev/null
> > +++ b/lib/igt_types.h
> > @@ -0,0 +1,47 @@
> > +/* SPDX-License-Identifier: MIT */
> > +/*
> > + * Copyright © 2022 Intel Corporation
> > + */
> > +
> > +#ifndef IGT_TYPES_H
> > +#define IGT_TYPES_H
> > +
> > +/*
> > + * GCC can automatically cleanup variables that go out of scope, but only
> > + * through normal means. Breaking out of scope using longjmp (i.e. igt_skip)
> > + * is not handled automatically by GCC. Such scoped variables must be tracked
> > + * in an outer scope to the skipping subtest.
> > + *
> > + * BAD:
> > + * igt_subtest("bad") {
> > + * igt_fd_t(fd);
> > + *
> > + * fd = drm_open_driver();
> > + * }
> > + *
> > + * GOOD:
> > + * igt_subtest_group() {
> > + * igt_fd_t(fd);
> > + *
> > + * igt_fixture {
> > + * fd = drm_open_driver();
> > + * }
> > + *
> > + * igt_subtest("good")
> > + * ;
>
> Should we close here fd ? This is example of good practice so
> imho here:
> igt_fixture
> close(fd);
I would rather suggest igt_cleanup_fd(fd), if at all.
>
> even if this is redundant in case of assert or fail, or just
> close(fd) without fixture ?
>
> > + * }
> > + *
> > + * A rule of thumb is that anything that is initialised through a fixture can
> > + * be combined with automatic cleanup.
> > + */
> > +
> > +#define cleanup_with(fn) __attribute__((__cleanup__(fn)))
> > +
> > +/* Prevent use within the inner scope subtests, it will be broken by igt_skip */
> > +#define IGT_OUTER_SCOPE_INIT(x) ({ __igt_assert_in_outer_scope(); x; })
> > +
> > +void igt_cleanup_fd(volatile int *fd);
> > +#define igt_fd_t(x__) \
> > + volatile int x__ cleanup_with(igt_cleanup_fd) = IGT_OUTER_SCOPE_INIT(-1)
> > +
> > +#endif /* IGT_TYPES_H */
> > diff --git a/lib/meson.build b/lib/meson.build
> > index 548835b5833f..c665bd25073a 100644
> > --- a/lib/meson.build
> > +++ b/lib/meson.build
> > @@ -38,6 +38,7 @@ lib_sources = [
> > 'igt_sysrq.c',
> > 'igt_taints.c',
> > 'igt_thread.c',
> > + 'igt_types.c',
> > 'igt_vec.c',
> > 'igt_vgem.c',
> > 'igt_x86.c',
> > diff --git a/lib/tests/bad_subtest_type.c b/lib/tests/bad_subtest_type.c
> > new file mode 100644
> > index 000000000000..7f5efdfba8e2
> > --- /dev/null
> > +++ b/lib/tests/bad_subtest_type.c
> > @@ -0,0 +1,19 @@
> > +// SPDX-License-Identifier: MIT
> > +/*
> > +* Copyright © 2022 Intel Corporation
> > +*/
> > +
> > +#include "igt_core.h"
> > +#include "igt_types.h"
>
> Please put here description with IGT_TEST_DESCRIPTION() macro.
>
> > +
> > +igt_main
> > +{
>
> Please put description before test with igt_describe().
AFAICS none of lib/tests/*, except igt_describe that needs those clauses
because it exercises them, follows that convention. If you found them
helpful then OK, that could be a field for improvement, but that should not
block adding new self IGT tests as needed, I believe.
>
> Also copy comment from main test that after running this
> SIGABORT is intentional.
Since this test is added to lib_fail_tests category (see below changes to lib/
tests/meson.build) then I don't think we need further clarifications.
Thanks,
Janusz
>
> > + igt_subtest("bad-scoped-variable") {
> > + /*
> > + * Not allowed to nest a scoped variable inside a subtest as
> > + * we expect to longjmp out of the subtest on failure/skip
> > + * and automatic cleanup is not invoked for such jmps.
> > + */
> > + igt_fd_t(f);
> > + }
> > +}
> > diff --git a/lib/tests/igt_types.c b/lib/tests/igt_types.c
> > new file mode 100644
> > index 000000000000..2bd74df3a7d6
> > --- /dev/null
> > +++ b/lib/tests/igt_types.c
> > @@ -0,0 +1,136 @@
> > +// SPDX-License-Identifier: MIT
> > +/*
> > +* Copyright © 2022 Intel Corporation
> > +*/
> > +
> > +#include "igt_core.h"
> > +#include "igt_types.h"
> > +
>
> Same here, put global description.
>
> > +/* a lookalike of igt_fd_t for testing */
> > +#define scoped_int_t(x__) \
> > + volatile int x__ cleanup_with(cleanup) = IGT_OUTER_SCOPE_INIT(-1)
> > +
> > +static int cleanup_called;
> > +
> > +static void cleanup(volatile int *x)
> > +{
> > + cleanup_called++;
> > + *x = -1;
> > +}
> > +
> > +static void delegate(void)
> > +{
> > + scoped_int_t(x);
> > +
> > + igt_fixture
> > + x = 1;
> > +
>
> Put description here and before new tests below.
>
> > + igt_subtest("empty-subtest")
> > + x = 2;
> > +
> > + igt_fixture {
> > + /* Check that we went through both blocks without cleanup */
> > + igt_assert(!cleanup_called);
> > + igt_assert(x == 2);
> > + }
> > +}
> > +
> > +static void skip_delegate(void)
> > +{
> > + scoped_int_t(x);
> > +
> > + igt_fixture
> > + x = 1;
> > +
> > + igt_subtest("skipped-subtest") {
> > + igt_skip("Early skip for testing\n");
> > + x = 2; /* not reached due to lonjmp from igt_skip */
> > + }
> > +
> > + igt_fixture {
> > + /* Check that we went through both blocks without cleanup */
> > + igt_assert(!cleanup_called);
> > + igt_assert(x == 1);
> > + }
> > +}
> > +
> > +igt_main
> > +{
> > + /* Basic check that scopes will call their destructor */
> > + cleanup_called = 0;
> > + igt_fixture {
> > + scoped_int_t(x);
> > + }
> > + igt_subtest("cleanup-after-fixture")
> > + igt_assert(cleanup_called);
> > +
> > + /* But not before we go out of scope! */
> > + cleanup_called = 0;
> > + igt_subtest_group {
> > + scoped_int_t(x);
> > +
> > + igt_fixture {
> > + x = 0xdeadbeef;
> > + }
> > +
> > + igt_subtest("cleanup-not-before-subtest-group") {
> > + /* Check no scope destructor was called */
> > + igt_assert(cleanup_called == 0);
> > + /* Confirm that we did pass through a scoped block */
> > + igt_assert_eq_u32(x, 0xdeadbeef);
> > + }
> > + }
> > + igt_subtest("cleanup-after-subtest-group")
> > + igt_assert(cleanup_called);
> > +
> > + /* longjmp and __attribute__(cleanup) do not mix well together */
> > +#if 0 /* See bad_subtest_type, this is caught by an internal assertion */
> > + cleanup_called = 0;
> > + igt_subtest("skip-subtest") {
> > + scoped_int_t(x);
> > +
> > + igt_skip("Checking scoped cleanup on skip\n");
> > + }
> > + igt_subtest("cleanup-after-skip")
> > + igt_assert_f(!cleanup_called,
> > + "scoped closure was not compatible with igt_skip\n");
> > +#endif
> > +
> > + /*
> > + * However, if we igt_skip inside another block (subtest-group), then we
> > + * will get cleanup on the outer scope.
> > + */
> > + cleanup_called = 0;
> > + igt_subtest_group {
> > + scoped_int_t(x);
> > +
> > + igt_subtest("skip-subtest-group")
> > + igt_skip("Checking scoped cleanup after skip\n");
> > + }
> > + igt_subtest("cleanup-after-skip-group")
> > + igt_assert(cleanup_called);
> > +
> > + /* Check the same holds true for function calls */
> > + cleanup_called = 0;
> > + delegate();
> > + igt_subtest("cleanup-after-delegation")
> > + igt_assert(cleanup_called);
> > +
> > + cleanup_called = 0;
> > + igt_subtest_group
> > + delegate();
> > + igt_subtest("cleanup-after-group-delegation")
> > + igt_assert(cleanup_called);
> > +
> > + /* Check what happens with a igt_skip inside a function */
> > + cleanup_called = 0;
> > + skip_delegate();
> > + igt_subtest("cleanup-after-skipped-delegation")
> > + igt_assert(cleanup_called);
> > +
> > + cleanup_called = 0;
> > + igt_subtest_group
> > + skip_delegate();
> > + igt_subtest("cleanup-after-group-skipped-0delegation")
> ------------------------------------------------ ^
> s/-0delegation/-delegation/
>
> Regards,
> Kamil
>
> > + igt_assert(cleanup_called);
> > +}
> > diff --git a/lib/tests/meson.build b/lib/tests/meson.build
> > index ceaf548b2b2f..d5666c246146 100644
> > --- a/lib/tests/meson.build
> > +++ b/lib/tests/meson.build
> > @@ -19,10 +19,12 @@ lib_tests = [
> > 'igt_stats',
> > 'igt_subtest_group',
> > 'igt_thread',
> > + 'igt_types',
> > 'i915_perf_data_alignment',
> > ]
> >
> > lib_fail_tests = [
> > + 'bad_subtest_type',
> > 'igt_no_subtest',
> > 'igt_simple_test_subtests',
> > 'igt_timeout',
>
More information about the igt-dev
mailing list