build: add more picky warnings and fix them #12331
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
vszakats
force-pushed
the
more-picky-from-ng
branch
2 times, most recently
from
97879ac
to
c07543c
Compare
vszakats
force-pushed
the
more-picky-from-ng
branch
2 times, most recently
from
38a1b0e
to
ab3cf32
Compare
This comment was marked as outdated.
This comment was marked as outdated.
vszakats
changed the title
vszakats
changed the title
vszakats
force-pushed
the
more-picky-from-ng
branch
4 times, most recently
from
bccb316
to
e2a8170
Compare
This comment was marked as resolved.
This comment was marked as resolved.
|
Will continue with this after merging #12346. |
vszakats
force-pushed
the
more-picky-from-ng
branch
2 times, most recently
from
dcc309f
to
3019e33
Compare
This comment was marked as resolved.
This comment was marked as resolved.
vszakats
force-pushed
the
more-picky-from-ng
branch
from
975904b
to
a5ee38b
Compare
This comment was marked as resolved.
This comment was marked as resolved.
vszakats
changed the title
vszakats
force-pushed
the
more-picky-from-ng
branch
from
a5ee38b
to
e68feb8
Compare
This comment was marked as resolved.
This comment was marked as resolved.
vszakats
force-pushed
the
more-picky-from-ng
branch
2 times, most recently
from
1f2c59c
to
b98b600
Compare
The warning is triggered by libtool-generate code on MSYS gcc env:
```
bash.exe : ./.libs/lt-curl.c: In function 'lt_fatal':
At line:88 char:3
+ & bash -e -c "cd $env:POSIX_PATH_PREFIX/c/projects/curl && autoreco ...
+ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+ CategoryInfo : NotSpecified: (./.libs/lt-curl...ion 'lt_fatal'::String) [], RemoteException
+ FullyQualifiedErrorId : NativeCommandError
./.libs/lt-curl.c:594:1: warning: function might be candidate for attribute 'noreturn' [-Wsuggest-attribute=noreturn]
594 | lt_fatal (const char *file, int line, const char *message, ...)
| ^~~~~~~~
```
Ref: https://ci.appveyor.com/project/curlorg/curl/builds/48553586/job/3qkgjauiqla5fj45?fullLog=true#L1675
It might need to be applied also to clang and possible to other target
environments where this pops up in the future.
```
vtls/sectransp.c:3448:9: error: 'break' will never be executed [-Werror,-Wunreachable-code-break]
break;
^~~~~
vtls/sectransp.c:3428:9: error: 'break' will never be executed [-Werror,-Wunreachable-code-break]
break;
^~~~~
vtls/sectransp.c:3418:9: error: 'break' will never be executed [-Werror,-Wunreachable-code-break]
break;
^~~~~
```
Ref: https://github.com/curl/curl/actions/runs/6891484996/job/18746659406?pr=12331#step:9:1193
These were detected in `else` branches of dynamic feature checks,
where the results of these checks is known at compiler-time, e.g.
```
if(SecCertificateCopySubjectSummary) /* -> true */
```
Probably fixable as a separate micro-project, but given that
SecureTransport is a deprecated API anyway, for now I just silence
these warnings for the whole SecureTransport code.
```
vtls/sectransp.c:985:9: error: code will never be executed [-Werror,-Wunreachable-code]
(void)SecCertificateCopyCommonName(cert, &server_cert_summary);
^~~~~~~~~~~~~~~~~~~~~~~~~~~~
vtls/sectransp.c:980:6: error: code will never be executed [-Werror,-Wunreachable-code]
if(SecCertificateCopySubjectSummary)
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
vtls/sectransp.c:1408:14: error: code will never be executed [-Werror,-Wunreachable-code]
[...]
long i = ssl_version;
^~~~~~~~~~~
vtls/sectransp.c:1743:11: error: code will never be executed [-Werror,-Wunreachable-code]
(void)SSLSetProtocolVersionEnabled(backend->ssl_ctx,
^~~~~~~~~~~~~~~~~~~~~~~~~~~~
vtls/sectransp.c:1683:8: error: code will never be executed [-Werror,-Wunreachable-code]
[...]
if(backend->ssl_ctx)
^~~~~~~
vtls/sectransp.c:2969:11: error: code will never be executed [-Werror,-Wunreachable-code]
err = SSLCopyPeerCertificates(backend->ssl_ctx, &server_certs);
^~~~~~~~~~~~~~~~~~~~~~~
vtls/sectransp.c:3171:13: error: code will never be executed [-Werror,-Wunreachable-code]
(void)SSLDisposeContext(backend->ssl_ctx);
^~~~~~~~~~~~~~~~~
```
Ref: https://github.com/curl/curl/actions/runs/6891484996/job/18746659406?pr=12331#step:9:1168
Present since initial commit ae1912c (1999-12-29). This suggests this may not be needed anymore, but if it does, we may restore this for those specific (non-Windows) systems.
Originally added in 687df5c (2010-08-02). This got complicated later: c49e968 If there are still systems around where this is causing warnings, we may restore the prototype, but for those specific systems only. Otherwise it's a double declaration on every other system which we must match precisely, and may also cause a warning.
…sVistaOrGreater` in unity builds.
vszakats
force-pushed
the
more-picky-from-ng
branch
from
e58c761
to
02298a3
Compare
vszakats
added a commit
to vszakats/curl
that referenced
this pull request
Jul 12, 2024
vszakats
added a commit
to vszakats/curl
that referenced
this pull request
Jul 12, 2024
vszakats
added a commit
to vszakats/curl
that referenced
this pull request
Jul 12, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Enable more picky compiler warnings. I've found these options in the
nghttp3 project when implementing the CMake quick picky warning
functionality for it [1].
-Wunused-macroswas too noisy to keep around, but fixed a few issuesit revealed while testing.
autotools: reflect the more precisely-versioned clang warnings.
Follow-up to 033f8e2 build: picky warning updates #12324
autotools: sync between clang and gcc the way we set
no-multichar.autotools: avoid setting
-Wstrict-aliasing=3twice.autotools: disable
-Wmissing-noreturnfor MSYS gcc targets [2].It triggers in libtool-generated stub code.
lib/timeval: delete a redundant
!MSDOSguard from aWIN32branch.lib/curl_setup.h: delete duplicate declaration for
fileno.Added in initial commit ae1912c
(1999-12-29). This suggests this may not be needed anymore, but if
it does, we may restore this for those specific (non-Windows) systems.
lib: delete unused macro
FTP_BUFFER_ALLOCSIZEsincec1d6fe2.
lib: delete unused macro
isxdigit_asciisincef65f750.
lib/mqtt: delete unused macro
MQTT_HEADER_LEN.lib/multi: delete unused macro
SH_READ/SH_WRITE.lib/hostip: add
noreturnfunction attribute via newCURL_NORETURNmacro.
lib/mprintf: delete duplicate declaration for
Curl_dyn_vprintf.lib/rand: fix
-Wunreachable-codeand related fallouts [3].lib/setopt: fix
-Wunreachable-code-break.lib/system_win32 and lib/timeval: fix double declarations for
Curl_freqandCurl_isVistaOrGreaterin CMake UNITY mode [4].lib/warnless: fix double declarations in CMake UNITY mode [5].
This was due to force-disabling the header guard of
warnless.htoto reapply it to source code coming after
warnless.cin UNITYbuilds. This reapplied declarations too, causing the warnings.
Solved by adding a header guard for the lines that actually need
to be reapplied.
lib/vauth/digest: fix
-Wunreachable-code-break[6].lib/vssh/libssh2: fix
-Wunreachable-code-breakand delete redundantblock.
lib/vtls/sectransp: fix
-Wunreachable-code-break[7].lib/vtls/sectransp: suppress
-Wunreachable-code.Detected in
elsebranches of dynamic feature checks, with resultsknown at compile-time, e.g.
Likely fixable as a separate micro-project, but given SecureTransport
is deprecated anyway, let's just silence these locally.
src/tool_help: delete duplicate declaration for
helptext.src/tool_xattr: fix
-Wunreachable-code.tests: delete duplicate declaration for
unitfail[8].tests: delete duplicate declaration for
strncasecompare.tests/libtest: delete duplicate declaration for
gethostname.Originally added in 687df5c
(2010-08-02).
Got complicated later: c49e968
If there are still systems around with warnings, we may restore the
prototype, but limited for those systems.
tests/lib2305: delete duplicate declaration for
libtest_debug_config.tests/h2-download: fix
-Wunreachable-code-break.[1] https://github.com/ngtcp2/nghttp3/blob/a70edb08e954d690e8fb2c1df999b5a056f8bf9f/cmake/PickyWarningsC.cmake
[2] https://ci.appveyor.com/project/curlorg/curl/builds/48553586/job/3qkgjauiqla5fj45?fullLog=true#L1675
[3] https://github.com/curl/curl/actions/runs/6880886309/job/18716044703?pr=12331#step:7:72
https://github.com/curl/curl/actions/runs/6883016087/job/18722707368?pr=12331#step:7:109
[4] https://ci.appveyor.com/project/curlorg/curl/builds/48555101/job/9g15qkrriklpf1ut#L204
[5] https://ci.appveyor.com/project/curlorg/curl/builds/48555101/job/9g15qkrriklpf1ut#L218
[6] https://github.com/curl/curl/actions/runs/6880886309/job/18716042927?pr=12331#step:7:290
[7] https://github.com/curl/curl/actions/runs/6891484996/job/18746659406?pr=12331#step:9:1193
[8] https://github.com/curl/curl/actions/runs/6882803986/job/18722082562?pr=12331#step:33:1870
Closes #12331
Clearer without whitespace changes: https://github.com/curl/curl/pull/12331/files?w=1