cor3ntin added a comment.
In D154696#4525100 <https://reviews.llvm.org/D154696#4525100>, @nathanchance
wrote:
> The error added by this patch triggers on some recently added code to the
> Linux kernel's -next tree, which I failed to test above due to its generally
> unstable nature:
>
> 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
>
> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/include/drm/drm_exec.h
> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/drivers/gpu/drm/tests/drm_exec_test.c
>
> A standalone reproducer from the kernel sources:
>
> struct drm_exec {};
> int drm_exec_cleanup(struct drm_exec *);
> void drm_exec_fini(struct drm_exec *);
> void drm_exec_init(struct drm_exec *);
> int drm_exec_is_contended(struct drm_exec *);
> void drm_exec_lock_obj(struct drm_exec *);
>
> void test_lock(void)
> {
> struct drm_exec exec;
>
> drm_exec_init(&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);
> });) {
> drm_exec_lock_obj(&exec);
> do {
> if
> (__builtin_expect(!!(drm_exec_is_contended(&exec)),
> 0))
> goto *__drm_exec_retry_ptr;
> } while (0);
> }
> drm_exec_fini(&exec);
> }
>
>
>
> $ clang -fsyntax-only test.c
> test.c:24:5: error: cannot jump from this indirect goto statement to one of
> its possible targets
> 24 | goto *__drm_exec_retry_ptr;
> | ^
> test.c:15:6: note: possible target of indirect goto statement
> 15 | __drm_exec_retry:
> | ^
> test.c:13:35: note: jump enters a statement expression
> 13 | for (void *__drm_exec_retry_ptr; ({
> | ^
> 1 error generated.
>
> I am not sure this is a problem with the patch, since if I am reading GCC's
> documentation on statement expressions correctly
> (https://gcc.gnu.org/onlinedocs/gcc/Statement-Exprs.html), the construct the
> kernel is using with labels as values to jump into a statement expression
> with a computed goto is undefined behavior, but I figured I would bring it up
> just in case there is a problem.
>
>> Jumping into a statement expression with goto or using a switch statement
>> outside the statement expression with a case or default label inside the
>> statement expression is not permitted. Jumping into a statement expression
>> with a computed goto (see Labels as Values) has undefined behavior.
>
> If this is a problem on the kernel side, I can bring this up to them. GCC
> 13.1.0 at least does not error on this but if it is documented as UB, the
> construct should still be avoided.
At least GCC documentation says this is UB. It's interesting GCC does not
diagnose.
I'd be reluctant to revert the patch at this point as it ensure we don't jump
into unevaluated statements like sizeof, etc.
Can you let me know what the linux folks think? Thanks a lot!
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D154696/new/
https://reviews.llvm.org/D154696
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
