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
