On Thu, Jul 27, 2023 at 3:47 PM <ndesaulni...@google.com> 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:
> >> 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 <nat...@kernel.org>
> Reported-by: Naresh Kamboju <naresh.kamb...@linaro.org>
> Signed-off-by: Nick Desaulniers <ndesaulni...@google.com>
> ---
> include/drm/drm_exec.h | 13 ++-----------
> 1 file changed, 2 insertions(+), 11 deletions(-)
>
> diff --git a/include/drm/drm_exec.h b/include/drm/drm_exec.h
> index 73205afec162..393ac022ed3a 100644
> --- a/include/drm/drm_exec.h
> +++ b/include/drm/drm_exec.h
> @@ -70,18 +70,9 @@ 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); \
> - });)
> + while(drm_exec_cleanup(exec))
>
> /**
> * drm_exec_retry_on_contention - restart the loop to grap all locks
> @@ -93,7 +84,7 @@ __drm_exec_retry:
> \
> #define drm_exec_retry_on_contention(exec) \
> do { \
> if (unlikely(drm_exec_is_contended(exec))) \
> - goto *__drm_exec_retry_ptr; \
> + continue; \
d'oh that's going to continue the do {} while(0) ... let me send a v2...
> } while (0)
>
> /**
>
> ---
> base-commit: 451cc82bd11eb6a374f4dbcfc1cf007eafea91ab
> change-id: 20230727-amdgpu-93c0e5302951
>
> Best regards,
> --
> Nick Desaulniers <ndesaulni...@google.com>
>
--
Thanks,
~Nick Desaulniers
