On Fri, 28 Jul 2023 10:17:57 -0700
Nathan Chancellor <nat...@kernel.org> wrote:
> + people from trailers of 09593216bff1
>
> On Thu, Jul 27, 2023 at 03:50:58PM -0700, 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:
>
> ^ 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.
Hm, that's exactly the sort of things we were trying to be robust
against with the original approach. With this version, we're back to a
situation where
drm_exec_until_all_locked(exec) {
for (...) {
drm_exec_retry_on_contention(exec);
}
}
doesn't do what we expect it to do, and that's a use case we want to
support.
> >
> > 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>
>
> Thanks for the patch!
>
> Tested-by: Nathan Chancellor <nat...@kernel.org> # 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-a95690e75...@google.com
> > ---
> > 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 <ndesaulni...@google.com>
> >
