fix_1243_compiler_flags_0.27 #1245
Conversation
| check_c_compiler_flag(-fstack-protector-strong FSTACK_PROTECTOR_STRONG ) | ||
| check_c_compiler_flag(-fstack-clash-protection FSTACK_CLASH_PROTECTION ) | ||
| check_c_compiler_flag(-fcf-protection FCF-PROTECTION ) | ||
| check_c_compiler_flag(-fcf-protection FCF-PROTECTION ) |
There was a problem hiding this comment.
Look like the duplicate check for -fcf-protection could be removed.
In addition, shouldn't use - in cmake variable names. In this case, the variable name gets sent into the compile line as a define (-DFCF-PROTECTION) and the compiler doesn't like it:
/opt/local/bin/gcc-mp-9 -DFCF-PROTECTION -fcf-protection -o CMakeFiles/cmTC_d2242.dir/src.c.o -c path/to/CMakeFiles/CMakeTmp/src.c
<command-line>: warning: ISO C99 requires whitespace after the macro name
| # ASAN is available in clang from 3.1 and UBSAN from 3.3 | ||
| check_c_compiler_flag(-fno-omit-frame-pointer COMPILER_NO_OMIT_FRAME_POINTER ) | ||
| check_c_compiler_flag(-fsanitize=address,undefined COMPILER_SANITIZE_ADDRESS_UNDEFINED) | ||
| check_c_compiler_flag(--fsanitize=address COMPILER_SANITIZE_ADDRESS ) |
There was a problem hiding this comment.
This should be -fsanitize=address (one - not two).
Also, these tests don't work because although cmake passes the given flag to the compiler, it then does a separate linking step and it does not pass the flag to the linker so the link fails.
A workaround I've seen used elsewhere is to put the flag you're testing into CMAKE_REQUIRED_FLAGS instead of into the first argument of CHECK_C_COMPILER_FLAG. See my proposal below.
| check_c_compiler_flag(-fcf-protection FCF-PROTECTION ) | ||
| check_c_compiler_flag(-D_GLIBCXX_ASSERTIONS D_GLIBCXX_ASSERTIONS ) | ||
| check_c_compiler_flag(-D_FORTIFY_SOURCE=2 D_FORTIFY_SOURCE ) | ||
| check_c_compiler_flag(-Wp COMPILER_FLAG_WP ) |
There was a problem hiding this comment.
-Wp,something means "give the flag something only to the preprocessor." I would guess all compilers support this. For example, it is mentioned in the gcc 2.95.3 manpages from 2001. So I don't think you need to check -Wp.
In fact, on my macOS 10.13 system, the check failed:
-- Performing Test COMPILER_FLAG_WP - Failed
CMakeError.log contained:
warning: unknown warning option '-Wp' [-Wunknown-warning-option]
| check_c_compiler_flag(-Wformat-security COMPILER_FLAG_FORMAT_SECURITY ) | ||
| check_c_compiler_flag(-Wmissing-format-attribute COMPILER_FLAG_FORMAT_ATTTIBUTE ) | ||
| check_c_compiler_flag(-Woverloaded-virtual COMPILER_FLAG_OVERLOADED_VIRTUAL) | ||
| check_c_compiler_flag(-W COMPILER_FLAG_W ) |
There was a problem hiding this comment.
There seems to be some inconsistency here—not introduced by this PR but maybe it can be fixed. Here you use -W but elsewhere in the EXIV2_TEAM_EXTRA_WARNINGS section it uses -Wextra. These two flags are synonyms; -W is the old name for -Wextra. Maybe you can standardize on which of those flags you want to use and use it everywhere. I'm not sure when -Wextra replaced -W but if you want to support old compilers that know -W but not -Wextra you could first check if -Wextra is supported and if so use that, otherwise check -W and if it's supported then use that.
| add_compile_options(-fcf-protection) | ||
| endif() | ||
| if ( COMPILER_FLAG_WP ) | ||
| add_compile_options(-Wp) |
There was a problem hiding this comment.
-Wp,something means "give the flag something to the preprocessor only. It doesn't make sense to add the flag -Wp with no ,something after it.
| check_c_compiler_flag(-Wmissing-format-attribute COMPILER_FLAG_FORMAT_ATTTIBUTE ) | ||
| check_c_compiler_flag(-Woverloaded-virtual COMPILER_FLAG_OVERLOADED_VIRTUAL) | ||
| check_c_compiler_flag(-W COMPILER_FLAG_W ) | ||
| check_c_compiler_flag(--coverage COMPILER_FLAG_COVERAGE ) |
There was a problem hiding this comment.
Like the -fsanitize flags, --coverage needs to be supplied to both the compiler and the linker, so since the test only supplies it to the compiler it fails. This needs to be something more like:
cmake_push_check_state()
set(CMAKE_REQUIRED_FLAGS "--coverage")
check_c_compiler_flag("" COMPILER_FLAG_COVERAGE)
cmake_pop_check_state()
| check_c_compiler_flag(-Wpointer-arith COMPILER_FLAG_POINTER_ARITH ) | ||
| check_c_compiler_flag(-Wformat-security COMPILER_FLAG_FORMAT_SECURITY ) | ||
| check_c_compiler_flag(-Wmissing-format-attribute COMPILER_FLAG_FORMAT_ATTTIBUTE ) | ||
| check_c_compiler_flag(-Woverloaded-virtual COMPILER_FLAG_OVERLOADED_VIRTUAL) |
There was a problem hiding this comment.
cc1: warning: command line option '-Woverloaded-virtual' is valid for C++/ObjC++ but not for C
If your project contains C++ code and you want to use this flag for it, use check_cxx_compiler_flag instead, and make sure you add it to CXXFLAGS not CFLAGS.
| check_c_compiler_flag(-Wlogical-op COMPILE_FLAG_WLOGICAL_OP ) | ||
| check_c_compiler_flag(-Wdouble-promotion COMPILE_FLAG_WDOUBLE_PROMOTION ) | ||
| check_c_compiler_flag(-Wshadow COMPILE_FLAG_WSHADOW ) | ||
| check_c_compiler_flag(-Wuseless-cast COMPILE_FLAG_WUSELESS_CAST ) |
There was a problem hiding this comment.
cc1: warning: command line option '-Wuseless-cast' is valid for C++/ObjC++ but not for C
Same as above, use check_cxx_compiler_flag for this and later add the flag to whatever variable(s) are the cmake equivalent of CXXFLAGS.
| string(CONCAT EXTRA_COMPILE_FLAGS " -Wextra") | ||
| endif() | ||
| if ( COMPILE_FLAG_WSHADOW ) | ||
| string(CONCAT EXTRA_COMPILE_FLAGS " -Wshadow") |
There was a problem hiding this comment.
I don't think this is correct. string(CONCAT sets the given variable to the concatenation of all of the following strings. So this sets EXTRA_COMPILE_FLAGS to " -Wshadow", overwriting any previous value of EXTRA_COMPILE_FLAGS. Since what you actually want is to append " -Wshadow" to any existing value of that variable, it seems simpler to me to use set rather than string(CONCAT:
set(EXTRA_COMPILE_FLAGS "${EXTRA_COMPILE_FLAGS} -Wshadow")
| " -Wrestrict" | ||
| ) | ||
| if ( COMPILE_FLAG_WEXTRA ) | ||
| string(CONCAT EXTRA_COMPILE_FLAGS ${EXTRA_COMPILE_FLAGS}" -Wextra") |
There was a problem hiding this comment.
This results in strange quoting in the flags.make files, i.e.:
CXX_FLAGS = -fno-omit-frame-pointer -fsanitize=address,undefined -fno-sanitize-recover=all -fvisibility=hidden -fvisibility-inlines-hidden " -Wextra"" -Wlogical-op"" -Wdouble-promotion"" -Wshadow"" -Wpointer-arith"" -Wformat=2"" -Warray-bounds=2"" -Wduplicated-cond"" -Wduplicated-branches"" -Wrestrict" -fstack-protector-strong -fstack-clash-protection -D_GLIBCXX_ASSERTIONS -Wall -Wcast-align -Wpointer-arith -Wformat-security -Wmissing-format-attribute -W
I'm not sure whether gcc will use those oddly quoted flags or not.
It seems simpler to me to use set rather than string(CONCAT:
set(EXTRA_COMPILE_FLAGS "${EXTRA_COMPILE_FLAGS} -Wextra")
|
I haven't been able to get any help with this, so I'm closing this PR. |
See #1243
@piponazo As you know, I'm dyslexic and find changes like this very challenging. I think we have to use check_c_compiler_flag on every flag. It makes the CMake code longer and less brittle. I've submitted this to 0.27-maintenance.
Somebody reported yesterday that they couldn't build on a PPC. #1244. If you want to back-port PPC or ARM changes to 0.27-maintenance, that's fine. However I won't submit untested changes for PPC or ARM changes to 0.27-maintenance.