[PATCH i-g-t v4] Fix memory access issue due to variable block scope

[PATCH i-g-t v4] Fix memory access issue due to variable block scope

[Peter Senna Tschudin]

This patch fixes the tests many-4k-incremental and many-4k-zero from
gem_exec_capture that are currently failing with an invalid file
descriptor error.

tests/intel/gem_exec_capture.c
    many(), userptr(), capture_invisible()
        find_first_available_engine()
            for_each_ctx_engine()
                for_each_ctx_cfg_engine()

find_first_available_engine() expects a struct intel_execution_engine2
*e as parameter. for_each_ctx_cfg_engine() allocates a struct
intel_engine_data ed inside a for loop and then update e to point to an
element of ed.

The problem is that ed has the block scope of the for loop, and trying
to access its content through e after the for loop has ended creates
undefined behavior.

This patch updates e to point to the same content at a different memory
region, to avoid the block scope of ed.

v4: Move `saved = configure_hangs(fd, e, ctx->id);` to inside the for
loop where it is safe to use 'e'. This is because 'e' points to an
element of a variable that is defined with the block scope of the for
loop.

Signed-off-by: Peter Senna Tschudin <me@petersenna.com>
---
 tests/intel/gem_exec_capture.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/tests/intel/gem_exec_capture.c b/tests/intel/gem_exec_capture.c
index 57b178f3e..f7b10bdd4 100644
--- a/tests/intel/gem_exec_capture.c
+++ b/tests/intel/gem_exec_capture.c
@@ -665,10 +665,12 @@ static bool needs_recoverable_ctx(int fd)
 		ctx = intel_ctx_create_all_physical(fd); \
 		igt_assert(ctx); \
 		for_each_ctx_engine(fd, ctx, e) \
-			for_each_if(gem_class_can_store_dword(fd, e->class)) \
+			for_each_if(gem_class_can_store_dword(fd, e->class)) { \
+				saved = configure_hangs(fd, e, ctx->id); \
 				break; \
+			}  \
+		e = &saved.engine; \
 		igt_assert(e); \
-		saved = configure_hangs(fd, e, ctx->id); \
 	} while(0)
 
 static void many(int fd, int dir, uint64_t size, unsigned int flags)
-- 
2.44.0

✓ CI.xeBAT: success for Fix memory access issue due to variable block scope (rev4)

[Patchwork]

[-- Attachment #1: Type: text/plain, Size: 3222 bytes --]

== Series Details ==

Series: Fix memory access issue due to variable block scope (rev4)
URL   : https://patchwork.freedesktop.org/series/131602/
State : success

== Summary ==

CI Bug Log - changes from XEIGT_7785_BAT -> XEIGTPW_10926_BAT
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

  

Participating hosts (4 -> 5)
------------------------------

  Additional (1): bat-lnl-1 

Possible new issues
-------------------

  Here are the unknown changes that may have been introduced in XEIGTPW_10926_BAT:

### IGT changes ###

#### Suppressed ####

  The following results come from untrusted machines, tests, or statuses.
  They do not affect the overall result.

  * igt@xe_mmap@vram:
    - {bat-lnl-1}:        NOTRUN -> [SKIP][1]
   [1]: https://intel-gfx-ci.01.org/tree/intel-xe/IGTPW_10926/bat-lnl-1/igt@xe_mmap@vram.html

  
Known issues
------------

  Here are the changes found in XEIGTPW_10926_BAT that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@xe_evict@evict-beng-small:
    - bat-dg2-oem2:       NOTRUN -> [DMESG-WARN][2] ([Intel XE#1373])
   [2]: https://intel-gfx-ci.01.org/tree/intel-xe/IGTPW_10926/bat-dg2-oem2/igt@xe_evict@evict-beng-small.html

  * igt@xe_evict@evict-cm-threads-small:
    - bat-dg2-oem2:       NOTRUN -> [INCOMPLETE][3] ([Intel XE#1027] / [Intel XE#804])
   [3]: https://intel-gfx-ci.01.org/tree/intel-xe/IGTPW_10926/bat-dg2-oem2/igt@xe_evict@evict-cm-threads-small.html

  * igt@xe_evict@evict-mixed-threads-small:
    - bat-dg2-oem2:       NOTRUN -> [INCOMPLETE][4] ([Intel XE#804])
   [4]: https://intel-gfx-ci.01.org/tree/intel-xe/IGTPW_10926/bat-dg2-oem2/igt@xe_evict@evict-mixed-threads-small.html

  
#### Warnings ####

  * igt@xe_evict@evict-beng-mixed-threads-small-multi-vm:
    - bat-dg2-oem2:       [INCOMPLETE][5] ([Intel XE#804]) -> [TIMEOUT][6] ([Intel XE#1027] / [Intel XE#1088])
   [5]: https://intel-gfx-ci.01.org/tree/intel-xe/IGT_7785/bat-dg2-oem2/igt@xe_evict@evict-beng-mixed-threads-small-multi-vm.html
   [6]: https://intel-gfx-ci.01.org/tree/intel-xe/IGTPW_10926/bat-dg2-oem2/igt@xe_evict@evict-beng-mixed-threads-small-multi-vm.html

  
  {name}: This element is suppressed. This means it is ignored when computing
          the status of the difference (SUCCESS, WARNING, or FAILURE).

  [Intel XE#1027]: https://gitlab.freedesktop.org/drm/xe/kernel/issues/1027
  [Intel XE#1088]: https://gitlab.freedesktop.org/drm/xe/kernel/issues/1088
  [Intel XE#1373]: https://gitlab.freedesktop.org/drm/xe/kernel/issues/1373
  [Intel XE#688]: https://gitlab.freedesktop.org/drm/xe/kernel/issues/688
  [Intel XE#804]: https://gitlab.freedesktop.org/drm/xe/kernel/issues/804


Build changes
-------------

  * IGT: IGT_7785 -> IGTPW_10926
  * Linux: xe-995-79a0d94585cad50ad2446ebdf3f1317b7d18157c -> xe-998-f9c56f1a03b5c35488671e4ffe61e28b12ffe163

  IGTPW_10926: 10926
  IGT_7785: 7785
  xe-995-79a0d94585cad50ad2446ebdf3f1317b7d18157c: 79a0d94585cad50ad2446ebdf3f1317b7d18157c
  xe-998-f9c56f1a03b5c35488671e4ffe61e28b12ffe163: f9c56f1a03b5c35488671e4ffe61e28b12ffe163

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/intel-xe/IGTPW_10926/index.html

[-- Attachment #2: Type: text/html, Size: 4048 bytes --]

✗ Fi.CI.BAT: failure for Fix memory access issue due to variable block scope (rev4)

[Patchwork]

[-- Attachment #1: Type: text/plain, Size: 11055 bytes --]

== Series Details ==

Series: Fix memory access issue due to variable block scope (rev4)
URL   : https://patchwork.freedesktop.org/series/131602/
State : failure

== Summary ==

CI Bug Log - changes from CI_DRM_14489 -> IGTPW_10926
====================================================

Summary
-------

  **FAILURE**

  Serious unknown changes coming with IGTPW_10926 absolutely need to be
  verified manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in IGTPW_10926, please notify your bug team (I915-ci-infra@lists.freedesktop.org) to allow them
  to document this new failure mode, which will reduce false positives in CI.

  External URL: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_10926/index.html

Participating hosts (38 -> 37)
------------------------------

  Additional (2): bat-dg1-7 fi-kbl-8809g 
  Missing    (3): bat-arls-4 bat-jsl-1 fi-snb-2520m 

Possible new issues
-------------------

  Here are the unknown changes that may have been introduced in IGTPW_10926:

### IGT changes ###

#### Possible regressions ####

  * igt@i915_selftest@live@gt_pm:
    - fi-kbl-7567u:       [PASS][1] -> [INCOMPLETE][2]
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_14489/fi-kbl-7567u/igt@i915_selftest@live@gt_pm.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_10926/fi-kbl-7567u/igt@i915_selftest@live@gt_pm.html

  
Known issues
------------

  Here are the changes found in IGTPW_10926 that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@gem_mmap@basic:
    - bat-dg1-7:          NOTRUN -> [SKIP][3] ([i915#4083])
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_10926/bat-dg1-7/igt@gem_mmap@basic.html
    - bat-dg2-11:         NOTRUN -> [SKIP][4] ([i915#4083])
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_10926/bat-dg2-11/igt@gem_mmap@basic.html

  * igt@gem_tiled_fence_blits@basic:
    - bat-dg1-7:          NOTRUN -> [SKIP][5] ([i915#4077]) +2 other tests skip
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_10926/bat-dg1-7/igt@gem_tiled_fence_blits@basic.html
    - bat-dg2-11:         NOTRUN -> [SKIP][6] ([i915#4077]) +2 other tests skip
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_10926/bat-dg2-11/igt@gem_tiled_fence_blits@basic.html

  * igt@gem_tiled_pread_basic:
    - bat-dg1-7:          NOTRUN -> [SKIP][7] ([i915#4079]) +1 other test skip
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_10926/bat-dg1-7/igt@gem_tiled_pread_basic.html
    - bat-dg2-11:         NOTRUN -> [SKIP][8] ([i915#4079]) +1 other test skip
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_10926/bat-dg2-11/igt@gem_tiled_pread_basic.html

  * igt@i915_pm_rps@basic-api:
    - bat-dg1-7:          NOTRUN -> [SKIP][9] ([i915#6621])
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_10926/bat-dg1-7/igt@i915_pm_rps@basic-api.html
    - bat-dg2-11:         NOTRUN -> [SKIP][10] ([i915#6621])
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_10926/bat-dg2-11/igt@i915_pm_rps@basic-api.html

  * igt@i915_selftest@live@hangcheck:
    - bat-adlp-11:        [PASS][11] -> [ABORT][12] ([i915#10021])
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_14489/bat-adlp-11/igt@i915_selftest@live@hangcheck.html
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_10926/bat-adlp-11/igt@i915_selftest@live@hangcheck.html

  * igt@kms_addfb_basic@addfb25-x-tiled-mismatch-legacy:
    - bat-dg1-7:          NOTRUN -> [SKIP][13] ([i915#4212]) +7 other tests skip
   [13]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_10926/bat-dg1-7/igt@kms_addfb_basic@addfb25-x-tiled-mismatch-legacy.html
    - bat-dg2-11:         NOTRUN -> [SKIP][14] ([i915#4212]) +7 other tests skip
   [14]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_10926/bat-dg2-11/igt@kms_addfb_basic@addfb25-x-tiled-mismatch-legacy.html

  * igt@kms_addfb_basic@addfb25-y-tiled-small-legacy:
    - bat-dg2-11:         NOTRUN -> [SKIP][15] ([i915#5190])
   [15]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_10926/bat-dg2-11/igt@kms_addfb_basic@addfb25-y-tiled-small-legacy.html

  * igt@kms_addfb_basic@basic-y-tiled-legacy:
    - bat-dg1-7:          NOTRUN -> [SKIP][16] ([i915#4215])
   [16]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_10926/bat-dg1-7/igt@kms_addfb_basic@basic-y-tiled-legacy.html
    - bat-dg2-11:         NOTRUN -> [SKIP][17] ([i915#4215] / [i915#5190])
   [17]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_10926/bat-dg2-11/igt@kms_addfb_basic@basic-y-tiled-legacy.html

  * igt@kms_cursor_legacy@basic-busy-flip-before-cursor-atomic:
    - bat-dg2-11:         NOTRUN -> [SKIP][18] ([i915#4103] / [i915#4213]) +1 other test skip
   [18]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_10926/bat-dg2-11/igt@kms_cursor_legacy@basic-busy-flip-before-cursor-atomic.html

  * igt@kms_cursor_legacy@basic-busy-flip-before-cursor-legacy:
    - bat-dg1-7:          NOTRUN -> [SKIP][19] ([i915#4103] / [i915#4213]) +1 other test skip
   [19]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_10926/bat-dg1-7/igt@kms_cursor_legacy@basic-busy-flip-before-cursor-legacy.html

  * igt@kms_dsc@dsc-basic:
    - bat-dg2-11:         NOTRUN -> [SKIP][20] ([i915#3555] / [i915#3840])
   [20]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_10926/bat-dg2-11/igt@kms_dsc@dsc-basic.html
    - bat-dg1-7:          NOTRUN -> [SKIP][21] ([i915#3555] / [i915#3840])
   [21]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_10926/bat-dg1-7/igt@kms_dsc@dsc-basic.html

  * igt@kms_force_connector_basic@force-load-detect:
    - bat-dg1-7:          NOTRUN -> [SKIP][22]
   [22]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_10926/bat-dg1-7/igt@kms_force_connector_basic@force-load-detect.html
    - bat-dg2-11:         NOTRUN -> [SKIP][23]
   [23]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_10926/bat-dg2-11/igt@kms_force_connector_basic@force-load-detect.html

  * igt@kms_force_connector_basic@prune-stale-modes:
    - bat-dg2-11:         NOTRUN -> [SKIP][24] ([i915#5274])
   [24]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_10926/bat-dg2-11/igt@kms_force_connector_basic@prune-stale-modes.html

  * igt@kms_hdmi_inject@inject-audio:
    - bat-dg1-7:          NOTRUN -> [SKIP][25] ([i915#433])
   [25]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_10926/bat-dg1-7/igt@kms_hdmi_inject@inject-audio.html

  * igt@kms_pm_backlight@basic-brightness:
    - bat-dg1-7:          NOTRUN -> [SKIP][26] ([i915#5354])
   [26]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_10926/bat-dg1-7/igt@kms_pm_backlight@basic-brightness.html
    - bat-dg2-11:         NOTRUN -> [SKIP][27] ([i915#5354])
   [27]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_10926/bat-dg2-11/igt@kms_pm_backlight@basic-brightness.html

  * igt@kms_psr@psr-primary-page-flip:
    - bat-dg1-7:          NOTRUN -> [SKIP][28] ([i915#1072] / [i915#9732]) +3 other tests skip
   [28]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_10926/bat-dg1-7/igt@kms_psr@psr-primary-page-flip.html

  * igt@kms_psr@psr-sprite-plane-onoff:
    - bat-dg2-11:         NOTRUN -> [SKIP][29] ([i915#1072] / [i915#9732]) +3 other tests skip
   [29]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_10926/bat-dg2-11/igt@kms_psr@psr-sprite-plane-onoff.html

  * igt@kms_setmode@basic-clone-single-crtc:
    - bat-dg2-11:         NOTRUN -> [SKIP][30] ([i915#3555])
   [30]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_10926/bat-dg2-11/igt@kms_setmode@basic-clone-single-crtc.html
    - bat-dg1-7:          NOTRUN -> [SKIP][31] ([i915#3555])
   [31]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_10926/bat-dg1-7/igt@kms_setmode@basic-clone-single-crtc.html

  * igt@prime_vgem@basic-fence-flip:
    - bat-dg1-7:          NOTRUN -> [SKIP][32] ([i915#3708]) +3 other tests skip
   [32]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_10926/bat-dg1-7/igt@prime_vgem@basic-fence-flip.html
    - bat-dg2-11:         NOTRUN -> [SKIP][33] ([i915#3708])
   [33]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_10926/bat-dg2-11/igt@prime_vgem@basic-fence-flip.html

  * igt@prime_vgem@basic-fence-mmap:
    - bat-dg1-7:          NOTRUN -> [SKIP][34] ([i915#3708] / [i915#4077]) +1 other test skip
   [34]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_10926/bat-dg1-7/igt@prime_vgem@basic-fence-mmap.html
    - bat-dg2-11:         NOTRUN -> [SKIP][35] ([i915#3708] / [i915#4077]) +1 other test skip
   [35]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_10926/bat-dg2-11/igt@prime_vgem@basic-fence-mmap.html

  * igt@prime_vgem@basic-read:
    - bat-dg2-11:         NOTRUN -> [SKIP][36] ([i915#3291] / [i915#3708]) +2 other tests skip
   [36]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_10926/bat-dg2-11/igt@prime_vgem@basic-read.html

  * igt@runner@aborted:
    - fi-kbl-8809g:       NOTRUN -> [FAIL][37] ([i915#4991])
   [37]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_10926/fi-kbl-8809g/igt@runner@aborted.html

  
#### Possible fixes ####

  * igt@i915_selftest@live@ring_submission:
    - bat-dg2-14:         [ABORT][38] ([i915#10366]) -> [PASS][39]
   [38]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_14489/bat-dg2-14/igt@i915_selftest@live@ring_submission.html
   [39]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_10926/bat-dg2-14/igt@i915_selftest@live@ring_submission.html

  
  [i915#10021]: https://gitlab.freedesktop.org/drm/intel/issues/10021
  [i915#10366]: https://gitlab.freedesktop.org/drm/intel/issues/10366
  [i915#1072]: https://gitlab.freedesktop.org/drm/intel/issues/1072
  [i915#3291]: https://gitlab.freedesktop.org/drm/intel/issues/3291
  [i915#3555]: https://gitlab.freedesktop.org/drm/intel/issues/3555
  [i915#3708]: https://gitlab.freedesktop.org/drm/intel/issues/3708
  [i915#3840]: https://gitlab.freedesktop.org/drm/intel/issues/3840
  [i915#4077]: https://gitlab.freedesktop.org/drm/intel/issues/4077
  [i915#4079]: https://gitlab.freedesktop.org/drm/intel/issues/4079
  [i915#4083]: https://gitlab.freedesktop.org/drm/intel/issues/4083
  [i915#4103]: https://gitlab.freedesktop.org/drm/intel/issues/4103
  [i915#4212]: https://gitlab.freedesktop.org/drm/intel/issues/4212
  [i915#4213]: https://gitlab.freedesktop.org/drm/intel/issues/4213
  [i915#4215]: https://gitlab.freedesktop.org/drm/intel/issues/4215
  [i915#433]: https://gitlab.freedesktop.org/drm/intel/issues/433
  [i915#4991]: https://gitlab.freedesktop.org/drm/intel/issues/4991
  [i915#5190]: https://gitlab.freedesktop.org/drm/intel/issues/5190
  [i915#5274]: https://gitlab.freedesktop.org/drm/intel/issues/5274
  [i915#5354]: https://gitlab.freedesktop.org/drm/intel/issues/5354
  [i915#6621]: https://gitlab.freedesktop.org/drm/intel/issues/6621
  [i915#9732]: https://gitlab.freedesktop.org/drm/intel/issues/9732


Build changes
-------------

  * CI: CI-20190529 -> None
  * IGT: IGT_7785 -> IGTPW_10926

  CI-20190529: 20190529
  CI_DRM_14489: f9c56f1a03b5c35488671e4ffe61e28b12ffe163 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGTPW_10926: 10926
  IGT_7785: 7785

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_10926/index.html

[-- Attachment #2: Type: text/html, Size: 13771 bytes --]

Re: [PATCH i-g-t v4] Fix memory access issue due to variable block scope

[Kamil Konieczny]

Hi Peter,
On 2024-03-27 at 07:59:27 +0100, Peter Senna Tschudin wrote:

One nit about subject, as you can see on patchwork
https://patchwork.freedesktop.org/project/igt/series/?ordering=-last_updated

we use test name as prefix for a patch, so instead of:

[PATCH i-g-t v4] Fix memory access issue due to variable block

it should be:

[PATCH i-g-t v4] tests/intel/gem_exec_capture: Fix many-* subtests

> This patch fixes the tests many-4k-incremental and many-4k-zero from
> gem_exec_capture that are currently failing with an invalid file
> descriptor error.
> 
> tests/intel/gem_exec_capture.c
>     many(), userptr(), capture_invisible()
>         find_first_available_engine()
>             for_each_ctx_engine()
>                 for_each_ctx_cfg_engine()
> 
> find_first_available_engine() expects a struct intel_execution_engine2
> *e as parameter. for_each_ctx_cfg_engine() allocates a struct
> intel_engine_data ed inside a for loop and then update e to point to an
> element of ed.
> 
> The problem is that ed has the block scope of the for loop, and trying
> to access its content through e after the for loop has ended creates
> undefined behavior.
> 
> This patch updates e to point to the same content at a different memory
> region, to avoid the block scope of ed.
> 
> v4: Move `saved = configure_hangs(fd, e, ctx->id);` to inside the for
> loop where it is safe to use 'e'. This is because 'e' points to an
> element of a variable that is defined with the block scope of the for
> loop.

Yes, this way it works but I still think it would be better
to use function for it.

> 
> Signed-off-by: Peter Senna Tschudin <me@petersenna.com>
> ---
>  tests/intel/gem_exec_capture.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/tests/intel/gem_exec_capture.c b/tests/intel/gem_exec_capture.c
> index 57b178f3e..f7b10bdd4 100644
> --- a/tests/intel/gem_exec_capture.c
> +++ b/tests/intel/gem_exec_capture.c
> @@ -665,10 +665,12 @@ static bool needs_recoverable_ctx(int fd)
>  		ctx = intel_ctx_create_all_physical(fd); \
>  		igt_assert(ctx); \
>  		for_each_ctx_engine(fd, ctx, e) \
> -			for_each_if(gem_class_can_store_dword(fd, e->class)) \
> +			for_each_if(gem_class_can_store_dword(fd, e->class)) { \
> +				saved = configure_hangs(fd, e, ctx->id); \

Yes, it is a fix for a problem but something strange is
happening here, imho we need to debug it more.

>  				break; \
>               } \
>               e = &saved.engine; \
>       		igt_assert(e); \

Two above lines are not needed, 'e' will always be != NULL.

Regards,
Kamil

> -		saved = configure_hangs(fd, e, ctx->id); \
>  	} while(0)
>  
>  static void many(int fd, int dir, uint64_t size, unsigned int flags)
> -- 
> 2.44.0
> 

Re: [PATCH i-g-t v4] Fix memory access issue due to variable block scope

[Peter Senna Tschudin]

On Wed, Mar 27, 2024 at 7:43 PM Kamil Konieczny
<kamil.konieczny@linux.intel.com> wrote:
>
> Hi Peter,
> On 2024-03-27 at 07:59:27 +0100, Peter Senna Tschudin wrote:
>
> One nit about subject, as you can see on patchwork
> https://patchwork.freedesktop.org/project/igt/series/?ordering=-last_updated
>
> we use test name as prefix for a patch, so instead of:
>
> [PATCH i-g-t v4] Fix memory access issue due to variable block
>
> it should be:
>
> [PATCH i-g-t v4] tests/intel/gem_exec_capture: Fix many-* subtests

Oops, my bad. Thank you for pointing this out, I will fix it.

>
> > This patch fixes the tests many-4k-incremental and many-4k-zero from
> > gem_exec_capture that are currently failing with an invalid file
> > descriptor error.
> >
> > tests/intel/gem_exec_capture.c
> >     many(), userptr(), capture_invisible()
> >         find_first_available_engine()
> >             for_each_ctx_engine()
> >                 for_each_ctx_cfg_engine()
> >
> > find_first_available_engine() expects a struct intel_execution_engine2
> > *e as parameter. for_each_ctx_cfg_engine() allocates a struct
> > intel_engine_data ed inside a for loop and then update e to point to an
> > element of ed.
> >
> > The problem is that ed has the block scope of the for loop, and trying
> > to access its content through e after the for loop has ended creates
> > undefined behavior.
> >
> > This patch updates e to point to the same content at a different memory
> > region, to avoid the block scope of ed.
> >
> > v4: Move `saved = configure_hangs(fd, e, ctx->id);` to inside the for
> > loop where it is safe to use 'e'. This is because 'e' points to an
> > element of a variable that is defined with the block scope of the for
> > loop.
>
> Yes, this way it works but I still think it would be better
> to use function for it.

I have been there and I failed to find what better means. The core of
the problem is assigning a component from ed to e. In the macro it
does not work because ed has the scope of the for loop. Using a
function simply changes the scope of ed from the for loop to the
function and the same problem happens. Solutions include:
 - Adding `e = &saved.engine;` at the end of the function. This
updates e to an address without scope limitations. (saved is returned
by the function)
 - Making ed static: Elegant but will break the code if there is ever
concurrency
 - Removing e from the function arguments. This is ok, but then after
calling the function we will need to add `e = &saved.engine;`

So as the best solution seems to be adding `e = &saved.engine;` at the
end of the function, doing that at the existing macro felt like a
better approach.

>
> >
> > Signed-off-by: Peter Senna Tschudin <me@petersenna.com>
> > ---
> >  tests/intel/gem_exec_capture.c | 6 ++++--
> >  1 file changed, 4 insertions(+), 2 deletions(-)
> >
> > diff --git a/tests/intel/gem_exec_capture.c b/tests/intel/gem_exec_capture.c
> > index 57b178f3e..f7b10bdd4 100644
> > --- a/tests/intel/gem_exec_capture.c
> > +++ b/tests/intel/gem_exec_capture.c
> > @@ -665,10 +665,12 @@ static bool needs_recoverable_ctx(int fd)
> >               ctx = intel_ctx_create_all_physical(fd); \
> >               igt_assert(ctx); \
> >               for_each_ctx_engine(fd, ctx, e) \
> > -                     for_each_if(gem_class_can_store_dword(fd, e->class)) \
> > +                     for_each_if(gem_class_can_store_dword(fd, e->class)) { \
> > +                             saved = configure_hangs(fd, e, ctx->id); \
>
> Yes, it is a fix for a problem but something strange is
> happening here, imho we need to debug it more.
>
> >                               break; \
> >               } \
> >               e = &saved.engine; \
> >                       igt_assert(e); \
>
> Two above lines are not needed, 'e' will always be != NULL.

The  `igt_assert(e);` was already there and I kept it. The other line
is the actual fix for the problem. `e = &saved.engine;` updates e to
point to the same content but to an address that has no scope
limitation. There is no scope restrictions because saved is returned
by the function.

[...]

Re: [PATCH i-g-t v4] Fix memory access issue due to variable block scope

[Peter Senna Tschudin]

On Thu, Mar 28, 2024 at 7:55 AM Peter Senna Tschudin <me@petersenna.com> wrote:
>
> On Wed, Mar 27, 2024 at 7:43 PM Kamil Konieczny
> <kamil.konieczny@linux.intel.com> wrote:
> >
> > Hi Peter,
> > On 2024-03-27 at 07:59:27 +0100, Peter Senna Tschudin wrote:
> >
> > One nit about subject, as you can see on patchwork
> > https://patchwork.freedesktop.org/project/igt/series/?ordering=-last_updated
> >
> > we use test name as prefix for a patch, so instead of:
> >
> > [PATCH i-g-t v4] Fix memory access issue due to variable block
> >
> > it should be:
> >
> > [PATCH i-g-t v4] tests/intel/gem_exec_capture: Fix many-* subtests
>
> Oops, my bad. Thank you for pointing this out, I will fix it.
>
> >
> > > This patch fixes the tests many-4k-incremental and many-4k-zero from
> > > gem_exec_capture that are currently failing with an invalid file
> > > descriptor error.
> > >
> > > tests/intel/gem_exec_capture.c
> > >     many(), userptr(), capture_invisible()
> > >         find_first_available_engine()
> > >             for_each_ctx_engine()
> > >                 for_each_ctx_cfg_engine()
> > >
> > > find_first_available_engine() expects a struct intel_execution_engine2
> > > *e as parameter. for_each_ctx_cfg_engine() allocates a struct
> > > intel_engine_data ed inside a for loop and then update e to point to an
> > > element of ed.
> > >
> > > The problem is that ed has the block scope of the for loop, and trying
> > > to access its content through e after the for loop has ended creates
> > > undefined behavior.
> > >
> > > This patch updates e to point to the same content at a different memory
> > > region, to avoid the block scope of ed.
> > >
> > > v4: Move `saved = configure_hangs(fd, e, ctx->id);` to inside the for
> > > loop where it is safe to use 'e'. This is because 'e' points to an
> > > element of a variable that is defined with the block scope of the for
> > > loop.
> >
> > Yes, this way it works but I still think it would be better
> > to use function for it.
>
> I have been there and I failed to find what better means. The core of
> the problem is assigning a component from ed to e. In the macro it
> does not work because ed has the scope of the for loop. Using a
> function simply changes the scope of ed from the for loop to the
> function and the same problem happens. Solutions include:
>  - Adding `e = &saved.engine;` at the end of the function. This
> updates e to an address without scope limitations. (saved is returned
> by the function)
>  - Making ed static: Elegant but will break the code if there is ever
> concurrency
>  - Removing e from the function arguments. This is ok, but then after
> calling the function we will need to add `e = &saved.engine;`
>
> So as the best solution seems to be adding `e = &saved.engine;` at the
> end of the function, doing that at the existing macro felt like a
> better approach.
>
> >
> > >
> > > Signed-off-by: Peter Senna Tschudin <me@petersenna.com>
> > > ---
> > >  tests/intel/gem_exec_capture.c | 6 ++++--
> > >  1 file changed, 4 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/tests/intel/gem_exec_capture.c b/tests/intel/gem_exec_capture.c
> > > index 57b178f3e..f7b10bdd4 100644
> > > --- a/tests/intel/gem_exec_capture.c
> > > +++ b/tests/intel/gem_exec_capture.c
> > > @@ -665,10 +665,12 @@ static bool needs_recoverable_ctx(int fd)
> > >               ctx = intel_ctx_create_all_physical(fd); \
> > >               igt_assert(ctx); \
> > >               for_each_ctx_engine(fd, ctx, e) \
> > > -                     for_each_if(gem_class_can_store_dword(fd, e->class)) \
> > > +                     for_each_if(gem_class_can_store_dword(fd, e->class)) { \
> > > +                             saved = configure_hangs(fd, e, ctx->id); \
> >
> > Yes, it is a fix for a problem but something strange is
> > happening here, imho we need to debug it more.
> >
> > >                               break; \
> > >               } \
> > >               e = &saved.engine; \
> > >                       igt_assert(e); \
> >
> > Two above lines are not needed, 'e' will always be != NULL.
>
> The  `igt_assert(e);` was already there and I kept it. The other line
> is the actual fix for the problem. `e = &saved.engine;` updates e to
> point to the same content but to an address that has no scope
> limitation. There is no scope restrictions because saved is returned
> by the function.

Oops, there is no scope restrictions because 'saved' and 'e' are both
defined in many(), the function that calls
find_first_available_engine(). The change I am proposing is safe
because 'saved' and 'e' have the same variable scope: the function
many(). Before my change, 'e' had the broad scope of many() while it
was pointing to 'ed' which had the very limited scope of the
for_each_ctx_engine() macro.

✗ GitLab.Pipeline: warning for Fix memory access issue due to variable block scope (rev5)

[Patchwork]

== Series Details ==

Series: Fix memory access issue due to variable block scope (rev5)
URL   : https://patchwork.freedesktop.org/series/131602/
State : warning

== Summary ==

Pipeline status: FAILED.

see https://gitlab.freedesktop.org/gfx-ci/igt-ci-tags/-/pipelines/1139607 for the overview.

build-containers:build-debian-arm64 has failed (https://gitlab.freedesktop.org/gfx-ci/igt-ci-tags/-/jobs/56900405):
  Checking out dec96244 as detached HEAD (ref is intel/IGTPW_10939)...
  Removing build/
  Removing installdir/
  
  Skipping Git submodules setup
  section_end:1711606903:get_sources
  section_start:1711606903:step_script
  Executing "step_script" stage of the job script
  Using docker image sha256:594aa868d31ee3304dee8cae8a3433c89a6fcfcf6c7d420c04cce22f60147176 for registry.freedesktop.org/wayland/ci-templates/buildah:2019-08-13.0 with digest registry.freedesktop.org/wayland/ci-templates/buildah@sha256:7dbcf22cd2c1c7d49db0dc7b4ab207c3d6a4a09bd81cc3b71a688d3727d8749f ...
  $ /host/bin/curl -s -L --cacert /host/ca-certificates.crt --retry 4 -f --retry-delay 60 https://gitlab.freedesktop.org/freedesktop/helm-gitlab-infra/-/raw/main/runner-gating/runner-gating.sh  | sh
  Checking if the user of the pipeline is allowed...
  Checking if the job's project is part of a well-known group...
  Thank you for contributing to freedesktop.org
  $ podman login -u gitlab-ci-token -p $CI_JOB_TOKEN $CI_REGISTRY
  section_end:1711606906:step_script
  section_start:1711606906:cleanup_file_variables
  Cleaning up project directory and file based variables
  section_end:1711606907:cleanup_file_variables
  ERROR: Job failed: exit code 137
  

build-containers:build-debian-armhf has failed (https://gitlab.freedesktop.org/gfx-ci/igt-ci-tags/-/jobs/56900404):
  Checking if the user of the pipeline is allowed...
  Checking if the job's project is part of a well-known group...
  Thank you for contributing to freedesktop.org
  Fetching changes...
  Reinitialized existing Git repository in /builds/gfx-ci/igt-ci-tags/.git/
  Checking out dec96244 as detached HEAD (ref is intel/IGTPW_10939)...
  Removing build/
  Removing installdir/
  Removing undocumented_tests.txt
  
  Skipping Git submodules setup
  section_end:1711606906:get_sources
  section_start:1711606906:step_script
  Executing "step_script" stage of the job script
  Using docker image sha256:594aa868d31ee3304dee8cae8a3433c89a6fcfcf6c7d420c04cce22f60147176 for registry.freedesktop.org/wayland/ci-templates/buildah:2019-08-13.0 with digest registry.freedesktop.org/wayland/ci-templates/buildah@sha256:7dbcf22cd2c1c7d49db0dc7b4ab207c3d6a4a09bd81cc3b71a688d3727d8749f ...
  section_end:1711606906:step_script
  section_start:1711606906:cleanup_file_variables
  Cleaning up project directory and file based variables
  section_end:1711606908:cleanup_file_variables
  ERROR: Job failed (system failure): Error response from daemon: no such image: docker.io/library/sha256:594aa868d31ee3304dee8cae8a3433c89a6fcfcf6c7d420c04cce22f60147176: image not known (docker.go:570:0s)

build-containers:build-debian-mips has failed (https://gitlab.freedesktop.org/gfx-ci/igt-ci-tags/-/jobs/56900406):
  Checking if the user of the pipeline is allowed...
  Checking if the job's project is part of a well-known group...
  Thank you for contributing to freedesktop.org
  Fetching changes...
  Reinitialized existing Git repository in /builds/gfx-ci/igt-ci-tags/.git/
  Checking out dec96244 as detached HEAD (ref is intel/IGTPW_10939)...
  Removing build/
  Removing installdir/
  Removing undocumented_tests.txt
  
  Skipping Git submodules setup
  section_end:1711606906:get_sources
  section_start:1711606906:step_script
  Executing "step_script" stage of the job script
  Using docker image sha256:594aa868d31ee3304dee8cae8a3433c89a6fcfcf6c7d420c04cce22f60147176 for registry.freedesktop.org/wayland/ci-templates/buildah:2019-08-13.0 with digest registry.freedesktop.org/wayland/ci-templates/buildah@sha256:7dbcf22cd2c1c7d49db0dc7b4ab207c3d6a4a09bd81cc3b71a688d3727d8749f ...
  section_end:1711606906:step_script
  section_start:1711606906:cleanup_file_variables
  Cleaning up project directory and file based variables
  section_end:1711606907:cleanup_file_variables
  ERROR: Job failed (system failure): Error response from daemon: no such image: docker.io/library/sha256:594aa868d31ee3304dee8cae8a3433c89a6fcfcf6c7d420c04cce22f60147176: image not known (docker.go:570:0s)

== Logs ==

For more details see: https://gitlab.freedesktop.org/gfx-ci/igt-ci-tags/-/pipelines/1139607

✓ CI.xeBAT: success for Fix memory access issue due to variable block scope (rev5)

[Patchwork]

[-- Attachment #1: Type: text/plain, Size: 3975 bytes --]

== Series Details ==

Series: Fix memory access issue due to variable block scope (rev5)
URL   : https://patchwork.freedesktop.org/series/131602/
State : success

== Summary ==

CI Bug Log - changes from XEIGT_7786_BAT -> XEIGTPW_10939_BAT
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

  

Participating hosts (5 -> 4)
------------------------------

  Missing    (1): bat-lnl-1 

Known issues
------------

  Here are the changes found in XEIGTPW_10939_BAT that come from known issues:

### IGT changes ###

#### Possible fixes ####

  * igt@xe_evict@evict-beng-small-external:
    - bat-dg2-oem2:       [SKIP][1] ([Intel XE#1130]) -> [PASS][2] +13 other tests pass
   [1]: https://intel-gfx-ci.01.org/tree/intel-xe/IGT_7786/bat-dg2-oem2/igt@xe_evict@evict-beng-small-external.html
   [2]: https://intel-gfx-ci.01.org/tree/intel-xe/IGTPW_10939/bat-dg2-oem2/igt@xe_evict@evict-beng-small-external.html

  * igt@xe_module_load@load:
    - bat-dg2-oem2:       [FAIL][3] ([Intel XE#1132]) -> [PASS][4]
   [3]: https://intel-gfx-ci.01.org/tree/intel-xe/IGT_7786/bat-dg2-oem2/igt@xe_module_load@load.html
   [4]: https://intel-gfx-ci.01.org/tree/intel-xe/IGTPW_10939/bat-dg2-oem2/igt@xe_module_load@load.html

  
#### Warnings ####

  * igt@xe_evict@evict-beng-mixed-threads-small-multi-vm:
    - bat-dg2-oem2:       [SKIP][5] ([Intel XE#1130]) -> [TIMEOUT][6] ([Intel XE#1027] / [Intel XE#1088])
   [5]: https://intel-gfx-ci.01.org/tree/intel-xe/IGT_7786/bat-dg2-oem2/igt@xe_evict@evict-beng-mixed-threads-small-multi-vm.html
   [6]: https://intel-gfx-ci.01.org/tree/intel-xe/IGTPW_10939/bat-dg2-oem2/igt@xe_evict@evict-beng-mixed-threads-small-multi-vm.html

  * igt@xe_evict@evict-beng-small:
    - bat-dg2-oem2:       [SKIP][7] ([Intel XE#1130]) -> [DMESG-WARN][8] ([Intel XE#1088] / [Intel XE#1373])
   [7]: https://intel-gfx-ci.01.org/tree/intel-xe/IGT_7786/bat-dg2-oem2/igt@xe_evict@evict-beng-small.html
   [8]: https://intel-gfx-ci.01.org/tree/intel-xe/IGTPW_10939/bat-dg2-oem2/igt@xe_evict@evict-beng-small.html

  * igt@xe_evict@evict-cm-threads-small:
    - bat-dg2-oem2:       [SKIP][9] ([Intel XE#1130]) -> [INCOMPLETE][10] ([Intel XE#1027] / [Intel XE#804])
   [9]: https://intel-gfx-ci.01.org/tree/intel-xe/IGT_7786/bat-dg2-oem2/igt@xe_evict@evict-cm-threads-small.html
   [10]: https://intel-gfx-ci.01.org/tree/intel-xe/IGTPW_10939/bat-dg2-oem2/igt@xe_evict@evict-cm-threads-small.html

  * igt@xe_evict@evict-mixed-threads-small:
    - bat-dg2-oem2:       [SKIP][11] ([Intel XE#1130]) -> [INCOMPLETE][12] ([Intel XE#804])
   [11]: https://intel-gfx-ci.01.org/tree/intel-xe/IGT_7786/bat-dg2-oem2/igt@xe_evict@evict-mixed-threads-small.html
   [12]: https://intel-gfx-ci.01.org/tree/intel-xe/IGTPW_10939/bat-dg2-oem2/igt@xe_evict@evict-mixed-threads-small.html

  
  [Intel XE#1027]: https://gitlab.freedesktop.org/drm/xe/kernel/issues/1027
  [Intel XE#1088]: https://gitlab.freedesktop.org/drm/xe/kernel/issues/1088
  [Intel XE#1130]: https://gitlab.freedesktop.org/drm/xe/kernel/issues/1130
  [Intel XE#1132]: https://gitlab.freedesktop.org/drm/xe/kernel/issues/1132
  [Intel XE#1373]: https://gitlab.freedesktop.org/drm/xe/kernel/issues/1373
  [Intel XE#804]: https://gitlab.freedesktop.org/drm/xe/kernel/issues/804


Build changes
-------------

  * IGT: IGT_7786 -> IGTPW_10939
  * Linux: xe-998-f9c56f1a03b5c35488671e4ffe61e28b12ffe163 -> xe-1004-07c774152cf8a034784b40978a77b5ee66e4779b

  IGTPW_10939: dec9624472a3668b006dacb3c1ae45dca37954d1 @ https://gitlab.freedesktop.org/drm/igt-gpu-tools.git
  IGT_7786: 1e4a3cd0a4bb3419fb70ed3e01259485b056dcfd @ https://gitlab.freedesktop.org/drm/igt-gpu-tools.git
  xe-1004-07c774152cf8a034784b40978a77b5ee66e4779b: 07c774152cf8a034784b40978a77b5ee66e4779b
  xe-998-f9c56f1a03b5c35488671e4ffe61e28b12ffe163: f9c56f1a03b5c35488671e4ffe61e28b12ffe163

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/intel-xe/IGTPW_10939/index.html

[-- Attachment #2: Type: text/html, Size: 5168 bytes --]

[PATCH i-g-t v5] tests/intel/gem_exec_capture: Fix many-* subtests

[Peter Senna Tschudin]

This patch fixes the tests many-4k-incremental and many-4k-zero from
gem_exec_capture that are currently failing with an invalid file
descriptor error.

tests/intel/gem_exec_capture.c
    many(), userptr(), capture_invisible()
        find_first_available_engine()
            for_each_ctx_engine()
                for_each_ctx_cfg_engine()

find_first_available_engine() expects a struct intel_execution_engine2
*e as parameter. for_each_ctx_cfg_engine() allocates a struct
intel_engine_data ed inside a for loop and then update e to point to an
element of ed.

The problem is that ed has the block scope of the for loop, and trying
to access its content through e after the for loop has ended creates
undefined behavior.

This patch updates e to point to the same content at a different memory
region, to avoid the block scope of ed. `e = &saved.engine;` updates e
to point to the same content but without scope restrictions as both
saved and e have the scope of the calling function.

v5: Update subject and commit message.

Signed-off-by: Peter Senna Tschudin <me@petersenna.com>
---
 tests/intel/gem_exec_capture.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/tests/intel/gem_exec_capture.c b/tests/intel/gem_exec_capture.c
index 57b178f3e..f7b10bdd4 100644
--- a/tests/intel/gem_exec_capture.c
+++ b/tests/intel/gem_exec_capture.c
@@ -665,10 +665,12 @@ static bool needs_recoverable_ctx(int fd)
 		ctx = intel_ctx_create_all_physical(fd); \
 		igt_assert(ctx); \
 		for_each_ctx_engine(fd, ctx, e) \
-			for_each_if(gem_class_can_store_dword(fd, e->class)) \
+			for_each_if(gem_class_can_store_dword(fd, e->class)) { \
+				saved = configure_hangs(fd, e, ctx->id); \
 				break; \
+			}  \
+		e = &saved.engine; \
 		igt_assert(e); \
-		saved = configure_hangs(fd, e, ctx->id); \
 	} while(0)
 
 static void many(int fd, int dir, uint64_t size, unsigned int flags)
-- 
2.44.0

Re: [PATCH i-g-t v5] tests/intel/gem_exec_capture: Fix many-* subtests

[Kamil Konieczny]

Hi Peter,
On 2024-03-28 at 13:42:51 +0100, Peter Senna Tschudin wrote:
> This patch fixes the tests many-4k-incremental and many-4k-zero from
> gem_exec_capture that are currently failing with an invalid file
> descriptor error.
> 
> tests/intel/gem_exec_capture.c
>     many(), userptr(), capture_invisible()

Please cite error log, delete two above lines and write that problem
is with macro find_first_available_engine() (you later describe in datails).

>         find_first_available_engine()
>             for_each_ctx_engine()
>                 for_each_ctx_cfg_engine()
> 
> find_first_available_engine() expects a struct intel_execution_engine2
> *e as parameter. for_each_ctx_cfg_engine() allocates a struct
> intel_engine_data ed inside a for loop and then update e to point to an
> element of ed.
> 
> The problem is that ed has the block scope of the for loop, and trying
> to access its content through e after the for loop has ended creates
> undefined behavior.
> 
> This patch updates e to point to the same content at a different memory
> region, to avoid the block scope of ed. `e = &saved.engine;` updates e
> to point to the same content but without scope restrictions as both
> saved and e have the scope of the calling function.
> 
> v5: Update subject and commit message.
> 
> Signed-off-by: Peter Senna Tschudin <me@petersenna.com>
> ---
>  tests/intel/gem_exec_capture.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/tests/intel/gem_exec_capture.c b/tests/intel/gem_exec_capture.c
> index 57b178f3e..f7b10bdd4 100644
> --- a/tests/intel/gem_exec_capture.c
> +++ b/tests/intel/gem_exec_capture.c
> @@ -665,10 +665,12 @@ static bool needs_recoverable_ctx(int fd)
>  		ctx = intel_ctx_create_all_physical(fd); \
>  		igt_assert(ctx); \
>  		for_each_ctx_engine(fd, ctx, e) \
> -			for_each_if(gem_class_can_store_dword(fd, e->class)) \
> +			for_each_if(gem_class_can_store_dword(fd, e->class)) { \
> +				saved = configure_hangs(fd, e, ctx->id); \
>  				break; \
> +			}  \
> +		e = &saved.engine; \

Ok, we need 'e' later on, so it is ok.

>  		igt_assert(e); \

But this assert do no work, it will always succeed. Imho here
code should check if it actually saved anything, so what about:

#define find_first_available_engine(fd, ctx, e, saved) \
    do { \
        bool no_dw_store_engine = true; \
        \
        ctx = intel_ctx_create_all_physical(fd); \
        igt_assert(ctx); \
        for_each_ctx_engine(fd, ctx, e) \
            for_each_if(gem_class_can_store_dword(fd, e->class)) { \
                saved = configure_hangs(fd, e, ctx->id); \
                no_dw_store_engine = false; \
                break; \
            }  \
        e = &saved.engine; \
        igt_skip_on(no_dw_store_engine); \
    } while(0)


Regards,
Kamil

> -		saved = configure_hangs(fd, e, ctx->id); \
>  	} while(0)
>  
>  static void many(int fd, int dir, uint64_t size, unsigned int flags)
> -- 
> 2.44.0
> 

Re: [PATCH i-g-t v5] tests/intel/gem_exec_capture: Fix many-* subtests

[Peter Senna Tschudin]

Hi Kamil,

Thanks again!

On Thu, Mar 28, 2024 at 4:49 PM Kamil Konieczny
<kamil.konieczny@linux.intel.com> wrote:
>
> Hi Peter,
> On 2024-03-28 at 13:42:51 +0100, Peter Senna Tschudin wrote:
> > This patch fixes the tests many-4k-incremental and many-4k-zero from
> > gem_exec_capture that are currently failing with an invalid file
> > descriptor error.
> >
> > tests/intel/gem_exec_capture.c
> >     many(), userptr(), capture_invisible()
>
> Please cite error log, delete two above lines and write that problem
> is with macro find_first_available_engine() (you later describe in datails).
>
> >         find_first_available_engine()
> >             for_each_ctx_engine()
> >                 for_each_ctx_cfg_engine()
> >
> > find_first_available_engine() expects a struct intel_execution_engine2
> > *e as parameter. for_each_ctx_cfg_engine() allocates a struct
> > intel_engine_data ed inside a for loop and then update e to point to an
> > element of ed.
> >
> > The problem is that ed has the block scope of the for loop, and trying
> > to access its content through e after the for loop has ended creates
> > undefined behavior.
> >
> > This patch updates e to point to the same content at a different memory
> > region, to avoid the block scope of ed. `e = &saved.engine;` updates e
> > to point to the same content but without scope restrictions as both
> > saved and e have the scope of the calling function.
> >
> > v5: Update subject and commit message.
> >
> > Signed-off-by: Peter Senna Tschudin <me@petersenna.com>
> > ---
> >  tests/intel/gem_exec_capture.c | 6 ++++--
> >  1 file changed, 4 insertions(+), 2 deletions(-)
> >
> > diff --git a/tests/intel/gem_exec_capture.c b/tests/intel/gem_exec_capture.c
> > index 57b178f3e..f7b10bdd4 100644
> > --- a/tests/intel/gem_exec_capture.c
> > +++ b/tests/intel/gem_exec_capture.c
> > @@ -665,10 +665,12 @@ static bool needs_recoverable_ctx(int fd)
> >               ctx = intel_ctx_create_all_physical(fd); \
> >               igt_assert(ctx); \
> >               for_each_ctx_engine(fd, ctx, e) \
> > -                     for_each_if(gem_class_can_store_dword(fd, e->class)) \
> > +                     for_each_if(gem_class_can_store_dword(fd, e->class)) { \
> > +                             saved = configure_hangs(fd, e, ctx->id); \
> >                               break; \
> > +                     }  \
> > +             e = &saved.engine; \
>
> Ok, we need 'e' later on, so it is ok.
>
> >               igt_assert(e); \
>
> But this assert do no work, it will always succeed. Imho here
> code should check if it actually saved anything, so what about:

That was my bad, sorry. igt_assert(e) should be before the call to
configure_hangs() because 'e' is set by intel_get_current_engine()
which can return null.

>
> #define find_first_available_engine(fd, ctx, e, saved) \
>     do { \
>         bool no_dw_store_engine = true; \
>         \
>         ctx = intel_ctx_create_all_physical(fd); \
>         igt_assert(ctx); \
>         for_each_ctx_engine(fd, ctx, e) \
>             for_each_if(gem_class_can_store_dword(fd, e->class)) { \
>                 saved = configure_hangs(fd, e, ctx->id); \
>                 no_dw_store_engine = false; \
>                 break; \
>             }  \
>         e = &saved.engine; \
>         igt_skip_on(no_dw_store_engine); \
>     } while(0)

Yes! I sent a slightly different version skipping the test when
gem_class_can_store_dword() returns false. I sent a second patch for
this, in the hopes that having two patches is the "correct" way.

[...]

✗ Fi.CI.BAT: failure for Fix memory access issue due to variable block scope (rev6)

[Patchwork]

[-- Attachment #1: Type: text/plain, Size: 12643 bytes --]

== Series Details ==

Series: Fix memory access issue due to variable block scope (rev6)
URL   : https://patchwork.freedesktop.org/series/131602/
State : failure

== Summary ==

CI Bug Log - changes from CI_DRM_14502 -> IGTPW_10942
====================================================

Summary
-------

  **FAILURE**

  Serious unknown changes coming with IGTPW_10942 absolutely need to be
  verified manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in IGTPW_10942, please notify your bug team (I915-ci-infra@lists.freedesktop.org) to allow them
  to document this new failure mode, which will reduce false positives in CI.

  External URL: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_10942/index.html

Participating hosts (37 -> 33)
------------------------------

  Additional (2): bat-arls-1 bat-mtlp-8 
  Missing    (6): bat-kbl-2 fi-snb-2520m bat-atsm-1 fi-kbl-8809g fi-bsw-nick bat-arls-3 

Possible new issues
-------------------

  Here are the unknown changes that may have been introduced in IGTPW_10942:

### IGT changes ###

#### Possible regressions ####

  * igt@i915_selftest@live@migrate:
    - bat-arls-1:         NOTRUN -> [DMESG-WARN][1]
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_10942/bat-arls-1/igt@i915_selftest@live@migrate.html

  
Known issues
------------

  Here are the changes found in IGTPW_10942 that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@debugfs_test@basic-hwmon:
    - bat-mtlp-8:         NOTRUN -> [SKIP][2] ([i915#9318])
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_10942/bat-mtlp-8/igt@debugfs_test@basic-hwmon.html
    - bat-arls-1:         NOTRUN -> [SKIP][3] ([i915#9318])
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_10942/bat-arls-1/igt@debugfs_test@basic-hwmon.html

  * igt@gem_lmem_swapping@random-engines:
    - bat-arls-1:         NOTRUN -> [SKIP][4] ([i915#10213]) +3 other tests skip
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_10942/bat-arls-1/igt@gem_lmem_swapping@random-engines.html

  * igt@gem_lmem_swapping@verify-random:
    - bat-mtlp-8:         NOTRUN -> [SKIP][5] ([i915#4613]) +3 other tests skip
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_10942/bat-mtlp-8/igt@gem_lmem_swapping@verify-random.html
    - bat-dg2-8:          NOTRUN -> [SKIP][6] ([i915#9643])
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_10942/bat-dg2-8/igt@gem_lmem_swapping@verify-random.html

  * igt@gem_mmap@basic:
    - bat-mtlp-8:         NOTRUN -> [SKIP][7] ([i915#4083])
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_10942/bat-mtlp-8/igt@gem_mmap@basic.html
    - bat-arls-1:         NOTRUN -> [SKIP][8] ([i915#4083])
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_10942/bat-arls-1/igt@gem_mmap@basic.html

  * igt@gem_mmap_gtt@basic:
    - bat-mtlp-8:         NOTRUN -> [SKIP][9] ([i915#4077]) +2 other tests skip
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_10942/bat-mtlp-8/igt@gem_mmap_gtt@basic.html

  * igt@gem_render_tiled_blits@basic:
    - bat-mtlp-8:         NOTRUN -> [SKIP][10] ([i915#4079]) +1 other test skip
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_10942/bat-mtlp-8/igt@gem_render_tiled_blits@basic.html
    - bat-arls-1:         NOTRUN -> [SKIP][11] ([i915#10197] / [i915#10211] / [i915#4079])
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_10942/bat-arls-1/igt@gem_render_tiled_blits@basic.html

  * igt@gem_tiled_blits@basic:
    - bat-arls-1:         NOTRUN -> [SKIP][12] ([i915#10196] / [i915#4077]) +2 other tests skip
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_10942/bat-arls-1/igt@gem_tiled_blits@basic.html

  * igt@gem_tiled_pread_basic:
    - bat-arls-1:         NOTRUN -> [SKIP][13] ([i915#10206] / [i915#4079])
   [13]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_10942/bat-arls-1/igt@gem_tiled_pread_basic.html

  * igt@i915_pm_rps@basic-api:
    - bat-mtlp-8:         NOTRUN -> [SKIP][14] ([i915#6621])
   [14]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_10942/bat-mtlp-8/igt@i915_pm_rps@basic-api.html
    - bat-arls-1:         NOTRUN -> [SKIP][15] ([i915#10209])
   [15]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_10942/bat-arls-1/igt@i915_pm_rps@basic-api.html

  * igt@i915_selftest@live@gt_lrc:
    - bat-dg2-9:          [PASS][16] -> [ABORT][17] ([i915#10366])
   [16]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_14502/bat-dg2-9/igt@i915_selftest@live@gt_lrc.html
   [17]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_10942/bat-dg2-9/igt@i915_selftest@live@gt_lrc.html

  * igt@i915_selftest@live@objects:
    - bat-arls-1:         NOTRUN -> [DMESG-FAIL][18] ([i915#10262]) +23 other tests dmesg-fail
   [18]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_10942/bat-arls-1/igt@i915_selftest@live@objects.html

  * igt@kms_addfb_basic@addfb25-x-tiled-legacy:
    - bat-arls-1:         NOTRUN -> [SKIP][19] ([i915#10200]) +9 other tests skip
   [19]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_10942/bat-arls-1/igt@kms_addfb_basic@addfb25-x-tiled-legacy.html

  * igt@kms_addfb_basic@addfb25-y-tiled-small-legacy:
    - bat-mtlp-8:         NOTRUN -> [SKIP][20] ([i915#5190])
   [20]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_10942/bat-mtlp-8/igt@kms_addfb_basic@addfb25-y-tiled-small-legacy.html

  * igt@kms_addfb_basic@basic-y-tiled-legacy:
    - bat-mtlp-8:         NOTRUN -> [SKIP][21] ([i915#4212]) +8 other tests skip
   [21]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_10942/bat-mtlp-8/igt@kms_addfb_basic@basic-y-tiled-legacy.html

  * igt@kms_cursor_legacy@basic-busy-flip-before-cursor-legacy:
    - bat-mtlp-8:         NOTRUN -> [SKIP][22] ([i915#4213]) +1 other test skip
   [22]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_10942/bat-mtlp-8/igt@kms_cursor_legacy@basic-busy-flip-before-cursor-legacy.html
    - bat-arls-1:         NOTRUN -> [SKIP][23] ([i915#10202]) +1 other test skip
   [23]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_10942/bat-arls-1/igt@kms_cursor_legacy@basic-busy-flip-before-cursor-legacy.html

  * igt@kms_dsc@dsc-basic:
    - bat-mtlp-8:         NOTRUN -> [SKIP][24] ([i915#3555] / [i915#3840] / [i915#9159])
   [24]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_10942/bat-mtlp-8/igt@kms_dsc@dsc-basic.html
    - bat-arls-1:         NOTRUN -> [SKIP][25] ([i915#9886])
   [25]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_10942/bat-arls-1/igt@kms_dsc@dsc-basic.html

  * igt@kms_force_connector_basic@force-load-detect:
    - bat-mtlp-8:         NOTRUN -> [SKIP][26]
   [26]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_10942/bat-mtlp-8/igt@kms_force_connector_basic@force-load-detect.html
    - bat-arls-1:         NOTRUN -> [SKIP][27] ([i915#10207])
   [27]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_10942/bat-arls-1/igt@kms_force_connector_basic@force-load-detect.html

  * igt@kms_force_connector_basic@prune-stale-modes:
    - bat-mtlp-8:         NOTRUN -> [SKIP][28] ([i915#5274])
   [28]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_10942/bat-mtlp-8/igt@kms_force_connector_basic@prune-stale-modes.html

  * igt@kms_pm_backlight@basic-brightness:
    - bat-arls-1:         NOTRUN -> [SKIP][29] ([i915#9812])
   [29]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_10942/bat-arls-1/igt@kms_pm_backlight@basic-brightness.html

  * igt@kms_psr@psr-primary-mmap-gtt:
    - bat-arls-1:         NOTRUN -> [SKIP][30] ([i915#9732]) +3 other tests skip
   [30]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_10942/bat-arls-1/igt@kms_psr@psr-primary-mmap-gtt.html

  * igt@kms_psr@psr-primary-mmap-gtt@edp-1:
    - bat-mtlp-8:         NOTRUN -> [SKIP][31] ([i915#4077] / [i915#9688])
   [31]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_10942/bat-mtlp-8/igt@kms_psr@psr-primary-mmap-gtt@edp-1.html

  * igt@kms_setmode@basic-clone-single-crtc:
    - bat-mtlp-8:         NOTRUN -> [SKIP][32] ([i915#3555] / [i915#8809])
   [32]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_10942/bat-mtlp-8/igt@kms_setmode@basic-clone-single-crtc.html
    - bat-arls-1:         NOTRUN -> [SKIP][33] ([i915#10208] / [i915#8809])
   [33]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_10942/bat-arls-1/igt@kms_setmode@basic-clone-single-crtc.html

  * igt@prime_vgem@basic-fence-mmap:
    - bat-mtlp-8:         NOTRUN -> [SKIP][34] ([i915#3708] / [i915#4077]) +1 other test skip
   [34]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_10942/bat-mtlp-8/igt@prime_vgem@basic-fence-mmap.html

  * igt@prime_vgem@basic-fence-read:
    - bat-mtlp-8:         NOTRUN -> [SKIP][35] ([i915#3708]) +1 other test skip
   [35]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_10942/bat-mtlp-8/igt@prime_vgem@basic-fence-read.html
    - bat-arls-1:         NOTRUN -> [SKIP][36] ([i915#10212] / [i915#3708])
   [36]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_10942/bat-arls-1/igt@prime_vgem@basic-fence-read.html

  * igt@prime_vgem@basic-gtt:
    - bat-arls-1:         NOTRUN -> [SKIP][37] ([i915#10196] / [i915#3708] / [i915#4077]) +1 other test skip
   [37]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_10942/bat-arls-1/igt@prime_vgem@basic-gtt.html

  * igt@prime_vgem@basic-read:
    - bat-arls-1:         NOTRUN -> [SKIP][38] ([i915#10214] / [i915#3708])
   [38]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_10942/bat-arls-1/igt@prime_vgem@basic-read.html

  * igt@prime_vgem@basic-write:
    - bat-mtlp-8:         NOTRUN -> [SKIP][39] ([i915#10216] / [i915#3708])
   [39]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_10942/bat-mtlp-8/igt@prime_vgem@basic-write.html
    - bat-arls-1:         NOTRUN -> [SKIP][40] ([i915#10216] / [i915#3708])
   [40]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_10942/bat-arls-1/igt@prime_vgem@basic-write.html

  
  [i915#10196]: https://gitlab.freedesktop.org/drm/intel/issues/10196
  [i915#10197]: https://gitlab.freedesktop.org/drm/intel/issues/10197
  [i915#10200]: https://gitlab.freedesktop.org/drm/intel/issues/10200
  [i915#10202]: https://gitlab.freedesktop.org/drm/intel/issues/10202
  [i915#10206]: https://gitlab.freedesktop.org/drm/intel/issues/10206
  [i915#10207]: https://gitlab.freedesktop.org/drm/intel/issues/10207
  [i915#10208]: https://gitlab.freedesktop.org/drm/intel/issues/10208
  [i915#10209]: https://gitlab.freedesktop.org/drm/intel/issues/10209
  [i915#10211]: https://gitlab.freedesktop.org/drm/intel/issues/10211
  [i915#10212]: https://gitlab.freedesktop.org/drm/intel/issues/10212
  [i915#10213]: https://gitlab.freedesktop.org/drm/intel/issues/10213
  [i915#10214]: https://gitlab.freedesktop.org/drm/intel/issues/10214
  [i915#10216]: https://gitlab.freedesktop.org/drm/intel/issues/10216
  [i915#10262]: https://gitlab.freedesktop.org/drm/intel/issues/10262
  [i915#10366]: https://gitlab.freedesktop.org/drm/intel/issues/10366
  [i915#3555]: https://gitlab.freedesktop.org/drm/intel/issues/3555
  [i915#3708]: https://gitlab.freedesktop.org/drm/intel/issues/3708
  [i915#3840]: https://gitlab.freedesktop.org/drm/intel/issues/3840
  [i915#4077]: https://gitlab.freedesktop.org/drm/intel/issues/4077
  [i915#4079]: https://gitlab.freedesktop.org/drm/intel/issues/4079
  [i915#4083]: https://gitlab.freedesktop.org/drm/intel/issues/4083
  [i915#4212]: https://gitlab.freedesktop.org/drm/intel/issues/4212
  [i915#4213]: https://gitlab.freedesktop.org/drm/intel/issues/4213
  [i915#4613]: https://gitlab.freedesktop.org/drm/intel/issues/4613
  [i915#5190]: https://gitlab.freedesktop.org/drm/intel/issues/5190
  [i915#5274]: https://gitlab.freedesktop.org/drm/intel/issues/5274
  [i915#6621]: https://gitlab.freedesktop.org/drm/intel/issues/6621
  [i915#8809]: https://gitlab.freedesktop.org/drm/intel/issues/8809
  [i915#9159]: https://gitlab.freedesktop.org/drm/intel/issues/9159
  [i915#9318]: https://gitlab.freedesktop.org/drm/intel/issues/9318
  [i915#9643]: https://gitlab.freedesktop.org/drm/intel/issues/9643
  [i915#9688]: https://gitlab.freedesktop.org/drm/intel/issues/9688
  [i915#9732]: https://gitlab.freedesktop.org/drm/intel/issues/9732
  [i915#9812]: https://gitlab.freedesktop.org/drm/intel/issues/9812
  [i915#9886]: https://gitlab.freedesktop.org/drm/intel/issues/9886


Build changes
-------------

  * CI: CI-20190529 -> None
  * IGT: IGT_7789 -> IGTPW_10942

  CI-20190529: 20190529
  CI_DRM_14502: c185a0400ae0be7c5565e9eae63d5b81f5c242ff @ git://anongit.freedesktop.org/gfx-ci/linux
  IGTPW_10942: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_10942/index.html
  IGT_7789: 28352d6f0f514d38fe02574653878be9c3a3421c @ https://gitlab.freedesktop.org/drm/igt-gpu-tools.git

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_10942/index.html

[-- Attachment #2: Type: text/html, Size: 15161 bytes --]

[PATCH i-g-t v6 1/2] tests/intel/gem_exec_capture: Fix many-* subtests

[Peter Senna Tschudin]

Currently trying to run `gem_exec_capture --run-subtest
many-4K-incremental` or `gem_exec_capture --run-subtest many-4K-zero`
will fail with:

 (gem_exec_capture:81999) i915/gem_engine_topology-CRITICAL: Test
 assertion failure function gem_engine_properties_configure, file
 ../lib/i915/gem_engine_topology.c:577:
 (gem_exec_capture:81999) i915/gem_engine_topology-CRITICAL: Failed assertion: ret == 1
 (gem_exec_capture:81999) i915/gem_engine_topology-CRITICAL: Last errno: 9, Bad file descriptor
 (gem_exec_capture:81999) i915/gem_engine_topology-CRITICAL: error: -1 != 1

This problem happens inside the macro find_first_available_engine()
when:
 1. for_each_ctx_engine() allocates an struct intel_engine_data 'ed'
    inside a for loop. The core of the issue is that ed only exists
    inside the for loop. As soon as the for loop ends, ed is out of scope
    and after it's lifetime.
 2. intel_get_current_engine() sets '*e' to an address of ed. This is ok
    while inside the for loop, and is undefined behavior after the for
    loop ends.
 3. configure_hangs() uses '*e' after the lifetime of 'ed' has ended
    leading to undefined behavior
 4. After the call to find_first_available_engine() __captureN() will
    fail as it expects '*e' to be valid. This is also undefined
    behavior.

This patch fixes the issue in two steps:
 1. Moves the call to configure_hangs() to inside the for loop, where
    '*e' is valid because there are no issues with scope and lifetime of
    'ed'.
 2. Adds `e = &saved.engine;` to the end of
    find_first_available_engine(). The reason why this works is twofold:
    First 'saved' has the same content as e had when there were no
    variable scope and lifetime issues. Second both '*e' and 'saved' are
    defined in many() meaning that they both have the same scope and
    lifetime.

 v6: new commit message; moves igt_assert(e); to before the call to
     configure_hangs(). This check is there because '*e' is set by
     intel_get_current_engine() which will return NULL if ed->n >=
     ed->nengines.

Signed-off-by: Peter Senna Tschudin <me@petersenna.com>
---
 tests/intel/gem_exec_capture.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/tests/intel/gem_exec_capture.c b/tests/intel/gem_exec_capture.c
index 57b178f3e..a8348f21b 100644
--- a/tests/intel/gem_exec_capture.c
+++ b/tests/intel/gem_exec_capture.c
@@ -665,10 +665,12 @@ static bool needs_recoverable_ctx(int fd)
 		ctx = intel_ctx_create_all_physical(fd); \
 		igt_assert(ctx); \
 		for_each_ctx_engine(fd, ctx, e) \
-			for_each_if(gem_class_can_store_dword(fd, e->class)) \
+			for_each_if(gem_class_can_store_dword(fd, e->class)) { \
+				igt_assert(e); \
+				saved = configure_hangs(fd, e, ctx->id); \
 				break; \
-		igt_assert(e); \
-		saved = configure_hangs(fd, e, ctx->id); \
+			} \
+		e = &saved.engine; \
 	} while(0)
 
 static void many(int fd, int dir, uint64_t size, unsigned int flags)
-- 
2.44.0

[PATCH i-g-t v6 2/2] Skip the test when no engines are found

[Peter Senna Tschudin]

This patch calls igt_skip() when no engines are found by
find_first_available_engine() preventing downstream code from crashing.

Signed-off-by: Peter Senna Tschudin <me@petersenna.com>
---
 tests/intel/gem_exec_capture.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/tests/intel/gem_exec_capture.c b/tests/intel/gem_exec_capture.c
index a8348f21b..2afb84283 100644
--- a/tests/intel/gem_exec_capture.c
+++ b/tests/intel/gem_exec_capture.c
@@ -665,10 +665,12 @@ static bool needs_recoverable_ctx(int fd)
 		ctx = intel_ctx_create_all_physical(fd); \
 		igt_assert(ctx); \
 		for_each_ctx_engine(fd, ctx, e) \
-			for_each_if(gem_class_can_store_dword(fd, e->class)) { \
+			if(gem_class_can_store_dword(fd, e->class)) { \
 				igt_assert(e); \
 				saved = configure_hangs(fd, e, ctx->id); \
 				break; \
+			} else { \
+				igt_skip("find_first_available_engine(): No engine available\n"); \
 			} \
 		e = &saved.engine; \
 	} while(0)
-- 
2.44.0

Re: [PATCH i-g-t v6 2/2] Skip the test when no engines are found

[Kamil Konieczny]

Hi Peter,
On 2024-03-30 at 15:19:32 +0100, Peter Senna Tschudin wrote:
> This patch calls igt_skip() when no engines are found by
> find_first_available_engine() preventing downstream code from crashing.

I would prefer to have this change in one patch.

> 
> Signed-off-by: Peter Senna Tschudin <me@petersenna.com>
> ---
>  tests/intel/gem_exec_capture.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/tests/intel/gem_exec_capture.c b/tests/intel/gem_exec_capture.c
> index a8348f21b..2afb84283 100644
> --- a/tests/intel/gem_exec_capture.c
> +++ b/tests/intel/gem_exec_capture.c
> @@ -665,10 +665,12 @@ static bool needs_recoverable_ctx(int fd)
>  		ctx = intel_ctx_create_all_physical(fd); \
>  		igt_assert(ctx); \

If 'e' is causing problems here, maybe introduce new var 'tmpe'?
So you can start with:

        e = NULL;
  		for_each_ctx_engine(fd, ctx, tmpe) \

then use tmpe->engine and after for_each... loop you may add
igt_skip_on_f(e == NULL, "no capable engine found\n");

> -			for_each_if(gem_class_can_store_dword(fd, e->class)) { \
> +			if(gem_class_can_store_dword(fd, e->class)) { \

This looks much better, you replaced confusing macro with if()

>  				igt_assert(e); \
>  				saved = configure_hangs(fd, e, ctx->id); \
>  				break; \
> +			} else { \
> +				igt_skip("find_first_available_engine(): No engine available\n"); \

This do not solve a problem, you may have capable engine later on
as there is for_each... loop.

>  			} \
>  		e = &saved.engine; \

If using 'tmpe' this should be before 'break;' line and here
you should place igt_skip_on_f()

Regards,
Kamil

>  	} while(0)
> -- 
> 2.44.0
> 

Re: [PATCH i-g-t v6 2/2] Skip the test when no engines are found

[Peter Senna Tschudin]

On Tue, Apr 2, 2024 at 6:17 PM Kamil Konieczny
<kamil.konieczny@linux.intel.com> wrote:
>
> Hi Peter,
> On 2024-03-30 at 15:19:32 +0100, Peter Senna Tschudin wrote:
> > This patch calls igt_skip() when no engines are found by
> > find_first_available_engine() preventing downstream code from crashing.
>
> I would prefer to have this change in one patch.

v7 on the way as a single patch.

>
> >
> > Signed-off-by: Peter Senna Tschudin <me@petersenna.com>
> > ---
> >  tests/intel/gem_exec_capture.c | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/tests/intel/gem_exec_capture.c b/tests/intel/gem_exec_capture.c
> > index a8348f21b..2afb84283 100644
> > --- a/tests/intel/gem_exec_capture.c
> > +++ b/tests/intel/gem_exec_capture.c
> > @@ -665,10 +665,12 @@ static bool needs_recoverable_ctx(int fd)
> >               ctx = intel_ctx_create_all_physical(fd); \
> >               igt_assert(ctx); \
>
> If 'e' is causing problems here, maybe introduce new var 'tmpe'?

Not trivial as we can only define a new variable inside a macro inside
a block of something like a for loop.

> So you can start with:
>
>         e = NULL;
>                 for_each_ctx_engine(fd, ctx, tmpe) \
>
> then use tmpe->engine and after for_each... loop you may add
> igt_skip_on_f(&saved.engine == NULL, "no capable engine found\n"); \
>
> > -                     for_each_if(gem_class_can_store_dword(fd, e->class)) { \
> > +                     if(gem_class_can_store_dword(fd, e->class)) { \
>
> This looks much better, you replaced confusing macro with if()
>
> >                               igt_assert(e); \
> >                               saved = configure_hangs(fd, e, ctx->id); \
> >                               break; \
> > +                     } else { \
> > +                             igt_skip("find_first_available_engine(): No engine available\n"); \
>
> This do not solve a problem, you may have capable engine later on
> as there is for_each... loop.

Wow, thank you for catching that! I was confused by the 'break', my
reading of the code was wrong. Thank you!

>
> >                       } \
> >               e = &saved.engine; \
>
> If using 'tmpe' this should be before 'break;' line and here
> you should place igt_skip_on_f()

I guess we don't need another variable, saved is fine and will have a
valid engine or not when it is the case. What about initializing
`saved_engine.engine.name[0] = '\0';` in many() then:

#define find_first_available_engine(fd, ctx, e, saved) \
    do { \
        ctx = intel_ctx_create_all_physical(fd); \
        igt_assert(ctx); \
        for_each_ctx_engine(fd, ctx, e) { \
            if(gem_class_can_store_dword(fd, e->class)) { \
                saved = configure_hangs(fd, e, ctx->id); \
                break; \
            } \
        } \
        e = &saved.engine; \
        igt_skip_on_f(e->name[0] == '\0', "no capable engine found\n"); \
    } while(0)

Thank you Kamil!


>
> Regards,
> Kamil
>
> >       } while(0)
> > --
> > 2.44.0
> >

Re: [PATCH i-g-t v6 1/2] tests/intel/gem_exec_capture: Fix many-* subtests

[Kamil Konieczny]

Hi Peter,
On 2024-03-30 at 15:19:31 +0100, Peter Senna Tschudin wrote:
> Currently trying to run `gem_exec_capture --run-subtest
> many-4K-incremental` or `gem_exec_capture --run-subtest many-4K-zero`
> will fail with:
> 
>  (gem_exec_capture:81999) i915/gem_engine_topology-CRITICAL: Test
>  assertion failure function gem_engine_properties_configure, file
>  ../lib/i915/gem_engine_topology.c:577:
>  (gem_exec_capture:81999) i915/gem_engine_topology-CRITICAL: Failed assertion: ret == 1
>  (gem_exec_capture:81999) i915/gem_engine_topology-CRITICAL: Last errno: 9, Bad file descriptor
>  (gem_exec_capture:81999) i915/gem_engine_topology-CRITICAL: error: -1 != 1
> 
> This problem happens inside the macro find_first_available_engine()
> when:
>  1. for_each_ctx_engine() allocates an struct intel_engine_data 'ed'
>     inside a for loop. The core of the issue is that ed only exists
>     inside the for loop. As soon as the for loop ends, ed is out of scope
>     and after it's lifetime.
>  2. intel_get_current_engine() sets '*e' to an address of ed. This is ok
>     while inside the for loop, and is undefined behavior after the for
>     loop ends.
>  3. configure_hangs() uses '*e' after the lifetime of 'ed' has ended
>     leading to undefined behavior
>  4. After the call to find_first_available_engine() __captureN() will
>     fail as it expects '*e' to be valid. This is also undefined
>     behavior.
> 
> This patch fixes the issue in two steps:
>  1. Moves the call to configure_hangs() to inside the for loop, where
>     '*e' is valid because there are no issues with scope and lifetime of
>     'ed'.
>  2. Adds `e = &saved.engine;` to the end of
>     find_first_available_engine(). The reason why this works is twofold:
>     First 'saved' has the same content as e had when there were no
>     variable scope and lifetime issues. Second both '*e' and 'saved' are
>     defined in many() meaning that they both have the same scope and
>     lifetime.
> 
>  v6: new commit message; moves igt_assert(e); to before the call to
>      configure_hangs(). This check is there because '*e' is set by
>      intel_get_current_engine() which will return NULL if ed->n >=
>      ed->nengines.
> 
> Signed-off-by: Peter Senna Tschudin <me@petersenna.com>
> ---
>  tests/intel/gem_exec_capture.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/tests/intel/gem_exec_capture.c b/tests/intel/gem_exec_capture.c
> index 57b178f3e..a8348f21b 100644
> --- a/tests/intel/gem_exec_capture.c
> +++ b/tests/intel/gem_exec_capture.c
> @@ -665,10 +665,12 @@ static bool needs_recoverable_ctx(int fd)
>  		ctx = intel_ctx_create_all_physical(fd); \
>  		igt_assert(ctx); \
>  		for_each_ctx_engine(fd, ctx, e) \
> -			for_each_if(gem_class_can_store_dword(fd, e->class)) \
> +			for_each_if(gem_class_can_store_dword(fd, e->class)) { \
> +				igt_assert(e); \
--------------- ^
Why do you assert here? If it will be NULL code would blow at line
above, where e->class is used.

Regards,
Kamil

> +				saved = configure_hangs(fd, e, ctx->id); \
>  				break; \
> -		igt_assert(e); \
> -		saved = configure_hangs(fd, e, ctx->id); \
> +			} \
> +		e = &saved.engine; \
>  	} while(0)
>  
>  static void many(int fd, int dir, uint64_t size, unsigned int flags)
> -- 
> 2.44.0
> 

✓ CI.xeBAT: success for Fix memory access issue due to variable block scope (rev7)

[Patchwork]

[-- Attachment #1: Type: text/plain, Size: 1890 bytes --]

== Series Details ==

Series: Fix memory access issue due to variable block scope (rev7)
URL   : https://patchwork.freedesktop.org/series/131602/
State : success

== Summary ==

CI Bug Log - changes from XEIGT_7791_BAT -> XEIGTPW_10955_BAT
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

  

Participating hosts (5 -> 5)
------------------------------

  No changes in participating hosts

Known issues
------------

  Here are the changes found in XEIGTPW_10955_BAT that come from known issues:

### IGT changes ###

#### Possible fixes ####

  * igt@kms_frontbuffer_tracking@basic:
    - bat-adlp-7:         [FAIL][1] ([Intel XE#616]) -> [PASS][2]
   [1]: https://intel-gfx-ci.01.org/tree/intel-xe/IGT_7791/bat-adlp-7/igt@kms_frontbuffer_tracking@basic.html
   [2]: https://intel-gfx-ci.01.org/tree/intel-xe/IGTPW_10955/bat-adlp-7/igt@kms_frontbuffer_tracking@basic.html

  
  {name}: This element is suppressed. This means it is ignored when computing
          the status of the difference (SUCCESS, WARNING, or FAILURE).

  [Intel XE#1416]: https://gitlab.freedesktop.org/drm/xe/kernel/issues/1416
  [Intel XE#616]: https://gitlab.freedesktop.org/drm/xe/kernel/issues/616


Build changes
-------------

  * IGT: IGT_7791 -> IGTPW_10955
  * Linux: xe-1016-96000ec64d7e08436f2cafc89711c907576710f4 -> xe-1018-b334e21cd00347d48ee5e8e8ab0a69c91f97b036

  IGTPW_10955: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_10955/index.html
  IGT_7791: 7d7be3a56e299d9ad14ac2c6535a7e14b4cfd4df @ https://gitlab.freedesktop.org/drm/igt-gpu-tools.git
  xe-1016-96000ec64d7e08436f2cafc89711c907576710f4: 96000ec64d7e08436f2cafc89711c907576710f4
  xe-1018-b334e21cd00347d48ee5e8e8ab0a69c91f97b036: b334e21cd00347d48ee5e8e8ab0a69c91f97b036

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/intel-xe/IGTPW_10955/index.html

[-- Attachment #2: Type: text/html, Size: 2401 bytes --]

✗ Fi.CI.BAT: failure for Fix memory access issue due to variable block scope (rev7)

[Patchwork]

[-- Attachment #1: Type: text/plain, Size: 6665 bytes --]

== Series Details ==

Series: Fix memory access issue due to variable block scope (rev7)
URL   : https://patchwork.freedesktop.org/series/131602/
State : failure

== Summary ==

CI Bug Log - changes from CI_DRM_14509 -> IGTPW_10955
====================================================

Summary
-------

  **FAILURE**

  Serious unknown changes coming with IGTPW_10955 absolutely need to be
  verified manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in IGTPW_10955, please notify your bug team (I915-ci-infra@lists.freedesktop.org) to allow them
  to document this new failure mode, which will reduce false positives in CI.

  External URL: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_10955/index.html

Participating hosts (39 -> 33)
------------------------------

  Missing    (6): bat-arls-4 bat-dg1-7 bat-adlp-6 fi-snb-2520m bat-dg2-11 bat-arls-3 

Possible new issues
-------------------

  Here are the unknown changes that may have been introduced in IGTPW_10955:

### IGT changes ###

#### Possible regressions ####

  * igt@i915_selftest@live@execlists:
    - bat-adls-6:         [PASS][1] -> [TIMEOUT][2]
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_14509/bat-adls-6/igt@i915_selftest@live@execlists.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_10955/bat-adls-6/igt@i915_selftest@live@execlists.html

  
#### Warnings ####

  * igt@i915_selftest@live@gem:
    - bat-atsm-1:         [ABORT][3] ([i915#10564]) -> [INCOMPLETE][4]
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_14509/bat-atsm-1/igt@i915_selftest@live@gem.html
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_10955/bat-atsm-1/igt@i915_selftest@live@gem.html

  
Known issues
------------

  Here are the changes found in IGTPW_10955 that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@debugfs_test@basic-hwmon:
    - fi-tgl-1115g4:      NOTRUN -> [SKIP][5] ([i915#9318])
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_10955/fi-tgl-1115g4/igt@debugfs_test@basic-hwmon.html

  * igt@gem_huc_copy@huc-copy:
    - fi-cfl-8109u:       NOTRUN -> [SKIP][6] ([i915#2190])
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_10955/fi-cfl-8109u/igt@gem_huc_copy@huc-copy.html
    - fi-tgl-1115g4:      NOTRUN -> [SKIP][7] ([i915#2190])
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_10955/fi-tgl-1115g4/igt@gem_huc_copy@huc-copy.html

  * igt@gem_lmem_swapping@parallel-random-engines:
    - fi-tgl-1115g4:      NOTRUN -> [SKIP][8] ([i915#4613]) +3 other tests skip
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_10955/fi-tgl-1115g4/igt@gem_lmem_swapping@parallel-random-engines.html

  * igt@gem_lmem_swapping@verify-random:
    - fi-cfl-8109u:       NOTRUN -> [SKIP][9] ([i915#4613]) +3 other tests skip
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_10955/fi-cfl-8109u/igt@gem_lmem_swapping@verify-random.html

  * igt@kms_cursor_legacy@basic-busy-flip-before-cursor-atomic:
    - fi-tgl-1115g4:      NOTRUN -> [SKIP][10] ([i915#4103]) +1 other test skip
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_10955/fi-tgl-1115g4/igt@kms_cursor_legacy@basic-busy-flip-before-cursor-atomic.html

  * igt@kms_dsc@dsc-basic:
    - fi-tgl-1115g4:      NOTRUN -> [SKIP][11] ([i915#3555] / [i915#3840])
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_10955/fi-tgl-1115g4/igt@kms_dsc@dsc-basic.html

  * igt@kms_force_connector_basic@force-load-detect:
    - fi-tgl-1115g4:      NOTRUN -> [SKIP][12]
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_10955/fi-tgl-1115g4/igt@kms_force_connector_basic@force-load-detect.html

  * igt@kms_pm_backlight@basic-brightness:
    - fi-cfl-8109u:       NOTRUN -> [SKIP][13] +11 other tests skip
   [13]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_10955/fi-cfl-8109u/igt@kms_pm_backlight@basic-brightness.html
    - fi-tgl-1115g4:      NOTRUN -> [SKIP][14] ([i915#9812])
   [14]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_10955/fi-tgl-1115g4/igt@kms_pm_backlight@basic-brightness.html

  * igt@kms_psr@psr-sprite-plane-onoff:
    - fi-tgl-1115g4:      NOTRUN -> [SKIP][15] ([i915#9732]) +3 other tests skip
   [15]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_10955/fi-tgl-1115g4/igt@kms_psr@psr-sprite-plane-onoff.html

  * igt@kms_setmode@basic-clone-single-crtc:
    - fi-tgl-1115g4:      NOTRUN -> [SKIP][16] ([i915#3555])
   [16]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_10955/fi-tgl-1115g4/igt@kms_setmode@basic-clone-single-crtc.html

  
#### Possible fixes ####

  * igt@i915_selftest@live@hangcheck:
    - bat-adlp-11:        [ABORT][17] ([i915#10021]) -> [PASS][18]
   [17]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_14509/bat-adlp-11/igt@i915_selftest@live@hangcheck.html
   [18]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_10955/bat-adlp-11/igt@i915_selftest@live@hangcheck.html
    - bat-rpls-3:         [DMESG-WARN][19] ([i915#5591]) -> [PASS][20]
   [19]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_14509/bat-rpls-3/igt@i915_selftest@live@hangcheck.html
   [20]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_10955/bat-rpls-3/igt@i915_selftest@live@hangcheck.html

  
  {name}: This element is suppressed. This means it is ignored when computing
          the status of the difference (SUCCESS, WARNING, or FAILURE).

  [i915#10021]: https://gitlab.freedesktop.org/drm/intel/issues/10021
  [i915#10436]: https://gitlab.freedesktop.org/drm/intel/issues/10436
  [i915#10564]: https://gitlab.freedesktop.org/drm/intel/issues/10564
  [i915#2190]: https://gitlab.freedesktop.org/drm/intel/issues/2190
  [i915#3555]: https://gitlab.freedesktop.org/drm/intel/issues/3555
  [i915#3840]: https://gitlab.freedesktop.org/drm/intel/issues/3840
  [i915#4103]: https://gitlab.freedesktop.org/drm/intel/issues/4103
  [i915#4613]: https://gitlab.freedesktop.org/drm/intel/issues/4613
  [i915#5591]: https://gitlab.freedesktop.org/drm/intel/issues/5591
  [i915#9318]: https://gitlab.freedesktop.org/drm/intel/issues/9318
  [i915#9732]: https://gitlab.freedesktop.org/drm/intel/issues/9732
  [i915#9812]: https://gitlab.freedesktop.org/drm/intel/issues/9812


Build changes
-------------

  * CI: CI-20190529 -> None
  * IGT: IGT_7791 -> IGTPW_10955

  CI-20190529: 20190529
  CI_DRM_14509: b334e21cd00347d48ee5e8e8ab0a69c91f97b036 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGTPW_10955: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_10955/index.html
  IGT_7791: 7d7be3a56e299d9ad14ac2c6535a7e14b4cfd4df @ https://gitlab.freedesktop.org/drm/igt-gpu-tools.git

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_10955/index.html

[-- Attachment #2: Type: text/html, Size: 7741 bytes --]