build: fix -Wformat-security check when compiling with GCC#18882
build: fix -Wformat-security check when compiling with GCC#18882laanwj merged 1 commit intobitcoin:masterfrom
Conversation
|
I don't think we use printf, do we? |
configure.ac
Outdated
There was a problem hiding this comment.
I think it is counter intuitive to test whether flag X is supported and if yes, then compile with flag Y. IMO it is better to always have X equal to Y. In this particular case the code would depend on -Wformat being already set from somewhere else and would break if that is removed.
Having that in mind, would it be better to test whether -Wformat -Wformat-security is supported and if yes, then enable -Wformat -Wformat-security? We would end up with -Wformat being mentioned twice in the flags, but there is no downside to that?
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsReviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first. |
|
Looks like the issue here is more nuanced. # cat /etc/debian_version
bullseye/sid
# g++ --version
g++ (Ubuntu 9.3.0-10ubuntu2) 9.3.0
Copyright (C) 2019 Free Software Foundation, Inc.
This is free software; see the source for copying conditions. There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
# g++ test.cpp -Wformat-security
## cat /etc/debian_version
bullseye/sid
# g++ --version
g++ (Debian 9.3.0-11) 9.3.0
Copyright (C) 2019 Free Software Foundation, Inc.
This is free software; see the source for copying conditions. There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
# g++ test.cpp -Wformat-security
cc1plus: warning: '-Wformat-security' ignored without '-Wformat' [-Wformat-security]
# I think I'll just look at switching to |
I also wonder. Not that it hurts to enable the warning, but it seems pointless for our project. |
|
Is the reason so that we check during compilation of subtrees or header files of libs? |
|
🐙 This pull request conflicts with the target branch and needs rebase. |
fanquake
force-pushed
the
fix_gcc_wformat_security
branch
from
3c04a7a to
987c7df
Compare
Debian GCC ignores -Wformat-security, without -Wformat, which means when we test for it, it currently fails: ```bash checking whether C++ compiler accepts -Wformat-security... no ... configure:15907: checking whether C++ compiler accepts -Wformat-security configure:15926: g++ -std=c++11 -c -g -O2 -Werror -Wformat-security conftest.cpp >&5 cc1plus: error: '-Wformat-security' ignored without '-Wformat' [-Werror=format-security] cc1plus: all warnings being treated as errors ``` Fix this by just combining the -Wformat and -Wformat-security checks together.
fanquake
force-pushed
the
fix_gcc_wformat_security
branch
from
987c7df to
6cef365
Compare
|
I've modified the change to just combine the checking for Apple Clang: 11.0.3 (clang-1103.0.32.62) @vasild and maybe @hebasto would you like to take another look? |
|
Any reason to not use wformat=2 as pointed out in OP? |
I don't have the details on hand, but I remember it was generating additional warnings in in some dependencies. |
|
ACK 6cef365 No harm in fixing this. |
|
ACK 6cef365 |
7 participants
GCC expects
-Wformatto be passed with-Wformat-security, which meanswhen we test for it in configure it currently fails:
and never gets added to our CXX flags. Note that Clang does not have this requirement and the check is working correctly there.
The change in this PR is the simple fix, however we might want to consider using something like
-Wformat=2in future, which in GCC is equivalent to-Wformat -Wformat-nonliteral -Wformat-security -Wformat-y2k.and similar in Clang.