Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

"error: cannot jump from this indirect goto statement to one of its possible targets" in drivers/gpu/drm/tests/drm_exec_test.c #1890

Closed
nathanchance opened this issue Jul 22, 2023 · 4 comments
Closed

"error: cannot jump from this indirect goto statement to one of its possible targets" in drivers/gpu/drm/tests/drm_exec_test.c #1890

nathanchance opened this issue Jul 22, 2023 · 4 comments
Labels
[BUG] linux A bug that should be fixed in the mainline kernel. [BUG] llvm (main) A bug in an unreleased version of LLVM (this label is appropriate for regressions) [FIXED][LINUX] development cycle This bug was only present and fixed in a -next or -rc cycle Reported upstream This bug was filed on LLVM’s issue tracker, Phabricator, or the kernel mailing list.

Comments

@nathanchance
Copy link
Member

After llvm/llvm-project@2021910 (the reland of the patch that exposed #1887), I see what appears to be a legitimate error in -next:

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/next/linux-next/c/09593216bff15866f95c8ad406cb7fdcec1ee40a
https://git.kernel.org/next/linux-next/c/9710631cc8f367da04879760b892a8fc7aac9f45

I brought this up on Phabricator in case something with the error should be changed but I think the kernel is doing something that is documented as explicit undefined behavior according to GCC's documentation:

Jumping into a statement expression with a computed goto (see Labels as Values) has undefined behavior.

So it seems legitimate for clang to error on it and if that is indeed the case, I will bring it up on the upstream thread to see how the code can be reworked to avoid this undefined behavior.
@nathanchance nathanchance added [BUG] llvm (main) A bug in an unreleased version of LLVM (this label is appropriate for regressions) Reported upstream This bug was filed on LLVM’s issue tracker, Phabricator, or the kernel mailing list. Upstream report pending This bug needs to be reported upstream labels Jul 22, 2023
@nathanchance
Copy link
Member Author

Report send upstream to the kernel developer who wrote this code: https://lore.kernel.org/20230722220637.GA138486@dev-arch.thelio-3990X/

@nathanchance nathanchance removed the Upstream report pending This bug needs to be reported upstream label Jul 22, 2023
@nathanchance nathanchance mentioned this issue Jul 26, 2023
Merged
intel-lab-lkp pushed a commit to intel-lab-lkp/linux that referenced this issue Jul 27, 2023
@nickdesaulniers @intel-lab-lkp
drm: fix indirect goto into statement expression UB
7196972 
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 0959321 ("drm: execution context for GEM buffers v7")
Link: https://gcc.gnu.org/onlinedocs/gcc/Statement-Exprs.html
Link: ClangBuiltLinux#1890
Link: llvm/llvm-project@2021910
Reported-by: Nathan Chancellor <nathan@kernel.org>
Reported-by: Naresh Kamboju <naresh.kamboju@linaro.org>
Signed-off-by: Nick Desaulniers <ndesaulniers@google.com>
@nickdesaulniers
Copy link
Member

https://lore.kernel.org/lkml/20230727-amdgpu-v2-1-7fc66bc52bf6@google.com/

@nickdesaulniers nickdesaulniers self-assigned this Jul 27, 2023
@nickdesaulniers nickdesaulniers added [PATCH] Submitted A patch has been submitted for review [BUG] linux A bug that should be fixed in the mainline kernel. labels Jul 27, 2023
intel-lab-lkp pushed a commit to intel-lab-lkp/linux that referenced this issue Jul 27, 2023
@nickdesaulniers @intel-lab-lkp
drm: fix indirect goto into statement expression UB
7b0c54a 
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 0959321 ("drm: execution context for GEM buffers v7")
Link: https://gcc.gnu.org/onlinedocs/gcc/Statement-Exprs.html
Link: ClangBuiltLinux#1890
Link: llvm/llvm-project@2021910
Reported-by: Nathan Chancellor <nathan@kernel.org>
Reported-by: Naresh Kamboju <naresh.kamboju@linaro.org>
Signed-off-by: Nick Desaulniers <ndesaulniers@google.com>
intel-lab-lkp pushed a commit to intel-lab-lkp/linux that referenced this issue Jul 31, 2023
@intel-lab-lkp
drm/exec: use unique instead of local label
be60c44 
GCC forbids to jump to labels in loop conditions and a new clang
check stumbled over this.

So instead using a local label inside the loop condition use an
unique label outside of it.

Fixes: commit 0959321 ("drm: execution context for GEM buffers v7")
Link: https://gcc.gnu.org/onlinedocs/gcc/Statement-Exprs.html
Link: ClangBuiltLinux#1890
Link: llvm/llvm-project@2021910
Reported-by: Nathan Chancellor <nathan@kernel.org>
Reported-by: Naresh Kamboju <naresh.kamboju@linaro.org>
CC: Boris Brezillon <boris.brezillon@collabora.com>
Signed-off-by: Christian König <christian.koenig@amd.com>
@nathanchance
Copy link
Member Author

Alternative fix: https://lore.kernel.org/20230731123625.3766-1-christian.koenig@amd.com/

@nickdesaulniers nickdesaulniers removed their assignment Aug 1, 2023
intel-lab-lkp pushed a commit to intel-lab-lkp/linux that referenced this issue Aug 10, 2023
@ChristianKoenigAMD @bbrezillon
drm/exec: use unique instead of local label
616bcea 
GCC forbids to jump to labels in loop conditions and a new clang
check stumbled over this.

So instead using a local label inside the loop condition use an
unique label outside of it.

Fixes: 0959321 ("drm: execution context for GEM buffers v7")
Link: https://gcc.gnu.org/onlinedocs/gcc/Statement-Exprs.html
Link: ClangBuiltLinux#1890
Link: llvm/llvm-project@2021910
Reported-by: Nathan Chancellor <nathan@kernel.org>
Reported-by: Naresh Kamboju <naresh.kamboju@linaro.org>
CC: Boris Brezillon <boris.brezillon@collabora.com>
Signed-off-by: Christian König <christian.koenig@amd.com>
Tested-by: Nathan Chancellor <nathan@kernel.org>
Reviewed-by: Boris Brezillon <boris.brezillon@collabora.com>
Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>
Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
Link: https://patchwork.freedesktop.org/patch/msgid/20230731123625.3766-1-christian.koenig@amd.com
@nathanchance
Copy link
Member Author

https://cgit.freedesktop.org/drm/drm-misc/commit/?id=616bceae250d0bab7ab2cbcb0791d820434ffb71

@nathanchance nathanchance added [FIXED][LINUX] development cycle This bug was only present and fixed in a -next or -rc cycle and removed [PATCH] Submitted A patch has been submitted for review labels Aug 17, 2023
ishitatsuyuki pushed a commit to ishitatsuyuki/linux that referenced this issue Aug 20, 2023
@ChristianKoenigAMD @ishitatsuyuki
drm/exec: use unique instead of local label
2875c8b 
GCC forbids to jump to labels in loop conditions and a new clang
check stumbled over this.

So instead using a local label inside the loop condition use an
unique label outside of it.

Fixes: 0959321 ("drm: execution context for GEM buffers v7")
Link: https://gcc.gnu.org/onlinedocs/gcc/Statement-Exprs.html
Link: ClangBuiltLinux#1890
Link: llvm/llvm-project@2021910
Reported-by: Nathan Chancellor <nathan@kernel.org>
Reported-by: Naresh Kamboju <naresh.kamboju@linaro.org>
CC: Boris Brezillon <boris.brezillon@collabora.com>
Signed-off-by: Christian König <christian.koenig@amd.com>
Tested-by: Nathan Chancellor <nathan@kernel.org>
Reviewed-by: Boris Brezillon <boris.brezillon@collabora.com>
Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>
Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
Link: https://patchwork.freedesktop.org/patch/msgid/20230731123625.3766-1-christian.koenig@amd.com
ishitatsuyuki pushed a commit to ishitatsuyuki/linux that referenced this issue Sep 2, 2023
@ChristianKoenigAMD @ishitatsuyuki
drm/exec: use unique instead of local label
78966f8 
GCC forbids to jump to labels in loop conditions and a new clang
check stumbled over this.

So instead using a local label inside the loop condition use an
unique label outside of it.

Fixes: 0959321 ("drm: execution context for GEM buffers v7")
Link: https://gcc.gnu.org/onlinedocs/gcc/Statement-Exprs.html
Link: ClangBuiltLinux#1890
Link: llvm/llvm-project@2021910
Reported-by: Nathan Chancellor <nathan@kernel.org>
Reported-by: Naresh Kamboju <naresh.kamboju@linaro.org>
CC: Boris Brezillon <boris.brezillon@collabora.com>
Signed-off-by: Christian König <christian.koenig@amd.com>
Tested-by: Nathan Chancellor <nathan@kernel.org>
Reviewed-by: Boris Brezillon <boris.brezillon@collabora.com>
Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>
Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
Link: https://patchwork.freedesktop.org/patch/msgid/20230731123625.3766-1-christian.koenig@amd.com
jdpanderson pushed a commit to jdpanderson/linux that referenced this issue Oct 8, 2023
@ChristianKoenigAMD @jdpanderson
drm/exec: use unique instead of local label
b77a896 
GCC forbids to jump to labels in loop conditions and a new clang
check stumbled over this.

So instead using a local label inside the loop condition use an
unique label outside of it.

Fixes: 0959321 ("drm: execution context for GEM buffers v7")
Link: https://gcc.gnu.org/onlinedocs/gcc/Statement-Exprs.html
Link: ClangBuiltLinux#1890
Link: llvm/llvm-project@2021910
Reported-by: Nathan Chancellor <nathan@kernel.org>
Reported-by: Naresh Kamboju <naresh.kamboju@linaro.org>
CC: Boris Brezillon <boris.brezillon@collabora.com>
Signed-off-by: Christian König <christian.koenig@amd.com>
Tested-by: Nathan Chancellor <nathan@kernel.org>
Reviewed-by: Boris Brezillon <boris.brezillon@collabora.com>
Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>
Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
Link: https://patchwork.freedesktop.org/patch/msgid/20230731123625.3766-1-christian.koenig@amd.com
lutzbichler pushed a commit to lutzbichler/drm-kmod that referenced this issue Feb 4, 2024
@ChristianKoenigAMD @lutzbichler
drm/exec: use unique instead of local label
432204d 
GCC forbids to jump to labels in loop conditions and a new clang
check stumbled over this.

So instead using a local label inside the loop condition use an
unique label outside of it.

Fixes: 09593216bff1 ("drm: execution context for GEM buffers v7")
Link: https://gcc.gnu.org/onlinedocs/gcc/Statement-Exprs.html
Link: ClangBuiltLinux/linux#1890
Link: llvm/llvm-project@2021910
Reported-by: Nathan Chancellor <nathan@kernel.org>
Reported-by: Naresh Kamboju <naresh.kamboju@linaro.org>
CC: Boris Brezillon <boris.brezillon@collabora.com>
Signed-off-by: Christian König <christian.koenig@amd.com>
Tested-by: Nathan Chancellor <nathan@kernel.org>
Reviewed-by: Boris Brezillon <boris.brezillon@collabora.com>
Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>
Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
Link: https://patchwork.freedesktop.org/patch/msgid/20230731123625.3766-1-christian.koenig@amd.com
fadlyas07 pushed a commit to greenforce-project/chromeos-kernel-mirror that referenced this issue Feb 24, 2024
@ChristianKoenigAMD @ddavenport-chromium
UPSTREAM: drm/exec: use unique instead of local label
87cdb39 
GCC forbids to jump to labels in loop conditions and a new clang
check stumbled over this.

So instead using a local label inside the loop condition use an
unique label outside of it.

Fixes: 0959321 ("drm: execution context for GEM buffers v7")
Link: https://gcc.gnu.org/onlinedocs/gcc/Statement-Exprs.html
Link: ClangBuiltLinux/linux#1890
Link: llvm/llvm-project@2021910
Reported-by: Nathan Chancellor <nathan@kernel.org>
Reported-by: Naresh Kamboju <naresh.kamboju@linaro.org>
CC: Boris Brezillon <boris.brezillon@collabora.com>
Signed-off-by: Christian König <christian.koenig@amd.com>
Tested-by: Nathan Chancellor <nathan@kernel.org>
Reviewed-by: Boris Brezillon <boris.brezillon@collabora.com>
Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>
Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
Link: https://patchwork.freedesktop.org/patch/msgid/20230731123625.3766-1-christian.koenig@amd.com
(cherry picked from commit 616bcea)
Signed-off-by: Drew Davenport <ddavenport@chromium.org>

BUG=323757777
TEST=cq dry run

Backported using forklift.py

Change-Id: I82fdc1102f78ebb80507f1a48a2884b87844ae11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
[BUG] linux A bug that should be fixed in the mainline kernel. [BUG] llvm (main) A bug in an unreleased version of LLVM (this label is appropriate for regressions) [FIXED][LINUX] development cycle This bug was only present and fixed in a -next or -rc cycle Reported upstream This bug was filed on LLVM’s issue tracker, Phabricator, or the kernel mailing list.
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@nickdesaulniers @nathanchance