Skip to content

clang-tidy: add to CI, add cmake support, fix fallouts#15825

Closed
vszakats wants to merge 78 commits intocurl:masterfrom
vszakats:gha-clang-tidy
Closed

clang-tidy: add to CI, add cmake support, fix fallouts#15825
vszakats wants to merge 78 commits intocurl:masterfrom
vszakats:gha-clang-tidy

Conversation

@vszakats
Copy link
Member

@vszakats vszakats commented Dec 24, 2024

build:

  • autotools: fix to build generated sources for the tidy target.
  • autotools: allow passing custom clang-tidy options via
    CURL_CLANG_TIDYFLAGS env.
  • cmake: add CURL_CLANG_TIDY option to configure for clang-tidy.
    Also add:
    • CLANG_TIDY variable to customize the clang-tidy tool.
    • CURL_CLANG_TIDYFLAGS to pass custom options to clang-tidy.
  • apply --enable-werror and -DCURL_WERROR=ON to clang-tidy.

CI/GHA:

  • add clang-tidy job for Linux, using autotools and clang-tidy v18.
    This one needs to disable clang-analyzer-valist.Uninitialized
    to avoid false positives:
    clang-analyzer-valist.Uninitialized warning on use of a copied valist llvm/llvm-project#40656
    Duration: 5.5 minutes
  • add clang-tidy job for macOS, using cmake and clang-tidy v19.
    This one also covers tests and examples, and doesn't hit the false
    positives seen with llvm v18 and earlier.
    Duration: 4.5 minutes
  • Linux/macOS: skip installing test dependencies when not building or
    running tests.

fix fallouts reported by clang-tidy:

  • lib:
  • src/tool_writeout: NULL passed to fputs().
  • examples:
    • invalid file pointers.
    • missing fclose().
  • tests:
    • http/clients/hx-download: memory leaks on error.
    • http/clients/hx-download: memory leak on repeat -r option.
    • server: double fclose().
      https://www.man7.org/linux/man-pages/man3/fclose.3.html
    • server: invalid file pointer/handle.
    • server/getpart: unused assignments.
    • server/mqttd: leak on failed realloc().
    • server/tftpd: NULL passed to strcmp().

  • move CMake to separate PR? [NO]
  • add CMake clang-tidy job? [DONE]
  • suppress clang-analyzer-valist.Uninitialized per-file per-line, not globally? [DONE for GHA/linux]
  • move clang-tidy to a single macOS job.

@vszakats vszakats added build CI Continuous Integration labels Dec 24, 2024
@vszakats vszakats marked this pull request as draft December 24, 2024 01:47
@vszakats vszakats force-pushed the gha-clang-tidy branch 2 times, most recently from d5e851b to 86d411f Compare December 24, 2024 11:30
@vszakats
Copy link
Member Author

vszakats commented Dec 24, 2024

There are lots of clang-analyzer-valist.Uninitialized false-positive warnings
(in tool_setopt, getinfo, multi, setopt and share) due to this clang-tidy issue:
llvm/llvm-project#40656

I re-excluded it to avoid the log noise, but it's possible to just have them
there, or to suppress via /* NOLINT */ or NOLINTBEGIN/NOLINTEND
in source.

Ref: https://clang.llvm.org/extra/clang-tidy/#suppressing-undesired-diagnostics

@vszakats vszakats changed the title GHA/linux: try LTO and clang-tidy GHA/linux: add clang-tidy job and enable LTO in another Dec 24, 2024
@vszakats vszakats added the tests label Dec 24, 2024
@vszakats vszakats changed the title GHA/linux: add clang-tidy job and enable LTO in another GHA/linux: add clang-tidy job Dec 24, 2024
@vszakats vszakats changed the title GHA/linux: add clang-tidy job GHA/linux: add clang-tidy job, add cmake support, fix fallouts Dec 24, 2024
@vszakats vszakats marked this pull request as ready for review December 24, 2024 13:46
@dfandrich
Copy link
Contributor

dfandrich commented Dec 24, 2024 via email

@bagder
Copy link
Member

bagder commented Dec 24, 2024

There are lots of clang-analyzer-valist.Uninitialized false-positive warnings

Yeah, we used to disable that warning before. I used clang-tidy 19.1.6 locally and I did not see it using this version anymore.

@vszakats
Copy link
Member Author

llvm 10 is available on the Ubuntu 20.04 runner, but also affected by the bug:
https://github.com/curl/curl/actions/runs/12488068561/job/34850007591

On macOS llvm@11 is not installable, but there is llvm 19, which
indeed seems to have the valist false-positives fixed.

One option is to limit clang-tidy to macOS. It uses CMake there,
which runs it for tests and examples too.

For keeping the Linux job, we have these workaround options:

  • disable the warning globally. (easy, but risks missing valid valist issues)
  • disable per-file or per switch block. (almost as easy, with risk of missing valid issues in excluded code)
  • disable it per line. (as committed ATM) (not pretty, but avoids the risk of missing valid issues.)

@vszakats
Copy link
Member Author

This clang-tidy warning remains:

/Users/runner/work/curl/curl/lib/cf-socket.c:1853:8: warning: The 1st argument to 'connect' is -1 but should be >= 0 [clang-analyzer-unix.StdCLibraryFunctions]
 1853 |   rc = connect(ctx->sock, &ctx->addr.curl_sa_addr,
      |        ^

https://github.com/curl/curl/actions/runs/12492756056/job/34860387302#step:11:33

It's caught by DEBUGASSERT() in debug builds, but not otherwise,
and according to clang-tidy, it may happen.

@vszakats vszakats changed the title GHA/linux: add clang-tidy job, add cmake support, fix fallouts GHA: add clang-tidy jobs, add cmake support, fix fallouts Dec 25, 2024
@vszakats
Copy link
Member Author

OK, I kept the Linux job and silenced the false-positives with
custom global option for the CI only.

I silenced the cf-socket issue with NOLINT and FIXME.
This needs a true fix.
#15825 (comment)

tests/server/rtspd.c could also probably use a better fix than the
rough one I committed.

I'd appreciate a review for the all warning fixes.

Other than these, this seems ready to merge.

@vszakats vszakats changed the title GHA: add clang-tidy jobs, add cmake support, fix fallouts clang-tidy: add to CI, add cmake support, fix fallouts Dec 26, 2024
@bagder
Copy link
Member

bagder commented Dec 26, 2024

disable the warning globally. (easy, but risks missing valid valist issues)

I think that's fine, We ran many other tools as well that can discover such problems, plus we have never actually had a real such valist problem AFAICR.

```
Error while processing /home/runner/work/curl/curl/src/var.c.
[44/44] Processing file /home/runner/work/curl/curl/src/tool_ca_embed.c.
Error while processing /home/runner/work/curl/curl/src/tool_ca_embed.c.
error: no input files [clang-diagnostic-error]
error: no such file or directory: '/home/runner/work/curl/curl/src/tool_ca_embed.c' [clang-diagnostic-error]
error: no such file or directory: '/home/runner/work/curl/curl/src/tool_hugehelp.c' [clang-diagnostic-error]
error: unable to handle compilation, expected exactly one compiler job in '' [clang-diagnostic-error]
```
https://github.com/curl/curl/actions/runs/12480426541/job/34831254945?pr=15825#step:35:108
@vszakats
Copy link
Member Author

I'm merging this. valist warnings are kept by default and only disabled globally in Linux CI where hitting the pre-19 llvm.

Also the FIXME remains, ref #15825 (comment).

@vszakats vszakats closed this in fabfa8e Dec 27, 2024
@vszakats vszakats deleted the gha-clang-tidy branch December 27, 2024 12:43
vszakats added a commit to vszakats/curl that referenced this pull request Mar 17, 2025
Seen when running the macOS clang-tidy job with test bundles disabled:
```
tests/libtest/libauthretry.c:96:12: error: The value '126' provided to the cast
  expression is not in the valid range of values for the enum
  [clang-analyzer-optin.core.EnumCastOutOfRange,-warnings-as-errors]
   96 |     return TEST_ERR_MAJOR_BAD;
      |            ^
tests/libtest/test.h:99:32: note: expanded from macro 'TEST_ERR_MAJOR_BAD'
   99 | #define TEST_ERR_MAJOR_BAD     (CURLcode) 126
      |                                ^~~~~~~~~~~~~~
include/curl/curl.h:519:9: note: enum declared here
[...]
```
Ref: https://github.com/curl/curl/actions/runs/13909934306/job/38921798619?pr=16750#step:14:384

Follow-up to fabfa8e curl#15825
vszakats added a commit to vszakats/curl that referenced this pull request Mar 18, 2025
Seen when running the macOS clang-tidy job with test bundles disabled:
```
tests/libtest/libauthretry.c:96:12: error: The value '126' provided to the cast
  expression is not in the valid range of values for the enum
  [clang-analyzer-optin.core.EnumCastOutOfRange,-warnings-as-errors]
   96 |     return TEST_ERR_MAJOR_BAD;
      |            ^
tests/libtest/test.h:99:32: note: expanded from macro 'TEST_ERR_MAJOR_BAD'
   99 | #define TEST_ERR_MAJOR_BAD     (CURLcode) 126
      |                                ^~~~~~~~~~~~~~
include/curl/curl.h:519:9: note: enum declared here
[...]
```
Ref: https://github.com/curl/curl/actions/runs/13909934306/job/38921798619?pr=16750#step:14:384

Follow-up to fabfa8e curl#15825
vszakats added a commit to vszakats/curl that referenced this pull request Mar 18, 2025
Seen when running the macOS clang-tidy job with test bundles disabled:
```
tests/libtest/libauthretry.c:96:12: error: The value '126' provided to the cast
  expression is not in the valid range of values for the enum
  [clang-analyzer-optin.core.EnumCastOutOfRange,-warnings-as-errors]
   96 |     return TEST_ERR_MAJOR_BAD;
      |            ^
tests/libtest/test.h:99:32: note: expanded from macro 'TEST_ERR_MAJOR_BAD'
   99 | #define TEST_ERR_MAJOR_BAD     (CURLcode) 126
      |                                ^~~~~~~~~~~~~~
include/curl/curl.h:519:9: note: enum declared here
[...]
```
Ref: https://github.com/curl/curl/actions/runs/13909934306/job/38921798619?pr=16750#step:14:384

Follow-up to fabfa8e curl#15825
vszakats added a commit that referenced this pull request Mar 24, 2025
- cmake: disable test bundles for clang-tidy builds.
  clang-tidy ignores #included .c sources, and incompatible with unity
  and bundles. It caused clang-tidy ignoring all test sources. It also
  means this is the first time tests sources are checked with
  clang-tidy. (autotools doesn't run it on tests.)

- cmake: update description for `CURL_TEST_BUNDLES` option.

- fix tests using special `CURLE_*` enums that were missing from
  `curl/curl.h`. Add them as reserved codes.

- fix about ~50 other issues detected by clang-tidy: unchecked results,
  NULL derefs, memory leaks, casts to enums, unused assigments,
  uninitialized `errno` uses, unchecked `open`, indent, and more.

- drop unnecessary casts (lib1533, lib3207).

- suppress a few impossible cases with detailed `NOLINT`s.

- lib/escape.c: drop `NOLINT` no longer necessary.
  Follow-up to 72abf7c #13862 (possibly)

- extend two existing `NOLINT` comments with details.

Follow-up to fabfa8e #15825

Closes #16756
pps83 pushed a commit to pps83/curl that referenced this pull request Apr 26, 2025
build:
- autotools: fix to build generated sources for the `tidy` target.
- autotools: allow passing custom clang-tidy options via
  `CURL_CLANG_TIDYFLAGS` env.
- cmake: add `CURL_CLANG_TIDY` option to configure for `clang-tidy`.
  Also add:
  - `CLANG_TIDY` variable to customize the `clang-tidy` tool.
  - `CURL_CLANG_TIDYFLAGS` to pass custom options to `clang-tidy`.
- apply `--enable-werror` and `-DCURL_WERROR=ON` to `clang-tidy`.

CI/GHA:
- add clang-tidy job for Linux, using autotools and clang-tidy v18.
  This one needs to disable `clang-analyzer-valist.Uninitialized`
  to avoid false positives:
  llvm/llvm-project#40656
  Duration: 5.5 minutes
- add clang-tidy job for macOS, using cmake and clang-tidy v19.
  This one also covers tests and examples, and doesn't hit the false
  positives seen with llvm v18 and earlier.
  Duration: 4.5 minutes
- Linux/macOS: skip installing test dependencies when not building or
  running tests.

fix fallouts reported by `clang-tidy`:
- lib:
  - cf-h2-proxy: unused assignment in non-debug builds.
  - cf-socket: silence warning.
    FIXME: curl#15825 (comment)
  - ftp: NULL passed to `strncmp()`.
  - http2: NULL-ptr deref.
  - mprintf: silence warning.
- src/tool_writeout: NULL passed to `fputs()`.
- examples:
  - invalid file pointers.
  - missing `fclose()`.
- tests:
  - http/clients/hx-download: memory leaks on error.
  - http/clients/hx-download: memory leak on repeat `-r` option.
  - server: double `fclose()`.
    https://www.man7.org/linux/man-pages/man3/fclose.3.html
  - server: invalid file pointer/handle.
  - server/getpart: unused assignments.
  - server/mqttd: leak on failed `realloc()`.
  - server/tftpd: NULL passed to `strcmp()`.

Closes curl#15825
pps83 pushed a commit to pps83/curl that referenced this pull request Apr 26, 2025
- cmake: disable test bundles for clang-tidy builds.
  clang-tidy ignores #included .c sources, and incompatible with unity
  and bundles. It caused clang-tidy ignoring all test sources. It also
  means this is the first time tests sources are checked with
  clang-tidy. (autotools doesn't run it on tests.)

- cmake: update description for `CURL_TEST_BUNDLES` option.

- fix tests using special `CURLE_*` enums that were missing from
  `curl/curl.h`. Add them as reserved codes.

- fix about ~50 other issues detected by clang-tidy: unchecked results,
  NULL derefs, memory leaks, casts to enums, unused assigments,
  uninitialized `errno` uses, unchecked `open`, indent, and more.

- drop unnecessary casts (lib1533, lib3207).

- suppress a few impossible cases with detailed `NOLINT`s.

- lib/escape.c: drop `NOLINT` no longer necessary.
  Follow-up to 72abf7c curl#13862 (possibly)

- extend two existing `NOLINT` comments with details.

Follow-up to fabfa8e curl#15825

Closes curl#16756
vszakats added a commit that referenced this pull request Jun 21, 2025
Skip clang-tidy while compiling curlu and curltool internal libraries.
To save about 1 minute per run. These libraries compile the lib and src
sources a second time, with the `UNITTESTS` macro enabled, which makes
tiny difference, for internal use. I figure it's not worth the extra CI
(and local) time because finding extra issues in these passes is
unlikely, and if found, not critical.

autotools also doesn't check curlu and curltool with clang-tidy.

Ref: #17680 (comment)
Ref: https://stackoverflow.com/questions/61867616/ignore-certain-files-when-using-clang-tidy
Ref: https://cmake.org/cmake/help/latest/prop_tgt/LANG_CLANG_TIDY.html

Follow-up to fabfa8e #15825

Closes #17693
denandz pushed a commit to denandz/curl that referenced this pull request Jun 21, 2025
Skip clang-tidy while compiling curlu and curltool internal libraries.
To save about 1 minute per run. These libraries compile the lib and src
sources a second time, with the `UNITTESTS` macro enabled, which makes
tiny difference, for internal use. I figure it's not worth the extra CI
(and local) time because finding extra issues in these passes is
unlikely, and if found, not critical.

autotools also doesn't check curlu and curltool with clang-tidy.

Ref: curl#17680 (comment)
Ref: https://stackoverflow.com/questions/61867616/ignore-certain-files-when-using-clang-tidy
Ref: https://cmake.org/cmake/help/latest/prop_tgt/LANG_CLANG_TIDY.html

Follow-up to fabfa8e curl#15825

Closes curl#17693
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.

3 participants