clang-tidy: add to CI, add cmake support, fix fallouts#15825
clang-tidy: add to CI, add cmake support, fix fallouts#15825vszakats wants to merge 78 commits intocurl:masterfrom
Conversation
d5e851b to
86d411f
Compare
|
There are lots of I re-excluded it to avoid the log noise, but it's possible to just have them Ref: https://clang.llvm.org/extra/clang-tidy/#suppressing-undesired-diagnostics |
db72a9c to
5f4eadd
Compare
35dcb20 to
7b19419
Compare
e7bc420 to
a053b97
Compare
|
Or, perhaps use an old version of clang-tidy? The bug report states 12–18 has
the problem, but it's not actually stated that <=11 doesn't have it.
|
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. |
|
llvm 10 is available on the Ubuntu 20.04 runner, but also affected by the bug: On macOS llvm@11 is not installable, but there is llvm 19, which One option is to limit clang-tidy to macOS. It uses CMake there, For keeping the Linux job, we have these workaround options:
|
b25c78a to
957f069
Compare
|
This clang-tidy warning remains: https://github.com/curl/curl/actions/runs/12492756056/job/34860387302#step:11:33 It's caught by |
|
OK, I kept the Linux job and silenced the false-positives with I silenced the
I'd appreciate a review for the all warning fixes. Other than these, this seems ready to merge. |
b8225ce to
20a3b0a
Compare
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
cannot pass multiple -check options additively, so pass all default options plus our custom one.
20a3b0a to
7a03c49
Compare
|
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). |
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
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
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
- 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
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
- 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
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
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
build:
tidytarget.CURL_CLANG_TIDYFLAGSenv.CURL_CLANG_TIDYoption to configure forclang-tidy.Also add:
CLANG_TIDYvariable to customize theclang-tidytool.CURL_CLANG_TIDYFLAGSto pass custom options toclang-tidy.--enable-werrorand-DCURL_WERROR=ONtoclang-tidy.CI/GHA:
This one needs to disable
clang-analyzer-valist.Uninitializedto avoid false positives:
clang-analyzer-valist.Uninitialized warning on use of a copied valist llvm/llvm-project#40656
Duration: 5.5 minutes
This one also covers tests and examples, and doesn't hit the false
positives seen with llvm v18 and earlier.
Duration: 4.5 minutes
running tests.
fix fallouts reported by
clang-tidy:FIXME: clang-tidy: add to CI, add cmake support, fix fallouts #15825 (comment)
strncmp().fputs().fclose().-roption.fclose().https://www.man7.org/linux/man-pages/man3/fclose.3.html
realloc().strcmp().clang-tidyjob? [DONE]clang-analyzer-valist.Uninitializedper-fileper-line, not globally? [DONE for GHA/linux]