[igt-dev] [PATCH i-g-t 2/5] lib/tests/igt_subtest_group: Operate within defined behavior

[Ser, Simon]

On Thu, 2019-05-09 at 13:03 +0300, Arkadiusz Hiler wrote:
> Quoting lib/igt_core.c:
>  * - Code blocks with magic control flow are implemented with setjmp() and
>  *   longjmp(). This applies to #igt_fixture and #igt_subtest blocks and all the
>  *   three variants to finish test: igt_success(), igt_skip() and igt_fail().
>  *   Mostly this is of no concern, except when such a control block changes
>  *   stack variables defined in the same function as the control block resides.
>  *   Any store/load behaviour after a longjmp() is ill-defined for these
>  *   variables. Avoid such code.
>  *
>  *   Quoting the man page for longjmp():
>  *
>  *   "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() call;
>  *    - "their values are changed between the calls to setjmp() and longjmp(); and
>  *    - "they are not declared as volatile."
> 
> igt_subtest_group test uses two local variables for tracking the state
> of execution, making sure that skips are working correctly.
> 
> We can just make them volatile to be back on the defined grounds.
> 
> Cc: Simon Ser <simon.ser at intel.com>
> Cc: Daniel Vetter <daniel.vetter at ffwll.ch>
> Signed-off-by: Arkadiusz Hiler <arkadiusz.hiler at intel.com>
> ---
>  lib/tests/igt_subtest_group.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/lib/tests/igt_subtest_group.c b/lib/tests/igt_subtest_group.c
> index 7783d021..d8e7d861 100644
> --- a/lib/tests/igt_subtest_group.c
> +++ b/lib/tests/igt_subtest_group.c
> @@ -28,8 +28,8 @@
>  
>  igt_main
>  {
> -	bool t1 = false;
> -	int t2 = 0;
> +	volatile bool t1 = false;
> +	volatile int t2 = 0;

Hi,

I think a little comment explaining why these need to be volatile would
be a good addition to this patch. Something among the lines of: "Local
variables updated in subtests have undefined contents unless marked as
volatile".

>  	igt_subtest_group {
>  		igt_fixture {