8:07 a.m.
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#L... 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=changedd...
--
Cheers,
Tim