On Thu, 2021-01-28 at 10:35 +0000, Daniel P. Berrangé wrote: > On Thu, Jan 28, 2021 at 11:24:30AM +0100, Tim Wiederhake wrote: > > clang-tidy is a static code analysis tool under the llvm umbrella. > > It is > > primarily meant to be used on C++ code bases, but some of the > > checks it > > provides also apply to C. > > > > The findings vary in severity and contain pseudo-false-positives, > > i.e. > > clang-tidy is flagging potential execution flows that could happen > > in > > theory but are virtually impossible in real life: In function > > `virGetUnprivSGIOSysfsPath`, variables `maj` and `min` would be > > read > > unintialized if `stat()` failed and set `errno` to a negative > > value, to name > > just one example. > > > > The main source of false positive findings is the lack of support > > for > > `__attribute__((cleanup))` in clang-tidy, which is heavily used in > > libvirt > > through glib's `g_autofree` and `g_auto()` macros: > > > > #include <stdlib.h> > > > > void freeptr(int** p) { > > if (*p) > > free(*p); > > } > > > > int main() { > > __attribute__((cleanup(freeptr))) int *ptr = NULL; > > ptr = calloc(sizeof(int), 1); > > return 0; /* flagged as memory leak of `ptr` */ > > } > > > > This sadly renders clang-tidy's analysis of dynamic memory useless, > > hiding all > > real issues that it could otherwise find. > > > > Meson provides excellent integration for clang-tidy (a "clang-tidy" > > target is > > automatically generated if a ".clang-tidy" configuration file is > > present > > in the project's root directory). The amount of false-positives and > > the slow > > analysis, triggering time-outs in the CI, make this tool unfit for > > inclusion > > in libvirt's GitLab CI though. > > Is it possible to make it viable for CI by disabling *all* checks by > default and then selectively re-enabling just the handful that are > useful and don't have false positives ? > > Regards, > Daniel
That is what I tried at first. Unfortunately, it does not work for several reasons: * clang-tidy does not like memory constraint environments -- at least that appears to be the bottom of it. The result is random segfaults. * There are false positive findings in the group of "clang-analyze-*" and "clang-diagnostic-*", which cannot be disabled or rather, are implicitly re-enabled no matter your configuration. E.g: Flagging https://gitlab.com/libvirt/libvirt/-/blob/master/src/util/virfdstream.c#L1240 as dead store, see above explation of "__attribute__((cleanup))" for background. * There are true positive findings in the same group of checks that I, frankly, just disagree with. E.g: Flagging https://gitlab.com/libvirt/libvirt/-/blob/master/src/util/virobject.c#L228 as tautological comparison (in the first iteration of the `while` loop), as `klass` is declared ATTRIBUTE_NONNULL. "Unrolling" the first iteration does not make the code better in my eyes. "no-lint" markers for clang-tidy do exist, but I would rather not litter the code with them to make some software happy. * clang-tidy requires all files to be present, or the run will terminate with a non-zero return code and analysis errors. This includes generated files. My Meson-fu is not particularly great, and I have not found a "run generators only" target. Building libvirt completely before running clang-tidy cuts badly in gitlab's one-hour time budget, and running the monster below, for which I would like to profoundly apologize, is also not a future-proof option: [ -d "${bld_dir}" ] || CC=clang meson "${bld_dir}" "${src_dir}" ninja -C "${bld_dir}" \ src/access/viraccessapicheck.c \ src/access/viraccessapicheck.h \ src/access/viraccessapichecklxc.c \ src/access/viraccessapichecklxc.h \ src/access/viraccessapicheckqemu.c \ src/access/viraccessapicheckqemu.h \ src/admin/admin_client.h \ src/admin/admin_protocol.c \ src/admin/admin_protocol.h \ src/admin/admin_server_dispatch_stubs.h \ src/esx/esx_vi.generated.c \ src/esx/esx_vi.generated.h \ src/esx/esx_vi_methods.generated.c \ src/esx/esx_vi_methods.generated.h \ src/esx/esx_vi_methods.generated.macro \ src/esx/esx_vi_types.generated.c \ src/esx/esx_vi_types.generated.h \ src/esx/esx_vi_types.generated.typedef \ src/esx/esx_vi_types.generated.typeenum \ src/esx/esx_vi_types.generated.typefromstring \ src/esx/esx_vi_types.generated.typetostring \ src/hyperv/hyperv_wmi_classes.generated.c \ src/hyperv/hyperv_wmi_classes.generated.h \ src/hyperv/hyperv_wmi_classes.generated.typedef \ src/libvirt_functions.stp \ src/libvirt_probes.h \ src/libvirt_probes.stp \ src/locking/lock_daemon_dispatch_stubs.h \ src/locking/lock_protocol.c \ src/locking/lock_protocol.h \ src/logging/log_daemon_dispatch_stubs.h \ src/logging/log_protocol.c \ src/logging/log_protocol.h \ src/lxc/lxc_controller_dispatch.h \ src/lxc/lxc_monitor_dispatch.h \ src/lxc/lxc_monitor_protocol.c \ src/lxc/lxc_monitor_protocol.h \ src/qemu/libvirt_qemu_probes.h \ src/qemu/libvirt_qemu_probes.stp \ src/remote/lxc_client_bodies.h \ src/remote/lxc_daemon_dispatch_stubs.h \ src/remote/lxc_protocol.c \ src/remote/lxc_protocol.h \ src/remote/qemu_client_bodies.h \ src/remote/qemu_daemon_dispatch_stubs.h \ src/remote/qemu_protocol.c \ src/remote/qemu_protocol.h \ src/remote/remote_client_bodies.h \ src/remote/remote_daemon_dispatch_stubs.h \ src/remote/remote_protocol.c \ src/remote/remote_protocol.h \ src/rpc/virkeepaliveprotocol.c \ src/rpc/virkeepaliveprotocol.h \ src/rpc/virnetprotocol.c \ src/rpc/virnetprotocol.h \ src/util/virkeycodetable_atset1.h \ src/util/virkeycodetable_atset2.h \ src/util/virkeycodetable_atset3.h \ src/util/virkeycodetable_linux.h \ src/util/virkeycodetable_osx.h \ src/util/virkeycodetable_qnum.h \ src/util/virkeycodetable_usb.h \ src/util/virkeycodetable_win32.h \ src/util/virkeycodetable_xtkbd.h \ src/util/virkeynametable_linux.h \ src/util/virkeynametable_osx.h \ src/util/virkeynametable_win32.h \ tools/wireshark/src/libvirt/keepalive.h \ tools/wireshark/src/libvirt/lxc.h \ tools/wireshark/src/libvirt/protocol.h \ tools/wireshark/src/libvirt/qemu.h \ tools/wireshark/src/libvirt/remote.h ninja -C "${bld_dir}" clang-tidy In my opinion, it might be worthwile to have a `.clang-tidy` configuration for libvirt, but running the tests is up to humans for the forseeable future, not the CI. At the very least until clang-tidy understands `__attribute__(cleanup))`, which I would love to report as a bug to clang-tidy, but the disabled "self user registration" and the amount of stale bugs that have not seen any attention in the last five years is not giving raise to hope: https://bugs.llvm.org/buglist.cgi?component=clang-tidy&order=changeddate%2Cpriority%2Cbug_severity&product=clang-tools-extra&query_format=advanced&resolution=- -- Cheers, Tim