+ people from trailers of 09593216bff1
On Thu, Jul 27, 2023 at 03:50:58PM -0700, ndesaulniers@xxxxxxxxxx wrote:
> A new diagnostic in clang-17 now produces the following build error:
>
> drivers/gpu/drm/tests/drm_exec_test.c:41:3: error: cannot jump from this
> indirect goto statement to one of its possible targets
> 41 | drm_exec_retry_on_contention(&exec);
> | ^
> include/drm/drm_exec.h:96:4: note: expanded from macro
> 'drm_exec_retry_on_contention'
> 96 | goto *__drm_exec_retry_ptr;
> | ^
> drivers/gpu/drm/tests/drm_exec_test.c:39:2: note: possible target of
> indirect goto statement
> 39 | drm_exec_until_all_locked(&exec) {
> | ^
> include/drm/drm_exec.h:79:33: note: expanded from macro
> 'drm_exec_until_all_locked'
> 79 | __label__ __drm_exec_retry;
> drivers/gpu/drm/tests/drm_exec_test.c:39:2: note: jump enters a
> statement expression
>
> The GCC manually currently states that:
^ manual
> >> Jumping into a statement expression with a computed goto (see Labels
> >> as Values) has undefined behavior.
>
> So the diagnostic appears correct, even if codegen happened to produce
> working code.
>
> Looking closer at this code, while the original combination of statement
> expression, local label, and computed/indirect goto GNU C expressions
> were clever, a simple while loop and continue block might have sufficed.
>
> This approach might not work as expected if drm_exec_until_all_locked
> "loops" can be nested, but that doesn't appear to be an existing use
> case in the codebase.
>
> Fixes: commit 09593216bff1 ("drm: execution context for GEM buffers v7")
> Link: https://gcc.gnu.org/onlinedocs/gcc/Statement-Exprs.html
> Link: https://github.com/ClangBuiltLinux/linux/issues/1890
> Link: https://github.com/llvm/llvm-project/commit/20219106060208f0c2f5d096eb3aed7b712f5067
> Reported-by: Nathan Chancellor <nathan@xxxxxxxxxx>
> Reported-by: Naresh Kamboju <naresh.kamboju@xxxxxxxxxx>
> Signed-off-by: Nick Desaulniers <ndesaulniers@xxxxxxxxxx>
Thanks for the patch!
Tested-by: Nathan Chancellor <nathan@xxxxxxxxxx> # build
> ---
> Changes in v2:
> Fix the continue to be outside of the do while
> - Link to v1: https://lore.kernel.org/r/20230727-amdgpu-v1-1-a95690e75388@xxxxxxxxxx
> ---
> include/drm/drm_exec.h | 21 +++++----------------
> 1 file changed, 5 insertions(+), 16 deletions(-)
>
> diff --git a/include/drm/drm_exec.h b/include/drm/drm_exec.h
> index 73205afec162..fa1cc5c3d021 100644
> --- a/include/drm/drm_exec.h
> +++ b/include/drm/drm_exec.h
> @@ -70,18 +70,8 @@ struct drm_exec {
> * Core functionality of the drm_exec object. Loops until all GEM objects are
> * locked and no more contention exists. At the beginning of the loop it is
> * guaranteed that no GEM object is locked.
> - *
> - * Since labels can't be defined local to the loops body we use a jump pointer
> - * to make sure that the retry is only used from within the loops body.
> */
> -#define drm_exec_until_all_locked(exec) \
> - for (void *__drm_exec_retry_ptr; ({ \
> - __label__ __drm_exec_retry; \
> -__drm_exec_retry: \
> - __drm_exec_retry_ptr = &&__drm_exec_retry; \
> - (void)__drm_exec_retry_ptr; \
> - drm_exec_cleanup(exec); \
> - });)
> +#define drm_exec_until_all_locked(exec) while(drm_exec_cleanup(exec))
>
> /**
> * drm_exec_retry_on_contention - restart the loop to grap all locks
> @@ -90,11 +80,10 @@ __drm_exec_retry: \
> * Control flow helper to continue when a contention was detected and we need to
> * clean up and re-start the loop to prepare all GEM objects.
> */
> -#define drm_exec_retry_on_contention(exec) \
> - do { \
> - if (unlikely(drm_exec_is_contended(exec))) \
> - goto *__drm_exec_retry_ptr; \
> - } while (0)
> +#define drm_exec_retry_on_contention(exec) \
> + if (unlikely(drm_exec_is_contended(exec))) \
> + continue; \
> + do {} while (0)
>
> /**
> * drm_exec_is_contended - check for contention
>
> ---
> base-commit: 451cc82bd11eb6a374f4dbcfc1cf007eafea91ab
> change-id: 20230727-amdgpu-93c0e5302951
>
> Best regards,
> --
> Nick Desaulniers <ndesaulniers@xxxxxxxxxx>
>
