[igt-dev] [PATCH i-g-t 01/74] lib: Introduce typed cleanups

[Mauro Carvalho Chehab]

On Thu, 1 Sep 2022 12:19:28 +0300
Petri Latvala <petri.latvala at intel.com> wrote:

> On Thu, Sep 01, 2022 at 08:36:09AM +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.
> > 
> > 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>

...

> > +void igt_cleanup_fd(int *fd);
> > +#define igt_fd_t(x__) \
> > +	int x__ cleanup_with(igt_cleanup_fd) = IGT_OUTER_SCOPE_INIT(-1)
> > +
> > +#endif /* IGT_TYPES_H */

...

> 
> This fails with clang currently, and might fail with gcc in the
> future. longjmp is funky.

It is unlikely that gcc would change it, but yeah, there's a possibility.

There's an easy to workaround: just check if CONFIG_CLANG_VERSION is
defined, disabling the logic on such case with something like:

/*
 * FIXME: on clang, stack variables are lost after setjmp()
 */
#ifdef CONFIG_CLANG_VERSION
#  define igt_fd_t(x__) int x__
#else
#  define igt_fd_t(x__) \
       int x__ cleanup_with(igt_cleanup_fd) = IGT_OUTER_SCOPE_INIT(-1)
#endif

This would keep the file descriptors opened on Clang, but it would
at least solve for gcc.

> If an automatic (stack) variable is assigned after setjmp(), the
> longjmp to "before" the assignment has the variable in unspecified
> state as per the C spec. In the sense that the value can be either.

I see. Yeah, handling undefined behavior cases are complex.

I guess one possible solution would be to define the macro as:

#define igt_fd_t(x__) \
       static int x__ cleanup_with(igt_cleanup_fd) = IGT_OUTER_SCOPE_INIT(-1)
#endif

but I didn't check the C spec to see if this has a defined behavior,
nor tested it.

> We've already had to deal with some of this, see the comment at the
> beginning of runner/runner_tests.c and the sprinkled use of volatile
> for variables that are declared in subtest_group scope.
> 
> (It could be a gcc implementation detail but variables declared in
> igt_main scope have been fine in testing this curiosity)

Regards,
Mauro