[PATCH] D154696: [Clang] Diagnose jumps into statement expressions

[Corentin Jabot via Phabricator via cfe-commits]

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