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

[Mauro Carvalho Chehab]

On Thu, 1 Sep 2022 12:56:09 +0200
Mauro Carvalho Chehab <mauro.chehab at linux.intel.com> wrote:

> 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.

Answering myself:  this won't work, as the macro uses __cleanup__
atribute, which is only valid for stack-allocated variables:

	../tests/i915/gem_request_retire.c:109:2: warning: '__cleanup__' attribute only applies to local variables [-Wignored-attributes]

A solution that would work would be to store a list of file
descriptors on a static list and then use it during cleanup time
(assuming that clang will still call the cleanup function with 
longjmp). 

> > 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