build: add more picky warnings and fix them by vszakats · Pull Request #12331 · curl/curl

vszakats · 2023-11-15T14:44:38Z

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-macros was too noisy to keep around, but fixed a few issues
it 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=3 twice.
autotools: disable -Wmissing-noreturn for MSYS gcc targets [2].
It triggers in libtool-generated stub code.
lib/timeval: delete a redundant !MSDOS guard from a WIN32 branch.
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_ALLOCSIZE since
c1d6fe2.
lib: delete unused macro isxdigit_ascii since
f65f750.
lib/mqtt: delete unused macro MQTT_HEADER_LEN.
lib/multi: delete unused macro SH_READ/SH_WRITE.
lib/hostip: add noreturn function attribute via new CURL_NORETURN
macro.
lib/mprintf: delete duplicate declaration for Curl_dyn_vprintf.
lib/rand: fix -Wunreachable-code and related fallouts [3].
lib/setopt: fix -Wunreachable-code-break.
lib/system_win32 and lib/timeval: fix double declarations for
Curl_freq and Curl_isVistaOrGreater in 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.h to
to reapply it to source code coming after warnless.c in UNITY
builds. 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-break and delete redundant
block.
lib/vtls/sectransp: fix -Wunreachable-code-break [7].
lib/vtls/sectransp: suppress -Wunreachable-code.
Detected in else branches of dynamic feature checks, with results
known at compile-time, e.g.
```
if(SecCertificateCopySubjectSummary)  /* -> true */
```
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

vszakats · 2023-11-17T12:58:37Z

Will continue with this after merging #12346.

``` 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.

Follow-up to 84338c4 curl#12331

This warning remains silent in unity builds. Since we're using unity build in CI for most jobs, warnings remain undetected there. Disable them for all builds to avoid a surprise warning outside our CI. The issue caught by the warning is useful for a tidy codebase, but doesn't affect executed code. It was enabled in 84338c4 curl#12331 (2023-11-15). llvm source: https://github.com/llvm/llvm-project/blob/fee2953f23bd8a8a71e574e6a8db08033778d3a4/clang/lib/Sema/AnalysisBasedWarnings.cpp#L125-L134 llvm issue: llvm/llvm-project#71046 Follow-up to 7c023c3 curl#15384

This warning remains silent in unity builds. Since we're using unity in CI for most jobs, warnings remain undetected there. Disable them for all builds to avoid a surprise warning outside our CI. The issue caught by the warning is useful for a tidy codebase, but doesn't affect executed code. It was enabled in 84338c4 #12331 (2023-11-15). llvm source: https://github.com/llvm/llvm-project/blob/fee2953f23bd8a8a71e574e6a8db08033778d3a4/clang/lib/Sema/AnalysisBasedWarnings.cpp#L125-L134 llvm issue: llvm/llvm-project#71046 Follow-up to 7c023c3 #15384 Closes #15416

Fixes: ``` In file included from C:\projects\curl\_bld\lib\CMakeFiles\libcurl_object.dir\Unity\unity_0_c.c:145: C:/projects/curl/lib/formdata.c(797,29): error : extension used [-Werror,-Wlanguage-extension-token] [C:\projects\curl\_bld\lib\libcurl_object.vcxproj] In file included from C:\projects\curl\_bld\lib\CMakeFiles\libcurl_object.dir\Unity\unity_0_c.c:391: C:/projects/curl/lib/warnless.c(117,33): error : extension used [-Werror,-Wlanguage-extension-token] [C:\projects\curl\_bld\lib\libcurl_object.vcxproj] C:/projects/curl/lib/warnless.c(60,28): message : expanded from macro 'CURL_MASK_SCOFFT' [C:\projects\curl\_bld\lib\libcurl_object.vcxproj] C:/projects/curl/lib/warnless.c(59,38): message : expanded from macro 'CURL_MASK_UCOFFT' [C:\projects\curl\_bld\lib\libcurl_object.vcxproj] C:\projects\curl\include\curl/system.h(352,40): message : expanded from macro 'CURL_TYPEOF_CURL_OFF_T' [C:\projects\curl\_bld\lib\libcurl_object.vcxproj] ``` https://ci.appveyor.com/project/curlorg/curl/builds/50887118/job/3kjlub2t0k6kadka#L180 Triggered by the `__int64` type, when using clang-cl (17.0.3). Follow-up to 84338c4 curl#12331

This warning remains silent in unity builds. Since we're using unity in CI for most jobs, warnings remain undetected there. Disable them for all builds to avoid a surprise warning outside our CI. The issue caught by the warning is useful for a tidy codebase, but doesn't affect executed code. It was enabled in 84338c4 curl#12331 (2023-11-15). llvm source: https://github.com/llvm/llvm-project/blob/fee2953f23bd8a8a71e574e6a8db08033778d3a4/clang/lib/Sema/AnalysisBasedWarnings.cpp#L125-L134 llvm issue: llvm/llvm-project#71046 Follow-up to 7c023c3 curl#15384 Closes curl#15416

The `#undef` hack is no longer necessary after changing the redifitions to not map back to the original symbols. This makes it unnecessary to repeat the redefinitions after compiling `warnless.c` itself (in unity mode). Which in turns makes it unnecessary to include `warnless.h` again, to trigger such redefinition. This also means that `read`/`write` are now redefined on Windows from the first inclusion of `warnless.h`. Also: - tests/server: drop a repeat `warnless.h` include, that is unnecessary after this patch. - tests/unit: drop repeat `warnless.h` include. - tests/libtest: drop repeat `warnless.h` includes. - tests/libtest: formatting. Follow-up to 2f312a1 #17619 Follow-up to 84338c4 #12331 Follow-up to 6239146 Closes #17673

The `#undef` hack is no longer necessary after changing the redifitions to not map back to the original symbols. This makes it unnecessary to repeat the redefinitions after compiling `warnless.c` itself (in unity mode). Which in turns makes it unnecessary to include `warnless.h` again, to trigger such redefinition. This also means that `read`/`write` are now redefined on Windows from the first inclusion of `warnless.h`. Also: - tests/server: drop a repeat `warnless.h` include, that is unnecessary after this patch. - tests/unit: drop repeat `warnless.h` include. - tests/libtest: drop repeat `warnless.h` includes. - tests/libtest: formatting. Follow-up to 2f312a1 curl#17619 Follow-up to 84338c4 curl#12331 Follow-up to 6239146 Closes curl#17673

vszakats marked this pull request as draft November 15, 2023 14:44

github-actions bot added build cmake tests cmdline tool labels Nov 15, 2023

vszakats force-pushed the more-picky-from-ng branch 2 times, most recently from 97879ac to c07543c Compare November 16, 2023 03:44

github-actions bot added the CI Continuous Integration label Nov 16, 2023

vszakats force-pushed the more-picky-from-ng branch 2 times, most recently from 38a1b0e to ab3cf32 Compare November 16, 2023 05:31

This comment was marked as outdated.

Sign in to view

vszakats changed the title ~~cmake: test more picky warnings [WIP]~~ cmake: test more picky warnings] Nov 16, 2023

vszakats changed the title ~~cmake: test more picky warnings]~~ cmake: test more picky warnings Nov 16, 2023

vszakats force-pushed the more-picky-from-ng branch 4 times, most recently from bccb316 to e2a8170 Compare November 16, 2023 13:34

This comment was marked as resolved.

Sign in to view

vszakats force-pushed the more-picky-from-ng branch 2 times, most recently from dcc309f to 3019e33 Compare November 17, 2023 17:59

This comment was marked as resolved.

Sign in to view

vszakats force-pushed the more-picky-from-ng branch from 975904b to a5ee38b Compare November 17, 2023 22:16

This comment was marked as resolved.

Sign in to view

vszakats changed the title ~~cmake: test more picky warnings~~ build: add more picky warnings and fix them Nov 17, 2023

vszakats force-pushed the more-picky-from-ng branch from a5ee38b to e68feb8 Compare November 18, 2023 01:33

vszakats marked this pull request as ready for review November 18, 2023 02:00

This comment was marked as resolved.

Sign in to view

vszakats force-pushed the more-picky-from-ng branch 2 times, most recently from 1f2c59c to b98b600 Compare November 18, 2023 04:00

vszakats added 14 commits November 20, 2023 22:31

tests/lib2305: delete duplicate declaration for libtest_debug_config

b7d00ee

tests/h2-download: fix -Wunreachable-code-break

aacfef4

hostip: add noreturn function attribute via new CURL_NORETURN macro

f4ede8a

setopt: fix -Wunreachable-code-break

61d99b8

vssh/libssh2: fix -Wunreachable-code-break and delete redundant block

aaa0906

curl_setup: delete duplicate declaration for fileno

77c9577

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.

lib/system_win32: fix double declarations for Curl_freq and `Curl_i…

9dc519d

…sVistaOrGreater` in unity builds.

lib/warnless: fix double declarations with cmake unity

bc42c89

lib/timeval: delete redundant guard (MSDOS never true when WIN32)

a541f73

autotools: formatting

63d714e

lib/warnless: tidy-up

02298a3

vszakats force-pushed the more-picky-from-ng branch from e58c761 to 02298a3 Compare November 20, 2023 22:35

vszakats closed this in 84338c4 Nov 21, 2023

vszakats deleted the more-picky-from-ng branch November 21, 2023 16:38

vszakats mentioned this pull request Dec 2, 2023

test1545 fails to compile in an AppVeyor CI job with [-Werror=deprecated-declarations] #12445

Closed

vszakats added a commit to vszakats/curl that referenced this pull request Jul 12, 2024

sectransp: try re-enabling -Wunreachable-code with gcc

ff8e05b

Follow-up to 84338c4 curl#12331

vszakats mentioned this pull request Oct 25, 2024

build: disable warning -Wunreachable-code-break #15416

Closed

vszakats mentioned this pull request Jun 19, 2025

warnless: drop parts of the read/write preprocessor hack (Windows) #17673

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

build: add more picky warnings and fix them#12331

build: add more picky warnings and fix them#12331
vszakats wants to merge 33 commits intocurl:masterfrom
vszakats:more-picky-from-ng

vszakats commented Nov 15, 2023 •

edited

Loading

Uh oh!

This comment was marked as outdated.

This comment was marked as resolved.

vszakats commented Nov 17, 2023

Uh oh!

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

1 participant

Uh oh!

Conversation

vszakats commented Nov 15, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment was marked as outdated.

This comment was marked as resolved.

vszakats commented Nov 17, 2023

Uh oh!

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

1 participant

vszakats commented Nov 15, 2023 •

edited

Loading