Messages in this thread | | | Date | Mon, 31 Jul 2023 11:50:53 +0200 | Subject | Re: [PATCH v2] drm: fix indirect goto into statement expression UB | From | Christian König <> |
| |
Am 31.07.23 um 09:03 schrieb Boris Brezillon: > On Fri, 28 Jul 2023 10:17:57 -0700 > Nathan Chancellor <nathan@kernel.org> wrote: > >> + people from trailers of 09593216bff1 >> >> On Thu, Jul 27, 2023 at 03:50:58PM -0700, ndesaulniers@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.
Yeah, agree that isn't what's supposed to happen here and would break a couple of use cases.
As a workaround we could define the label before the loop, but that makes the label local to the enclosing block, e.g. allows for using drm_exec_retry_on_contention() outside of drm_exec_until_all_locked().
Going to work on a patch, thanks for the notice.
Regards, Christian.
> >>> 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@kernel.org> >>> Reported-by: Naresh Kamboju <naresh.kamboju@linaro.org> >>> Signed-off-by: Nick Desaulniers <ndesaulniers@google.com> >> Thanks for the patch! >> >> Tested-by: Nathan Chancellor <nathan@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-a95690e75388@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 <ndesaulniers@google.com> >>>
| |