Re: [RFC v2 00/10] Introduce an extensible static analyzer

[Marc-André Lureau]

Hi

On Fri, Jul 29, 2022 at 5:01 PM Alberto Faria <afaria@redhat.com> wrote:
>
> This series introduces a static analyzer for QEMU. It consists of a
> single static-analyzer.py script that relies on libclang's Python
> bindings, and provides a common framework on which arbitrary static
> analysis checks can be developed and run against QEMU's code base.
>
> Summary of the series:
>
>   - Patch 1 adds the base static analyzer, along with a simple check
>     that finds static functions whose return value is never used, and
>     patch 2 fixes many occurrences of this.
>
>   - Patch 3 introduces support for output-comparison check tests, and
>     adds some tests to the abovementioned check.
>
>   - Patch 4 makes the analyzer skip checks on a translation unit when it
>     hasn't been modified since the last time those checks passed.
>
>   - Patch 5 adds a check to ensure that non-coroutine_fn functions don't
>     perform direct calls to coroutine_fn functions, and patch 6 fixes
>     some violations of this rule.
>
>   - Patch 7 adds a check to ensure that operations on coroutine_fn
>     pointers make sense, like assignment and indirect calls, and patch 8
>     fixes some problems detected by the check. (Implementing this check
>     properly is complicated, since AFAICT annotation attributes cannot
>     be applied directly to types. This part still needs a lot of work.)
>
>   - Patch 9 introduces a no_coroutine_fn marker for functions that
>     should not be called from coroutines, makes generated_co_wrapper
>     evaluate to no_coroutine_fn, and adds a check enforcing this rule.
>     Patch 10 fixes some violations that it finds.
>
> The current primary motivation for this work is enforcing rules around
> block layer coroutines, which is why most of the series focuses on that.
> However, the static analyzer is intended to be sufficiently generic to
> satisfy other present and future QEMU static analysis needs.
>
> Performance isn't great, but with some more optimization, the analyzer
> should be fast enough to be used iteratively during development, given
> that it avoids reanalyzing unmodified translation units, and that users
> can restrict the set of translation units under consideration. It should
> also be fast enough to run in CI (?).
>
> Consider a small QEMU configuration and build (all commands were run on
> the same 12-thread laptop):
>
>     $ cd build && time ../configure --target-list=x86_64-softmmu && cd ..
>     [...]
>
>     real    0m17.232s
>     user    0m13.261s
>     sys     0m3.895s
>
>     $ time make -C build -j $(nproc) all
>     [...]
>
>     real    2m39.029s
>     user    14m49.370s
>     sys     1m57.364s
>
>     $ time make -C build -j $(nproc) check
>     [...]
>
>     real    2m46.349s
>     user    6m4.718s
>     sys     4m15.660s
>
> We can run the static analyzer against all translation units enabled in
> this configuration:
>
>     $ time ./static-analyzer.py build
>     util/qemu-coroutine.c:122:23: non-coroutine_fn function calls 
> coroutine_fn qemu_coroutine_self()
>     io/channel.c:152:17: non-coroutine_fn function calls coroutine_fn 
> qio_channel_yield()
>     [...]
>     Analyzed 1649 translation units in 520.3 seconds.
>
>     real    8m42.342s
>     user    95m51.759s
>     sys     0m21.576s
>
> You will need libclang's Python bindings to run this. Try `dnf install
> python3-clang` or `apt install python3-clang`.
>
> It takes around 1 to 2 seconds for the analyzer to load the compilation
> database, determine which translation units to analyze, etc. The
> durations reported by the analyzer itself don't include those steps,
> which is why they differ from what `time` reports.
>
> We can also analyze only some of the translation units:
>
>     $ time ./static-analyzer.py build block
>     block/raw-format.c:420:12: non-coroutine_fn function calls coroutine_fn 
> bdrv_co_ioctl()
>     block/blkverify.c:266:12: non-coroutine_fn function calls coroutine_fn 
> bdrv_co_flush()
>     [...]
>     Analyzed 21 translation units (58 other were up-to-date) in 5.8 seconds.
>
>     real    0m7.031s
>     user    0m40.951s
>     sys     0m1.299s
>
> Since the previous command had already analyzed all translation units,
> only the ones that had problems were reanalyzed.
>
> Now skipping all the actual checks, but still parsing and building the
> AST for each translation unit, and adding --force to reanalyze all
> translation units:
>
>     $ time ./static-analyzer.py build --force --skip-checks
>     Analyzed 1649 translation units in 41.2 seconds.
>
>     real    0m42.296s
>     user    7m14.256s
>     sys     0m15.803s
>
> And now running a single check:
>
>     $ time ./static-analyzer.py build --force --check return-value-never-used
>     Analyzed 1649 translation units in 157.6 seconds.
>
>     real    2m38.759s
>     user    29m28.930s
>     sys     0m17.968s
>
> TODO:
>   - Run in GitLab CI (?).
>   - Finish the "coroutine_fn" check.
>   - Add check tests where missing.
>   - Avoid redundant AST traversals while keeping checks modular.
>   - More optimization.

Great work so far! This seems easier to hack than my attempt to use
clang-tidy to write some qemu checks
(https://github.com/elmarco/clang-tools-extra)

The code seems quite generic, I wonder if such a tool in python wasn't
already developed (I couldn't find it easily searching on github).

Why not make it standalone from qemu? Similar to
https://gitlab.com/qemu-project/python-qemu-qmp, you could have your
own release management, issue tracker, code formatting, license, CI
etc. (you should add copyright header in each file, at least that's
pretty much required in qemu nowadays). You could also have the
qemu-specific checks there imho (clang-tidy has google & llvm specific
checks too)

It would be nice to write some docs, in docs/devel/testing.rst and
some new meson/ninja/make targets to run the checks directly from a
build tree.

On fc36, I had several dependencies I needed to install manually (imho
they should have been pulled by python3-clang), but more annoyingly I
got:
clang.cindex.LibclangError: libclang.so: cannot open shared object
file: No such file or directory. To provide a path to libclang use
Config.set_library_path() or Config.set_library_file().

clang-libs doesn't install libclang.so, I wonder why. I made a link
manually and it works, but it's probably incorrect. I'll try to open
issues for the clang packaging.

cheers


>
> v2:
>   - Fix parsing of compilation database commands.
>   - Reorganize checks and split them into separate modules.
>   - Make "return-value-never-used" ignore __attribute__((unused)) funcs.
>   - Add a visitor() abstraction wrapping clang_visitChildren() that is
>     faster than using Cursor.get_children() with recursion.
>   - Add support for implementing tests for checks, and add some tests to
>     "return-value-never-used".
>   - Use dependency information provided by Ninja to skip checks on
>     translation units that haven't been modified since they last passed
>     those checks.
>   - Ignore translation units from git submodules.
>   - And more.
>
> Alberto Faria (10):
>   Add an extensible static analyzer
>   Drop unused static function return values
>   static-analyzer: Support adding tests to checks
>   static-analyzer: Avoid reanalyzing unmodified translation units
>   static-analyzer: Enforce coroutine_fn restrictions for direct calls
>   Fix some direct calls from non-coroutine_fn to coroutine_fn
>   static-analyzer: Enforce coroutine_fn restrictions on function
>     pointers
>   Fix some bad coroutine_fn indirect calls and pointer assignments
>   block: Add no_coroutine_fn marker
>   Fix some calls from coroutine_fn to no_coroutine_fn
>
>  accel/kvm/kvm-all.c                        |  12 +-
>  accel/tcg/plugin-gen.c                     |   9 +-
>  accel/tcg/translate-all.c                  |   9 +-
>  audio/audio.c                              |   5 +-
>  block.c                                    |   2 +-
>  block/backup.c                             |   2 +-
>  block/block-copy.c                         |   4 +-
>  block/commit.c                             |   2 +-
>  block/dirty-bitmap.c                       |   6 +-
>  block/file-posix.c                         |   6 +-
>  block/io.c                                 |  52 +-
>  block/mirror.c                             |   4 +-
>  block/monitor/block-hmp-cmds.c             |   2 +-
>  block/nvme.c                               |   3 +-
>  block/parallels.c                          |  28 +-
>  block/qcow.c                               |  10 +-
>  block/qcow2-bitmap.c                       |   6 +-
>  block/qcow2-snapshot.c                     |   6 +-
>  block/qcow2.c                              |  38 +-
>  block/qcow2.h                              |  14 +-
>  block/qed-table.c                          |   2 +-
>  block/qed.c                                |  14 +-
>  block/quorum.c                             |   7 +-
>  block/ssh.c                                |   6 +-
>  block/throttle-groups.c                    |   3 +-
>  block/vdi.c                                |  17 +-
>  block/vhdx.c                               |   8 +-
>  block/vmdk.c                               |  11 +-
>  block/vpc.c                                |   4 +-
>  block/vvfat.c                              |  11 +-
>  blockdev.c                                 |   2 +-
>  chardev/char-ringbuf.c                     |   4 +-
>  contrib/ivshmem-server/main.c              |   4 +-
>  contrib/vhost-user-blk/vhost-user-blk.c    |   5 +-
>  dump/dump.c                                |   4 +-
>  fsdev/virtfs-proxy-helper.c                |   3 +-
>  gdbstub.c                                  |  18 +-
>  hw/audio/intel-hda.c                       |   7 +-
>  hw/audio/pcspk.c                           |   7 +-
>  hw/char/virtio-serial-bus.c                |  14 +-
>  hw/display/cirrus_vga.c                    |   5 +-
>  hw/hyperv/vmbus.c                          |  10 +-
>  hw/i386/intel_iommu.c                      |  28 +-
>  hw/i386/pc_q35.c                           |   5 +-
>  hw/ide/pci.c                               |   4 +-
>  hw/net/rtl8139.c                           |   3 +-
>  hw/net/virtio-net.c                        |   6 +-
>  hw/net/vmxnet3.c                           |   3 +-
>  hw/nvme/ctrl.c                             |  17 +-
>  hw/nvram/fw_cfg.c                          |   3 +-
>  hw/scsi/megasas.c                          |   6 +-
>  hw/scsi/mptconfig.c                        |   7 +-
>  hw/scsi/mptsas.c                           |  14 +-
>  hw/scsi/scsi-bus.c                         |   6 +-
>  hw/usb/dev-audio.c                         |  13 +-
>  hw/usb/hcd-ehci.c                          |   6 +-
>  hw/usb/hcd-ohci.c                          |   4 +-
>  hw/usb/hcd-xhci.c                          |  56 +-
>  hw/vfio/common.c                           |  21 +-
>  hw/virtio/vhost-vdpa.c                     |   3 +-
>  hw/virtio/vhost.c                          |  11 +-
>  hw/virtio/virtio-iommu.c                   |   4 +-
>  hw/virtio/virtio-mem.c                     |   9 +-
>  include/block/block-common.h               |   2 +-
>  include/block/block-hmp-cmds.h             |   2 +-
>  include/block/block-io.h                   |   5 +-
>  include/block/block_int-common.h           |  12 +-
>  include/qemu/coroutine.h                   |  43 +-
>  io/channel-command.c                       |  10 +-
>  migration/migration.c                      |  12 +-
>  net/dump.c                                 |  16 +-
>  net/vhost-vdpa.c                           |   8 +-
>  qemu-img.c                                 |   6 +-
>  qga/commands-posix-ssh.c                   |  10 +-
>  softmmu/physmem.c                          |  18 +-
>  softmmu/qtest.c                            |   5 +-
>  static-analyzer.py                         | 801 +++++++++++++++++++++
>  static_analyzer/__init__.py                | 348 +++++++++
>  static_analyzer/coroutine_fn.py            | 280 +++++++
>  static_analyzer/no_coroutine_fn.py         | 111 +++
>  static_analyzer/return_value_never_used.py | 220 ++++++
>  subprojects/libvduse/libvduse.c            |  12 +-
>  subprojects/libvhost-user/libvhost-user.c  |  24 +-
>  target/i386/host-cpu.c                     |   3 +-
>  target/i386/kvm/kvm.c                      |  19 +-
>  tcg/optimize.c                             |   3 +-
>  tests/qtest/libqos/malloc.c                |   5 +-
>  tests/qtest/libqos/qgraph.c                |   3 +-
>  tests/qtest/test-x86-cpuid-compat.c        |   8 +-
>  tests/qtest/virtio-9p-test.c               |   6 +-
>  tests/unit/test-aio-multithread.c          |   5 +-
>  tests/vhost-user-bridge.c                  |  19 +-
>  ui/vnc.c                                   |  23 +-
>  util/aio-posix.c                           |   7 +-
>  util/uri.c                                 |  18 +-
>  95 files changed, 2160 insertions(+), 519 deletions(-)
>  create mode 100755 static-analyzer.py
>  create mode 100644 static_analyzer/__init__.py
>  create mode 100644 static_analyzer/coroutine_fn.py
>  create mode 100644 static_analyzer/no_coroutine_fn.py
>  create mode 100644 static_analyzer/return_value_never_used.py
>
> --
> 2.37.1
>