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

build: add more picky warnings and fix them #12331

Closed
wants to merge 33 commits into from

Conversation

vszakats
Copy link
Member

@vszakats vszakats commented Nov 15, 2023

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 vszakats marked this pull request as draft November 15, 2023 14:44
@vszakats 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 github-actions bot added the CI Continuous Integration label Nov 16, 2023
@vszakats vszakats force-pushed the more-picky-from-ng branch 2 times, most recently from 38a1b0e to ab3cf32 Compare November 16, 2023 05:31
@vszakats

This comment was marked as outdated.

@vszakats vszakats changed the title cmake: test more picky warnings [WIP] cmake: test more picky warnings] Nov 16, 2023
@vszakats vszakats changed the title cmake: test more picky warnings] cmake: test more picky warnings Nov 16, 2023
@vszakats vszakats force-pushed the more-picky-from-ng branch 4 times, most recently from bccb316 to e2a8170 Compare November 16, 2023 13:34
@vszakats

This comment was marked as resolved.

@vszakats
Copy link
Member Author

Will continue with this after merging #12346.

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

This comment was marked as resolved.

@vszakats

This comment was marked as resolved.

@vszakats vszakats changed the title cmake: test more picky warnings build: add more picky warnings and fix them Nov 17, 2023
@vszakats vszakats marked this pull request as ready for review November 18, 2023 02:00
@vszakats

This comment was marked as resolved.

@vszakats vszakats force-pushed the more-picky-from-ng branch 2 times, most recently from 1f2c59c to b98b600 Compare November 18, 2023 04:00
```
lib/rand.c:151:7: error: code will never be executed [-Werror,-Wunreachable-code]
  if(!seeded) {
      ^~~~~~
```
Ref: https://github.com/curl/curl/actions/runs/6880886309/job/18716044703?pr=12331#step:7:72

Including fixing these fallouts:
https://github.com/curl/curl/actions/runs/6883016087/job/18722707368?pr=12331#step:7:109
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

None yet

1 participant