Re: [PATCH] lib: Declare loop variable as volatile before setjmp

From: Daniel Vetter <daniel@ffwll.ch>
To: Tomeu Vizoso <tomeu.vizoso@collabora.com>
Cc: Intel GFX discussion <intel-gfx@lists.freedesktop.org>
Subject: Re: [PATCH] lib: Declare loop variable as volatile before setjmp
Date: Wed, 20 Apr 2016 14:53:49 +0200	[thread overview]
Message-ID: <20160420125349.GY2510@phenom.ffwll.local> (raw)
In-Reply-To: <1460721604-19685-1-git-send-email-tomeu.vizoso@collabora.com>

On Fri, Apr 15, 2016 at 02:00:04PM +0200, Tomeu Vizoso wrote:
> The variable used as loop counter in the igt_fixture macro had
> unspecified value from the setjmp(3) man page quoted below. Because of
> that, in certain circumstances and with -O2 and -Os, the initialization
> of that variable would be eliminated and the compiler would complain of
> uninitialized usage. Below can be found a snippet that reproduces the
> problem with GCC 5.3.1 and 4.9.3 and the errors as printed by 5.3.1.
> 
> "The compiler may optimize variables into registers, and longjmp() may
> restore the values of other registers in addition to the stack pointer
> and program  counter. Consequently, the values of automatic variables
> are unspecified after a call to longjmp() if they meet all the following
> criteria:
> 
> ·  they are local to the function that made the corresponding setjmp(3)
> call;
> 
> ·  their values are changed between the calls to setjmp(3) and
> longjmp(); and
> 
> ·  they are not declared as volatile."
> 
> static void test(void)
> {
> 	igt_subtest_group {
> 		igt_fixture {
> 		}
> 
> 		igt_subtest("foo") {
> 		}
> 
> 		igt_fixture {
> 		}
> 	}
> }
> 
> In file included from lib/intel_batchbuffer.h:8:0,
>                  from lib/drmtest.h:39,
>                  from lib/igt.h:27,
>                  from tests/kms_addfb_basic.c:28:
> tests/kms_addfb_basic.c: In function 'tiling_tests.isra.0':
> lib/igt_core.h:110:43: warning: '__tmpint245' is used uninitialized in
> this function [-Wuninitialized]
>  #define igt_fixture for (int igt_tokencat(__tmpint,__LINE__) = 0; \
>                                            ^
> lib/igt_core.h:110:43: note: '__tmpint245' was declared here
>  #define igt_fixture for (int igt_tokencat(__tmpint,__LINE__) = 0; \
>                                            ^
> lib/igt_core.h:148:31: note: in definition of macro '__igt_tokencat2'
>  #define __igt_tokencat2(x, y) x ## y
>                                ^
> lib/igt_core.h:110:30: note: in expansion of macro 'igt_tokencat'
>  #define igt_fixture for (int igt_tokencat(__tmpint,__LINE__) = 0; \
>                               ^
> tests/kms_addfb_basic.c:245:3: note: in expansion of macro 'igt_fixture'
>    igt_fixture {
> 
> Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>

Oh nice, I never figured this out. Ther's another char tmp in the
igt_subtest_f macro, maybe we should fix that up too, just for safety?

Meanwhile applied your patch, thanks.
-Daniel

> ---
>  lib/igt_core.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/lib/igt_core.h b/lib/igt_core.h
> index b3fa7356e473..1b62371a7138 100644
> --- a/lib/igt_core.h
> +++ b/lib/igt_core.h
> @@ -107,7 +107,7 @@ void __igt_fixture_end(void) __attribute__((noreturn));
>   * enumeration (e.g. when enumerating on systems without an intel gpu) such
>   * blocks should be annotated with igt_fixture.
>   */
> -#define igt_fixture for (int igt_tokencat(__tmpint,__LINE__) = 0; \
> +#define igt_fixture for (volatile int igt_tokencat(__tmpint,__LINE__) = 0; \
>  			 igt_tokencat(__tmpint,__LINE__) < 1 && \
>  			 __igt_fixture() && \
>  			 (sigsetjmp(igt_subtest_jmpbuf, 1) == 0); \
> -- 
> 2.5.5
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx