[PATCH i-g-t] tests/intel/gem_exec_capture: fix use of igt_allow_hang() / igt_disallow_hang()

[Kamil Konieczny]

Hi Peter,
On 2024-04-11 at 21:19:23 +0200, Peter Senna Tschudin wrote:

small nit on subject, you fixed uninitialized var used, so imho

[PATCH i-g-t] tests/intel/gem_exec_capture: fix use of igt_allow_hang() / igt_disallow_hang()

would be better and shorter with:

[PATCH i-g-t] tests/intel/gem_exec_capture: fix use of uninitialized hang var

> The function main has a variable igt_hang_t hang defined, and it is
> currently used by igt_disallow_hang(). However igt_disallow_hang() is
> using the variable uninitialized making valgrind unhappy:
> 
>  Syscall param ioctl(generic) points to uninitialised byte(s)
>     at 0x4ACD3ED: ioctl (in /usr/lib64/libc.so.6)
>     by 0x49ACF4F: drmIoctl (xf86drm.c:704)
>     by 0x488AF46: __gem_context_set_param (gem_context.c:260)
>     by 0x48A4D9F: context_set_ban (igt_gt.c:161)
>     by 0x48A53DC: igt_disallow_hang (igt_gt.c:228)
>     by 0x4013D7: __igt_unique____real_main959 (gem_exec_capture.c:1069)
>     by 0x401242: main (gem_exec_capture.c:959)
>   Address 0x1fff000170 is on thread 1's stack
>   in frame #3, created by context_set_ban (igt_gt.c:154)
>   Uninitialised value was created by a stack allocation
>     at 0x401340: __igt_unique____real_main959 (gem_exec_capture.c:960)
> 
>  Conditional jump or move depends on uninitialised value(s)
>     at 0x48A53E2: igt_disallow_hang (igt_gt.c:230)
>     by 0x4013D7: __igt_unique____real_main959 (gem_exec_capture.c:1069)
>     by 0x401242: main (gem_exec_capture.c:959)
>   Uninitialised value was created by a stack allocation
>     at 0x401340: __igt_unique____real_main959 (gem_exec_capture.c:960)
> 
>  Syscall param ioctl(generic) points to uninitialised byte(s)
>     at 0x4ACD3ED: ioctl (in /usr/lib64/libc.so.6)
>     by 0x49ACF4F: drmIoctl (xf86drm.c:704)
>     by 0x488AF46: __gem_context_set_param (gem_context.c:260)
>     by 0x48A540A: igt_disallow_hang (igt_gt.c:236)
>     by 0x4013D7: __igt_unique____real_main959 (gem_exec_capture.c:1069)
>     by 0x401242: main (gem_exec_capture.c:959)
>   Address 0x1fff0001b0 is on thread 1's stack
>   in frame #3, created by igt_disallow_hang (igt_gt.c:224)
>   Uninitialised value was created by a stack allocation
>     at 0x401340: __igt_unique____real_main959 (gem_exec_capture.c:960)

Good catch, could you leave only that 3rd report from valgrind
starting from 'Syscall param ...'? imho this should explain it.

With that,
Reviewed-by: Kamil Konieczny <kamil.konieczny at linux.intel.com>

> 
> This patch sets the content of the variable hang with the return value
> of igt_allow_hang() following what is used elsewhere in the codebase.
> After this patch valgrind does not complain about the  errors listed.
> 
> Signed-off-by: Peter Senna Tschudin <me at petersenna.com>
> ---
>  tests/intel/gem_exec_capture.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tests/intel/gem_exec_capture.c b/tests/intel/gem_exec_capture.c
> index 8800fa586..e462aa96f 100644
> --- a/tests/intel/gem_exec_capture.c
> +++ b/tests/intel/gem_exec_capture.c
> @@ -990,7 +990,7 @@ igt_main
>  
>  			gem_context_set_param(fd, &param);
>  		}
> -		igt_allow_hang(fd, ctx->id, HANG_ALLOW_CAPTURE | HANG_WANT_ENGINE_RESET);
> +		hang = igt_allow_hang(fd, ctx->id, HANG_ALLOW_CAPTURE | HANG_WANT_ENGINE_RESET);
>  
>  		dir = igt_sysfs_open(fd);
>  		igt_require(igt_sysfs_set(dir, "error", "Begone!"));
> -- 
> 2.44.0
>