Skip to content

[clang] Emit bad shift warnings #70307

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.

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

Merged
merged 1 commit into from
Jul 11, 2024

Conversation

budimirarandjelovichtec
Copy link
Contributor

Diagnose bad shifts and emit warnings

@llvmbot llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" labels Oct 26, 2023
Copy link
Collaborator

@AaronBallman AaronBallman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The patch summary doesn't really explain why the changes are needed. Can you explain what problem you're solving (perhaps link to an issue if this is fixing one that was reported by someone)?

@budimirarandjelovichtec
Copy link
Contributor Author

The patch summary doesn't really explain why the changes are needed. Can you explain what problem you're solving (perhaps link to an issue if this is fixing one that was reported by someone)?

Main goal of this patch is to enable diagnosing and emitting warnings related to shift operator that are already found by GCC.

Original proposal fix and comments were posted on Pharbricator (link). This patch continues that patch.
Original issue was posted here.

@budimirarandjelovichtec
Copy link
Contributor Author

Ping @AaronBallman

@tbaederr
Copy link
Contributor

There are two questions above about removing a diagnostic that you haven't answered yet

@budimirarandjelovichtec budimirarandjelovichtec force-pushed the left-shift-count branch 3 times, most recently from 0ed3473 to 69c6fc0 Compare June 18, 2024 09:56
@djtodoro djtodoro merged commit e4163c0 into llvm:main Jul 11, 2024
6 checks passed
@llvm-ci
Copy link
Collaborator

llvm-ci commented Jul 11, 2024

LLVM Buildbot has detected a new failure on builder clang-hip-vega20 running on hip-vega20-0 while building clang at step 3 "annotate".

Full details are available at: https://lab.llvm.org/buildbot/#/builders/123/builds/1622

Here is the relevant piece of the build log for the reference:

Step 3 (annotate) failure: '../llvm-zorg/zorg/buildbot/builders/annotated/hip-build.sh --jobs=' (failure)
...
[36/38] : && /buildbot/hip-vega20-0/clang-hip-vega20/llvm/bin/clang++ -O3 -DNDEBUG  External/HIP/CMakeFiles/InOneWeekend-hip-6.0.2.dir/workload/ray-tracing/InOneWeekend/main.cc.o -o External/HIP/InOneWeekend-hip-6.0.2  --rocm-path=/buildbot/Externals/hip/rocm-6.0.2 --hip-link -rtlib=compiler-rt -unwindlib=libgcc -frtlib-add-rpath && cd /buildbot/hip-vega20-0/clang-hip-vega20/test-suite-build/External/HIP && /usr/local/bin/cmake -E create_symlink /buildbot/llvm-test-suite/External/HIP/InOneWeekend.reference_output /buildbot/hip-vega20-0/clang-hip-vega20/test-suite-build/External/HIP/InOneWeekend.reference_output-hip-6.0.2
[37/38] /buildbot/hip-vega20-0/clang-hip-vega20/llvm/bin/clang++ -DNDEBUG  -O3 -DNDEBUG   -w -Werror=date-time --rocm-path=/buildbot/Externals/hip/rocm-6.0.2 --offload-arch=gfx908 --offload-arch=gfx90a --offload-arch=gfx1030 --offload-arch=gfx1100 -xhip -mfma -MD -MT External/HIP/CMakeFiles/TheNextWeek-hip-6.0.2.dir/workload/ray-tracing/TheNextWeek/main.cc.o -MF External/HIP/CMakeFiles/TheNextWeek-hip-6.0.2.dir/workload/ray-tracing/TheNextWeek/main.cc.o.d -o External/HIP/CMakeFiles/TheNextWeek-hip-6.0.2.dir/workload/ray-tracing/TheNextWeek/main.cc.o -c /buildbot/llvm-test-suite/External/HIP/workload/ray-tracing/TheNextWeek/main.cc
[38/38] : && /buildbot/hip-vega20-0/clang-hip-vega20/llvm/bin/clang++ -O3 -DNDEBUG  External/HIP/CMakeFiles/TheNextWeek-hip-6.0.2.dir/workload/ray-tracing/TheNextWeek/main.cc.o -o External/HIP/TheNextWeek-hip-6.0.2  --rocm-path=/buildbot/Externals/hip/rocm-6.0.2 --hip-link -rtlib=compiler-rt -unwindlib=libgcc -frtlib-add-rpath && cd /buildbot/hip-vega20-0/clang-hip-vega20/test-suite-build/External/HIP && /usr/local/bin/cmake -E create_symlink /buildbot/llvm-test-suite/External/HIP/TheNextWeek.reference_output /buildbot/hip-vega20-0/clang-hip-vega20/test-suite-build/External/HIP/TheNextWeek.reference_output-hip-6.0.2
+ build_step 'Testing HIP test-suite'
+ echo '@@@BUILD_STEP Testing HIP test-suite@@@'
@@@BUILD_STEP Testing HIP test-suite@@@
+ ninja -v check-hip-simple
[0/1] cd /buildbot/hip-vega20-0/clang-hip-vega20/test-suite-build/External/HIP && /buildbot/hip-vega20-0/clang-hip-vega20/llvm/bin/llvm-lit -sv empty-hip-6.0.2.test with-fopenmp-hip-6.0.2.test saxpy-hip-6.0.2.test InOneWeekend-hip-6.0.2.test TheNextWeek-hip-6.0.2.test blender.test
-- Testing: 6 tests, 6 workers --
Testing:  0.. 10.. 20.. 30.. 40.. 
FAIL: test-suite :: External/HIP/InOneWeekend-hip-6.0.2.test (4 of 6)
******************** TEST 'test-suite :: External/HIP/InOneWeekend-hip-6.0.2.test' FAILED ********************

/buildbot/hip-vega20-0/clang-hip-vega20/test-suite-build/tools/timeit-target --timeout 7200 --limit-core 0 --limit-cpu 7200 --limit-file-size 209715200 --limit-rss-size 838860800 --append-exitstatus --redirect-output /buildbot/hip-vega20-0/clang-hip-vega20/test-suite-build/External/HIP/Output/InOneWeekend-hip-6.0.2.test.out --redirect-input /dev/null --summary /buildbot/hip-vega20-0/clang-hip-vega20/test-suite-build/External/HIP/Output/InOneWeekend-hip-6.0.2.test.time /buildbot/hip-vega20-0/clang-hip-vega20/test-suite-build/External/HIP/InOneWeekend-hip-6.0.2
cd /buildbot/hip-vega20-0/clang-hip-vega20/test-suite-build/External/HIP ; /buildbot/hip-vega20-0/clang-hip-vega20/test-suite-build/tools/fpcmp-target /buildbot/hip-vega20-0/clang-hip-vega20/test-suite-build/External/HIP/Output/InOneWeekend-hip-6.0.2.test.out InOneWeekend.reference_output-hip-6.0.2

+ cd /buildbot/hip-vega20-0/clang-hip-vega20/test-suite-build/External/HIP
+ /buildbot/hip-vega20-0/clang-hip-vega20/test-suite-build/tools/fpcmp-target /buildbot/hip-vega20-0/clang-hip-vega20/test-suite-build/External/HIP/Output/InOneWeekend-hip-6.0.2.test.out InOneWeekend.reference_output-hip-6.0.2
/buildbot/hip-vega20-0/clang-hip-vega20/test-suite-build/tools/fpcmp-target: Comparison failed, textual difference between 'M' and 'i'

********************
/usr/bin/strip: /bin/bash.stripped: Bad file descriptor
Testing:  0.. 10.. 20.. 30.. 40.. 50.. 60.. 70.. 80.. 90.. 
********************
Failed Tests (1):
  test-suite :: External/HIP/InOneWeekend-hip-6.0.2.test


Testing Time: 385.34s

Total Discovered Tests: 6
  Passed: 5 (83.33%)
  Failed: 1 (16.67%)
FAILED: External/HIP/CMakeFiles/check-hip-simple-hip-6.0.2 
cd /buildbot/hip-vega20-0/clang-hip-vega20/test-suite-build/External/HIP && /buildbot/hip-vega20-0/clang-hip-vega20/llvm/bin/llvm-lit -sv empty-hip-6.0.2.test with-fopenmp-hip-6.0.2.test saxpy-hip-6.0.2.test InOneWeekend-hip-6.0.2.test TheNextWeek-hip-6.0.2.test blender.test
ninja: build stopped: subcommand failed.
Step 12 (Testing HIP test-suite) failure: Testing HIP test-suite (failure)
@@@BUILD_STEP Testing HIP test-suite@@@
+ ninja -v check-hip-simple
[0/1] cd /buildbot/hip-vega20-0/clang-hip-vega20/test-suite-build/External/HIP && /buildbot/hip-vega20-0/clang-hip-vega20/llvm/bin/llvm-lit -sv empty-hip-6.0.2.test with-fopenmp-hip-6.0.2.test saxpy-hip-6.0.2.test InOneWeekend-hip-6.0.2.test TheNextWeek-hip-6.0.2.test blender.test
-- Testing: 6 tests, 6 workers --
Testing:  0.. 10.. 20.. 30.. 40.. 
FAIL: test-suite :: External/HIP/InOneWeekend-hip-6.0.2.test (4 of 6)
******************** TEST 'test-suite :: External/HIP/InOneWeekend-hip-6.0.2.test' FAILED ********************

/buildbot/hip-vega20-0/clang-hip-vega20/test-suite-build/tools/timeit-target --timeout 7200 --limit-core 0 --limit-cpu 7200 --limit-file-size 209715200 --limit-rss-size 838860800 --append-exitstatus --redirect-output /buildbot/hip-vega20-0/clang-hip-vega20/test-suite-build/External/HIP/Output/InOneWeekend-hip-6.0.2.test.out --redirect-input /dev/null --summary /buildbot/hip-vega20-0/clang-hip-vega20/test-suite-build/External/HIP/Output/InOneWeekend-hip-6.0.2.test.time /buildbot/hip-vega20-0/clang-hip-vega20/test-suite-build/External/HIP/InOneWeekend-hip-6.0.2
cd /buildbot/hip-vega20-0/clang-hip-vega20/test-suite-build/External/HIP ; /buildbot/hip-vega20-0/clang-hip-vega20/test-suite-build/tools/fpcmp-target /buildbot/hip-vega20-0/clang-hip-vega20/test-suite-build/External/HIP/Output/InOneWeekend-hip-6.0.2.test.out InOneWeekend.reference_output-hip-6.0.2

+ cd /buildbot/hip-vega20-0/clang-hip-vega20/test-suite-build/External/HIP
+ /buildbot/hip-vega20-0/clang-hip-vega20/test-suite-build/tools/fpcmp-target /buildbot/hip-vega20-0/clang-hip-vega20/test-suite-build/External/HIP/Output/InOneWeekend-hip-6.0.2.test.out InOneWeekend.reference_output-hip-6.0.2
/buildbot/hip-vega20-0/clang-hip-vega20/test-suite-build/tools/fpcmp-target: Comparison failed, textual difference between 'M' and 'i'

********************
/usr/bin/strip: /bin/bash.stripped: Bad file descriptor
Testing:  0.. 10.. 20.. 30.. 40.. 50.. 60.. 70.. 80.. 90.. 
********************
Failed Tests (1):
  test-suite :: External/HIP/InOneWeekend-hip-6.0.2.test


Testing Time: 385.34s

Total Discovered Tests: 6
  Passed: 5 (83.33%)
  Failed: 1 (16.67%)
FAILED: External/HIP/CMakeFiles/check-hip-simple-hip-6.0.2 
cd /buildbot/hip-vega20-0/clang-hip-vega20/test-suite-build/External/HIP && /buildbot/hip-vega20-0/clang-hip-vega20/llvm/bin/llvm-lit -sv empty-hip-6.0.2.test with-fopenmp-hip-6.0.2.test saxpy-hip-6.0.2.test InOneWeekend-hip-6.0.2.test TheNextWeek-hip-6.0.2.test blender.test
ninja: build stopped: subcommand failed.
program finished with exit code 1
elapsedTime=543.516237

@alanzhao1
Copy link
Contributor

FYI this is causing breakages in chrome - the android NDK has an enum element set equal to 1UL << 32 and we get a compile failure for --target=i686-linux-android26. See https://crbug.com/352592408,

code in question is here: https://android.googlesource.com/platform/frameworks/native/+/master/libs/nativewindow/include/android/hardware_buffer.h#329

The android NDK code is wrong, but this no longer compiles (even with -Wno-error).

@AaronBallman
Copy link
Collaborator

The android NDK code is wrong, but this no longer compiles (even with -Wno-error).

Well that's unfortunate! Thank you for letting us know.

@alanzhao1 do you think it's reasonable for the workaround to only apply to code in system headers, or does the NDK get included as regular headers generally? Do you need us to revert the changes while we resolve this?

@budimirarandjelovicsyrmia, can you please add a workaround so that the NDK code can continue to compile?

@budimirarandjelovichtec
Copy link
Contributor Author

@budimirarandjelovicsyrmia, can you please add a workaround so that the NDK code can continue to compile?

I will investigate this.

@alanzhao1
Copy link
Contributor

@alanzhao1 do you think it's reasonable for the workaround to only apply to code in system headers, or does the NDK get included as regular headers generally?

Having it only apply to system headers should be OK - in our case, chrome imports these headers as part of a --sysroot, which IIRC treats warnings the same way as -isystem

Do you need us to revert the changes while we resolve this?

If you can revert while working on a solution, that would be helpful.

@budimirarandjelovicsyrmia, can you please add a workaround so that the NDK code can continue to compile?

I will investigate this.

FYI I noticed that gcc can compile this with -fpermissive

aaryanshukla pushed a commit to aaryanshukla/llvm-project that referenced this pull request Jul 14, 2024
Diagnose bad shifts and emit warnings
@stbergmann
Copy link
Collaborator

This started to cause

$ cat test.c
#include <string.h>
void f(void) {
    char a[strlen("x")];
    (void) a;
}
$ clang -Wall -fsyntax-only test.c
test.c:3:12: warning: variable length array folded to constant array as an extension [-Wgnu-folding-constant]
    3 |     char a[strlen("x")];
      |            ^~~~~~~~~~~
1 warning generated.

to emit a warning, and I'm not sure that's intentional?

@AaronBallman
Copy link
Collaborator

This started to cause

$ cat test.c
#include <string.h>
void f(void) {
    char a[strlen("x")];
    (void) a;
}
$ clang -Wall -fsyntax-only test.c
test.c:3:12: warning: variable length array folded to constant array as an extension [-Wgnu-folding-constant]
    3 |     char a[strlen("x")];
      |            ^~~~~~~~~~~
1 warning generated.

to emit a warning, and I'm not sure that's intentional?

That is intentional; strlen is not a valid constant expression in C, and so that is technically a VLA, except we have an extension to fold it back into a constant.

@stbergmann
Copy link
Collaborator

That is intentional; strlen is not a valid constant expression in C, and so that is technically a VLA, except we have an extension to fold it back into a constant.

Thanks for confirming. From the commit message and the changes to clang/docs/ReleaseNotes.rst it just didn't look like this newly emitted warning was intended by the commit. But now I notice the changes to clang/test/Sema/builtins.c that do acknowledge this newly emitted warning.

tdf-gerrit pushed a commit to LibreOffice/core that referenced this pull request Jul 15, 2024
> desktop/unx/source/start.c:788:23: error: variable length array folded to constant array as an extension [-Werror,-Wgnu-folding-constant]
>   788 |             char resp[strlen("InternalIPC::SendArguments") + 1];
>       |                       ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

since
<llvm/llvm-project@e4163c0>
"[clang] Emit bad shift warnings (#70307)" (and see the comments starting at
<llvm/llvm-project#70307 (comment)>
"[clang] Emit bad shift warnings" for how this new warning is indeed
intentional)

Change-Id: Ie439a0f5f6f3b256fa82ec3a05fdc5fb3b840715
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/170510
Reviewed-by: Stephan Bergmann <[email protected]>
Tested-by: Jenkins
@alexfh
Copy link
Contributor

alexfh commented Jul 18, 2024

Just to confirm: is this the intentional outcome of this patch?
https://gcc.godbolt.org/z/Kf9YK1qM3

enum {
    a = 1<<12,
    b = (-1)<<13
};
<source>:3:9: error: expression is not an integral constant expression
    3 |     b = (-1)<<13
      |         ^~~~~~~~
<source>:3:13: note: left shift of negative value -1
    3 |     b = (-1)<<13
      |             ^

Though technically, it's UB before C++20, the diagnostic - "expression is not an integral constant expression" - doesn't sound helpful (how is it not constant or not integral?). And what's worse, is that it's an error rather than a warning that can be disabled.

@AaronBallman
Copy link
Collaborator

Just to confirm: is this the intentional outcome of this patch?

Before C++20, left shift of -1 does not produce a mathematical result that's within the range of representable values for the result type, so it's undefined behavior, and therefore not a core constant expression, and therefore invalid as the initializer for an enumeration constant. So I believe the behavior is expected and correct; you can see the UB diagnostic in other contexts: https://gcc.godbolt.org/z/bqj6xhzzr and GCC agrees with Clang in the enumeration case: https://gcc.godbolt.org/z/qjKzeaYTz

the diagnostic - "expression is not an integral constant expression" - doesn't sound helpful (how is it not constant or not integral?). And what's worse, is that it's an error rather than a warning that can be disabled.

It has integral constants but it's not an integral constant expression as far as the language is concerned. The note tells you why it's not a valid integral constant expression. As for warning vs error; the standard makes the code ill-formed and we treat that as an error generally speaking.

If you can't enable C++20 mode, you can still get the same value in C++17 and earlier with well-formed code: https://gcc.godbolt.org/z/3ra83joqs

@alexfh
Copy link
Contributor

alexfh commented Jul 18, 2024

I see. Thanks for the explanation and for the suggestion on how to fix that. Actually, the only instance I've seen, had already been fixed upstream (https://hg.openjdk.org/jdk/jdk/rev/71495d579a65), so the impact of this compiler behavior change is quite low in my part of the universe.

@alexfh
Copy link
Contributor

alexfh commented Jul 18, 2024

Now there's a different and more subtle side-effect of this patch: https://gcc.godbolt.org/z/YP3EfGern

The invalid expression 1 << 59 leads to the compiler silently ignoring the in-class initializer of the static data member.

@AaronBallman
Copy link
Collaborator

Now there's a different and more subtle side-effect of this patch: https://gcc.godbolt.org/z/YP3EfGern

The invalid expression 1 << 59 leads to the compiler silently ignoring the in-class initializer of the static data member.

Huh, I don't think that's an intentional change in this patch, good catch! Adding -pedantic to the command line yields warning: in-class initializer for static data member is not a constant expression; folding it to a constant is a GNU extension so I think we're missing that constant folding behavior at codegen time. CC @efriedma-quic @rjmccall for opinions

@efriedma-quic
Copy link
Collaborator

I think the issue is the usage of "Info.EvalStatus.Diag" to determine whether we consider the expression an ICE; that's going to lead to unpredictable results, and it's not how we handle things anywhere else. Not sure what the goal of that change is.

Separately, Sema::AddInitializerToDecl should probably print diagnostics when it checks for an ICE, instead of just emitting the generic diag::err_in_class_initializer_non_constant/diag::ext_in_class_initializer_non_constant. Any maybe we should consider enabling ext_in_class_initializer_non_constant by default.

@alanzhao1
Copy link
Contributor

@alanzhao1 do you think it's reasonable for the workaround to only apply to code in system headers, or does the NDK get included as regular headers generally?

Having it only apply to system headers should be OK - in our case, chrome imports these headers as part of a --sysroot, which IIRC treats warnings the same way as -isystem

Do you need us to revert the changes while we resolve this?

If you can revert while working on a solution, that would be helpful.

We (Chrome) no longer need a revert - we patched the NDK locally.

@efriedma-quic
Copy link
Collaborator

Posted #99579.

@AaronBallman
Copy link
Collaborator

@alanzhao1 do you think it's reasonable for the workaround to only apply to code in system headers, or does the NDK get included as regular headers generally?

Having it only apply to system headers should be OK - in our case, chrome imports these headers as part of a --sysroot, which IIRC treats warnings the same way as -isystem

Do you need us to revert the changes while we resolve this?

If you can revert while working on a solution, that would be helpful.

We (Chrome) no longer need a revert - we patched the NDK locally.

Thank you for letting us know!

sdkrystian added a commit that referenced this pull request Aug 5, 2024
This patch removes trailing whitespace from `SemaExpr.cpp` added by
#70307
@glandium
Copy link
Contributor

glandium commented Aug 7, 2024

We (Chrome) no longer need a revert - we patched the NDK locally.

Patching the NDK is not a solution that is nice everywhere, though. And the new warning is an error that can't be disabled.

@AaronBallman
Copy link
Collaborator

We (Chrome) no longer need a revert - we patched the NDK locally.

Patching the NDK is not a solution that is nice everywhere, though. And the new warning is an error that can't be disabled.

FWIW, we addressed this issue and backported the fix in #100452 -- are you seeing issues with the latest release candidate?

@efriedma-quic
Copy link
Collaborator

#100452 only fixed the "ignoring the in-class initializer of the static data member" issue. I don't think we did anything to allow downgrading overflow errors to warnings.

@glandium
Copy link
Contributor

glandium commented Aug 7, 2024

We (Chrome) no longer need a revert - we patched the NDK locally.

Patching the NDK is not a solution that is nice everywhere, though. And the new warning is an error that can't be disabled.

FWIW, we addressed this issue and backported the fix in #100452 -- are you seeing issues with the latest release candidate?

I don't know about the latest release candidate, but trunk still has the problem.

@efriedma-quic
Copy link
Collaborator

#102390 "fixes" the Android NDK issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang:frontend Language frontend issues, e.g. anything involving "Sema" clang Clang issues not falling into any other category
Projects
None yet
Development

Successfully merging this pull request may close these issues.