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

[Mauro Carvalho Chehab]

On Thu, 1 Sep 2022 14:24:41 +0300
Petri Latvala <petri.latvala at intel.com> wrote:

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

Do you mean folding this change on patch 01/74?

<snip>
diff --git a/lib/igt_types.c b/lib/igt_types.c
index 7416b0fffc04..392f30fcab23 100644
--- a/lib/igt_types.c
+++ b/lib/igt_types.c
@@ -7,7 +7,7 @@
 
 #include "igt_types.h"
 
-void igt_cleanup_fd(int *fd)
+void igt_cleanup_fd(volatile int *fd)
 {
 	if (!fd || *fd < 0)
 		return;
diff --git a/lib/igt_types.h b/lib/igt_types.h
index 1852026969ae..c4bc01ecdb3b 100644
--- a/lib/igt_types.h
+++ b/lib/igt_types.h
@@ -40,8 +40,8 @@
 /* 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(int *fd);
+void igt_cleanup_fd(volatile int *fd);
 #define igt_fd_t(x__) \
-	int x__ cleanup_with(igt_cleanup_fd) = IGT_OUTER_SCOPE_INIT(-1)
+	volatile int x__ cleanup_with(igt_cleanup_fd) = IGT_OUTER_SCOPE_INIT(-1)
 
 #endif /* IGT_TYPES_H */

</snip>

This builds fine, but didn't try yet checking if it will do the right 
thing.

Regards,
Mauro