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

[Petri Latvala]

On Thu, Sep 01, 2022 at 01:09:46PM +0200, Mauro Carvalho Chehab wrote:
> 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). 

Yeah the only thing that helps really is volatile. That is fully
specified as per spec for this.

Having said that: What if igt_fd_t just used volatile across the
board?


-- 
Petri Latvala