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.

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

[compiler-rt] Fix incorrect usages of check_cxx_compiler_flag #83779

Conversation

arichardson
Copy link
Member

@arichardson arichardson commented Mar 4, 2024

These checks have been broken since 6afe972.
The check_cxx_compiler_flag macro only takes two arguments and passing
three to it ends up calling
cmake_check_compiler_flag(CXX "${_FLAG}" ${_RESULT}) with ${_FLAG}
equal to -Werror and the result variable being the actually tested
compiler flag.

I noticed this because some of the flags that I know should be supported
were being flagged as not supported. --debug-trycompile shows the
following surprising line in the generated CMakeLists.txt:
add_definitions([==[-D-Wempty-body]==] [==[-Werror]==]) which then
results in the following error while running the check:

FAILED: CMakeFiles/cmTC_72736.dir/src.cxx.o
tmp/upstream-llvm-readonly/bin/clang++   -nodefaultlibs -std=c++17 -fcolor-diagnostics   -D-Wempty-body -Werror -MD -MT CMakeFiles/cmTC_72736.dir/src.cxx.o -MF CMakeFiles/cmTC_72736.dir/src.cxx.o.d -o CMakeFiles/cmTC_72736.dir/src.cxx.o -c .../cmake-build-all-sanitizers/CMakeFiles/CMakeScratch/TryCompile-nyh3QR/src.cxx
In file included from <built-in>:450:
<command line>:1:9: error: macro name must be an identifier
    1 | #define -Wempty-body 1
      |         ^
   1 error generated.

It would be great if CMake could be a bit more helpful here so I've
filed https://gitlab.kitware.com/cmake/cmake/-/issues/25735.

See also https://reviews.llvm.org/D146920.

Created using spr 1.3.6-beta.1
check_cxx_compiler_flag(-Werror -Wformat-security COMPILER_RT_HAS_BUILTIN_FORMAL_SECURITY_FLAG)
check_cxx_compiler_flag(-Werror -Wsizeof-array-div COMPILER_RT_HAS_SIZEOF_ARRAY_DIV_FLAG)
check_cxx_compiler_flag(-Werror -Wsizeof-pointer-div COMPILER_RT_HAS_SIZEOF_POINTER_DIV_FLAG)
check_cxx_compiler_flag("-Werror -Warray-bounds" COMPILER_RT_HAS_ARRAY_BOUNDS_FLAG)
Copy link
Member Author

Choose a reason for hiding this comment

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

It sounds like we might be able to drop the -Werror from some of these but that might be better as a separate change to not break buildbots.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

I guess no -ftrivial-auto-var-init=pattern is enabled and triggers crash as intended :)
I'll try to fix

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for looking into this. I tried x86 and aarch64 and didn't see any problems so assumed this was safe. Maybe related to big endian?

Copy link
Collaborator

Choose a reason for hiding this comment

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

The crash looks bogus, I don't see how this can get uninitialized. Trying 6f7ebcb

Copy link
Contributor

@nikic nikic left a comment

Choose a reason for hiding this comment

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

LGTM

@s-barannikov
Copy link
Contributor

These checks have been broken since 6afe972.

Actually, they have been broken since f051c1d.

My previous attempts to land this patch were unsuccessful. @nikic does this patch work for you now?

arichardson added a commit to arichardson/upstream-llvm-project that referenced this pull request Mar 7, 2024
These checks have been broken since 6afe972.
The check_cxx_compiler_flag macro only takes two arguments and passing
three to it ends up calling
`cmake_check_compiler_flag(CXX "${_FLAG}" ${_RESULT})` with ${_FLAG}
equal to `-Werror` and the result variable being the actually tested
compiler flag.

I noticed this because some of the flags that I know should be supported
were being flagged as not supported. `--debug-trycompile` shows the
following surprising line in the generated CMakeLists.txt:
`add_definitions([==[-D-Wempty-body]==] [==[-Werror]==])` which then
results in the following error while running the check:
```
FAILED: CMakeFiles/cmTC_72736.dir/src.cxx.o
tmp/upstream-llvm-readonly/bin/clang++   -nodefaultlibs -std=c++17 -fcolor-diagnostics   -D-Wempty-body -Werror -MD -MT CMakeFiles/cmTC_72736.dir/src.cxx.o -MF CMakeFiles/cmTC_72736.dir/src.cxx.o.d -o CMakeFiles/cmTC_72736.dir/src.cxx.o -c .../cmake-build-all-sanitizers/CMakeFiles/CMakeScratch/TryCompile-nyh3QR/src.cxx
In file included from <built-in>:450:
<command line>:1:9: error: macro name must be an identifier
    1 | #define -Wempty-body 1
      |         ^
   1 error generated.
```

It would be great if CMake could be a bit more helpful here so I've
filed https://gitlab.kitware.com/cmake/cmake/-/issues/25735.

Pull Request: llvm#83779
@arichardson arichardson merged commit bfa6444 into main Mar 8, 2024
6 checks passed
@arichardson arichardson deleted the users/arichardson/spr/compiler-rt-fix-incorrect-usages-of-check_cxx_compiler_flag branch March 8, 2024 07:25
@dtcxzyw
Copy link
Member

dtcxzyw commented Mar 8, 2024

This patch seems to break the build :(

[3/4] Building ASM object projects/compiler-rt/lib/builtins/CMakeFiles/clang_rt.builtins-x86_64.dir/x86_64/floatundisf.S.o
FAILED: projects/compiler-rt/lib/builtins/CMakeFiles/clang_rt.builtins-x86_64.dir/x86_64/floatundisf.S.o
/usr/bin/cc -DVISIBILITY_HIDDEN -D_GNU_SOURCE -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -I/home/dtcxzyw/llvm-build/projects/compiler-rt/lib/builtins -I/home/dtcxzyw/llvm-project/compiler-rt/lib/builtins -I/home/dtcxzyw/llvm-build/include -I/home/dtcxzyw/llvm-project/llvm/include -fPIC -O3 -DNDEBUG -m64 -fno-lto -Werror=array-bounds -Werror=uninitialized -Werror=shadow -Werror=empty-body -Werror=sizeof-pointer-memaccess -Werror=sizeof-array-argument -Werror=format-security -std=c11 -Werror=builtin-declaration-mismatch -MD -MT projects/compiler-rt/lib/builtins/CMakeFiles/clang_rt.builtins-x86_64.dir/x86_64/floatundisf.S.o -MF projects/compiler-rt/lib/builtins/CMakeFiles/clang_rt.builtins-x86_64.dir/x86_64/floatundisf.S.o.d -o projects/compiler-rt/lib/builtins/CMakeFiles/clang_rt.builtins-x86_64.dir/x86_64/floatundisf.S.o -c /home/dtcxzyw/llvm-project/compiler-rt/lib/builtins/x86_64/floatundisf.S
cc1: error: ‘-Wformat-security’ ignored without ‘-Wformat’ [-Werror=format-security]
cc1: some warnings being treated as errors

arichardson added a commit that referenced this pull request Mar 8, 2024
GCC complains if we pass -Werror=format-security without -Wformat.
Reported at #83779 (comment)
@arichardson
Copy link
Member Author

This patch seems to break the build :(

[3/4] Building ASM object projects/compiler-rt/lib/builtins/CMakeFiles/clang_rt.builtins-x86_64.dir/x86_64/floatundisf.S.o
FAILED: projects/compiler-rt/lib/builtins/CMakeFiles/clang_rt.builtins-x86_64.dir/x86_64/floatundisf.S.o
/usr/bin/cc -DVISIBILITY_HIDDEN -D_GNU_SOURCE -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -I/home/dtcxzyw/llvm-build/projects/compiler-rt/lib/builtins -I/home/dtcxzyw/llvm-project/compiler-rt/lib/builtins -I/home/dtcxzyw/llvm-build/include -I/home/dtcxzyw/llvm-project/llvm/include -fPIC -O3 -DNDEBUG -m64 -fno-lto -Werror=array-bounds -Werror=uninitialized -Werror=shadow -Werror=empty-body -Werror=sizeof-pointer-memaccess -Werror=sizeof-array-argument -Werror=format-security -std=c11 -Werror=builtin-declaration-mismatch -MD -MT projects/compiler-rt/lib/builtins/CMakeFiles/clang_rt.builtins-x86_64.dir/x86_64/floatundisf.S.o -MF projects/compiler-rt/lib/builtins/CMakeFiles/clang_rt.builtins-x86_64.dir/x86_64/floatundisf.S.o.d -o projects/compiler-rt/lib/builtins/CMakeFiles/clang_rt.builtins-x86_64.dir/x86_64/floatundisf.S.o -c /home/dtcxzyw/llvm-project/compiler-rt/lib/builtins/x86_64/floatundisf.S
cc1: error: ‘-Wformat-security’ ignored without ‘-Wformat’ [-Werror=format-security]
cc1: some warnings being treated as errors

Thanks for the report, I managed to reproduce it with GCC and it should be fixed by 80a9574

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants