[Qemu-devel] [PATCH 00/31] Refactoring with clang-tidy

Marc-André Lureau posted 31 patches 8 years, 7 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20170622124204.19407-1-marcandre.lureau@redhat.com
Test FreeBSD passed
Test checkpatch failed
Test docker passed
Test s390x passed
There is a newer version of this series
include/ui/console.h        |  2 +-
linux-headers/asm-x86/kvm.h |  2 +-
slirp/ip6.h                 |  6 +++---
block/dmg.c                 |  2 +-
block/qcow2-cluster.c       |  2 +-
block/qcow2-refcount.c      |  2 +-
block/vhdx-log.c            |  2 +-
block/vpc.c                 |  4 ++--
block/vvfat.c               |  4 ++--
hw/9pfs/9p-synth.c          |  3 +--
hw/audio/pcspk.c            |  2 +-
hw/char/virtio-serial-bus.c |  8 ++++----
hw/display/vga.c            |  2 +-
hw/display/virtio-gpu.c     |  4 ++--
hw/i386/multiboot.c         |  3 +--
hw/net/eepro100.c           |  3 +--
hw/pci-host/piix.c          |  2 +-
hw/pci-host/q35.c           |  2 +-
hw/pci/msix.c               |  4 ++--
hw/timer/i8254_common.c     |  4 ++--
hw/usb/dev-hub.c            |  8 ++++----
hw/virtio/vhost.c           |  2 +-
libdecnumber/decNumber.c    |  2 +-
monitor.c                   |  4 ++--
target/i386/arch_dump.c     | 40 ++++++++++++++++++++--------------------
target/ppc/mem_helper.c     |  2 +-
target/ppc/translate.c      |  2 +-
tests/test-iov.c            |  3 +--
ui/cursor.c                 |  2 +-
ui/vnc-enc-tight.c          |  2 +-
ui/vnc.c                    | 10 +++++-----
31 files changed, 68 insertions(+), 72 deletions(-)
[Qemu-devel] [PATCH 00/31] Refactoring with clang-tidy
Posted by Marc-André Lureau 8 years, 7 months ago
Hi,

Various refactring questions on previously sent series prompted me to
look at coccinelle to automate some changes again. Alas, semantic
patches are not so easy to express for me, cocci doesn't catch all
cases, is quite slow, and it doesn't seem possible to evaluate
expressions to check if E == E-1 or if E is pow2 for example.

I started looking at clang-tidy
(http://clang.llvm.org/extra/clang-tidy/) as an alternative to do some
refactoring.

Our build-system doesn't generate compile_commands.json, which is
pretty much required to use clang refactoring tools. But it is as easy
as running "bear make" to make one, using https://github.com/rizsotto/Bear.

Then, you can run checks and automate fixes (here only "qemu-round")
over the code base with:
run-clang-tidy.py -header-filter='.*' -checks='-*,qemu-round' -fix

There are some path bugs that can easily be solved, see
https://bugs.llvm.org/show_bug.cgi?id=33440 for details.

I ran 'qemu-round' check in this series, and added a few other manual
refactoring I had pending. (I'll submit other series for the rest of
the checks later)

My WIP qemu checks are here:
https://github.com/elmarco/clang-tools-extra/tree/master/clang-tidy/qemu

The "round-check" is:
https://github.com/elmarco/clang-tools-extra/blob/master/clang-tidy/qemu/RoundCheck.cpp

I don't know if you can express a semantic patch that would be as
powerful. The code remains easy to write & read imho, and quite fast
to apply. I like the tool, it's probably a good complement to
coccinelle overall.

Thanks

Marc-André Lureau (31):
  i386: use ROUND_UP macro
  vnc: use QEMU_ALIGN_DOWN
  vhdx: use QEMU_ALIGN_DOWN
  vhost: use QEMU_ALIGN_DOWN
  i8254: use QEMU_ALIGN_DOWN
  pcspk: use QEMU_ALIGN_DOWN
  dmg: use DIV_ROUND_UP
  qcow2: use DIV_ROUND_UP
  vpc: use DIV_ROUND_UP
  vvfat: use DIV_ROUND_UP
  vnc: use DIV_ROUND_UP
  slirp: use DIV_ROUND_UP
  ui: use DIV_ROUND_UP
  vga: use DIV_ROUND_UP
  virtio-gpu: use DIV_ROUND_UP
  monitor: use DIV_ROUND_UP
  console: use DIV_ROUND_UP
  virtio-serial: use DIV_ROUND_UP
  piix: use DIV_ROUND_UP
  q35: use DIV_ROUND_UP
  usb-hub: use DIV_ROUND_UP
  msix: use DIV_ROUND_UP
  ppc: use DIV_ROUND_UP
  i386/dump: use DIV_ROUND_UP
  kvm: use DIV_ROUND_UP
  decnumber: use DIV_ROUND_UP
  i386: introduce ELF_NOTE_SIZE macro
  9pfs: replace g_malloc()+memcpy() with g_memdup()
  i386: replace g_malloc()+memcpy() with g_memdup()
  test-iov: replace g_malloc()+memcpy() with g_memdup()
  eepro100: replace g_malloc()+memcpy() with g_memdup()

 include/ui/console.h        |  2 +-
 linux-headers/asm-x86/kvm.h |  2 +-
 slirp/ip6.h                 |  6 +++---
 block/dmg.c                 |  2 +-
 block/qcow2-cluster.c       |  2 +-
 block/qcow2-refcount.c      |  2 +-
 block/vhdx-log.c            |  2 +-
 block/vpc.c                 |  4 ++--
 block/vvfat.c               |  4 ++--
 hw/9pfs/9p-synth.c          |  3 +--
 hw/audio/pcspk.c            |  2 +-
 hw/char/virtio-serial-bus.c |  8 ++++----
 hw/display/vga.c            |  2 +-
 hw/display/virtio-gpu.c     |  4 ++--
 hw/i386/multiboot.c         |  3 +--
 hw/net/eepro100.c           |  3 +--
 hw/pci-host/piix.c          |  2 +-
 hw/pci-host/q35.c           |  2 +-
 hw/pci/msix.c               |  4 ++--
 hw/timer/i8254_common.c     |  4 ++--
 hw/usb/dev-hub.c            |  8 ++++----
 hw/virtio/vhost.c           |  2 +-
 libdecnumber/decNumber.c    |  2 +-
 monitor.c                   |  4 ++--
 target/i386/arch_dump.c     | 40 ++++++++++++++++++++--------------------
 target/ppc/mem_helper.c     |  2 +-
 target/ppc/translate.c      |  2 +-
 tests/test-iov.c            |  3 +--
 ui/cursor.c                 |  2 +-
 ui/vnc-enc-tight.c          |  2 +-
 ui/vnc.c                    | 10 +++++-----
 31 files changed, 68 insertions(+), 72 deletions(-)

-- 
2.13.1.395.gf7b71de06


Re: [Qemu-devel] [PATCH 00/31] Refactoring with clang-tidy
Posted by Peter Maydell 8 years, 7 months ago
On 22 June 2017 at 13:41, Marc-André Lureau <marcandre.lureau@redhat.com> wrote:
> Various refactring questions on previously sent series prompted me to
> look at coccinelle to automate some changes again. Alas, semantic
> patches are not so easy to express for me, cocci doesn't catch all
> cases, is quite slow, and it doesn't seem possible to evaluate
> expressions to check if E == E-1 or if E is pow2 for example.
>
> I started looking at clang-tidy
> (http://clang.llvm.org/extra/clang-tidy/) as an alternative to do some
> refactoring.

Oh, very nice. I've had "investigate clang-tidy" in my todo list for
ages so it's nice that somebody has done it for me ;-)

I think it would be particularly interesting to look at whether
we can use this infrastructure to automatically detect some kinds
of bug, rather than merely stylistic issues. (As a random
example, could you detect calls to coroutine_fn functions from
normal functions?)

thanks
-- PMM

Re: [Qemu-devel] [PATCH 00/31] Refactoring with clang-tidy
Posted by no-reply@patchew.org 8 years, 7 months ago
Hi,

This series seems to have some coding style problems. See output below for
more information:

Type: series
Message-id: 20170622124204.19407-1-marcandre.lureau@redhat.com
Subject: [Qemu-devel] [PATCH 00/31] Refactoring with clang-tidy

=== TEST SCRIPT BEGIN ===
#!/bin/bash

BASE=base
n=1
total=$(git log --oneline $BASE.. | wc -l)
failed=0

git config --local diff.renamelimit 0
git config --local diff.renames True

commits="$(git log --format=%H --reverse $BASE..)"
for c in $commits; do
    echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..."
    if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
        failed=1
        echo
    fi
    n=$((n+1))
done

exit $failed
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
 - [tag update]      patchew/20170622033231.19344-1-f4bug@amsat.org -> patchew/20170622033231.19344-1-f4bug@amsat.org
 - [tag update]      patchew/20170622124204.19407-1-marcandre.lureau@redhat.com -> patchew/20170622124204.19407-1-marcandre.lureau@redhat.com
Switched to a new branch 'test'
ef7ecd5 eepro100: replace g_malloc()+memcpy() with g_memdup()
e835e82 test-iov: replace g_malloc()+memcpy() with g_memdup()
6cf725b i386: replace g_malloc()+memcpy() with g_memdup()
e8fb6bf 9pfs: replace g_malloc()+memcpy() with g_memdup()
8890651 i386: introduce ELF_NOTE_SIZE macro
3b504b6 decnumber: use DIV_ROUND_UP
20d7521 kvm: use DIV_ROUND_UP
aff7bf0 i386/dump: use DIV_ROUND_UP
24b41f9 ppc: use DIV_ROUND_UP
5796db0 msix: use DIV_ROUND_UP
15bf263 usb-hub: use DIV_ROUND_UP
1a7a691 q35: use DIV_ROUND_UP
5854633 piix: use DIV_ROUND_UP
7131ae7 virtio-serial: use DIV_ROUND_UP
cc43300 console: use DIV_ROUND_UP
f50d148 monitor: use DIV_ROUND_UP
059282b virtio-gpu: use DIV_ROUND_UP
2054fcf vga: use DIV_ROUND_UP
e9c4107 ui: use DIV_ROUND_UP
033d501 slirp: use DIV_ROUND_UP
a6560ec vnc: use DIV_ROUND_UP
889a8de vvfat: use DIV_ROUND_UP
2ace8f6 vpc: use DIV_ROUND_UP
b14cee9 qcow2: use DIV_ROUND_UP
0ae8a14 dmg: use DIV_ROUND_UP
f34c5a5 pcspk: use QEMU_ALIGN_DOWN
85c3191 i8254: use QEMU_ALIGN_DOWN
ad42a17 vhost: use QEMU_ALIGN_DOWN
207228e vhdx: use QEMU_ALIGN_DOWN
d4a22e3 vnc: use QEMU_ALIGN_DOWN
cf87ec7 i386: use ROUND_UP macro

=== OUTPUT BEGIN ===
Checking PATCH 1/31: i386: use ROUND_UP macro...
Checking PATCH 2/31: vnc: use QEMU_ALIGN_DOWN...
Checking PATCH 3/31: vhdx: use QEMU_ALIGN_DOWN...
Checking PATCH 4/31: vhost: use QEMU_ALIGN_DOWN...
Checking PATCH 5/31: i8254: use QEMU_ALIGN_DOWN...
Checking PATCH 6/31: pcspk: use QEMU_ALIGN_DOWN...
ERROR: line over 90 characters
#24: FILE: hw/audio/pcspk.c:72:
+        s->samples = (QEMU_ALIGN_DOWN(PCSPK_BUF_LEN * PIT_FREQ, m) / (PIT_FREQ >> 1) + 1) >> 1;

total: 1 errors, 0 warnings, 8 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

Checking PATCH 7/31: dmg: use DIV_ROUND_UP...
Checking PATCH 8/31: qcow2: use DIV_ROUND_UP...
Checking PATCH 9/31: vpc: use DIV_ROUND_UP...
Checking PATCH 10/31: vvfat: use DIV_ROUND_UP...
ERROR: spaces required around that '=' (ctx:VxV)
#24: FILE: block/vvfat.c:435:
+        number_of_entries=DIV_ROUND_UP(length, 26),i;
                          ^

ERROR: space required after that ',' (ctx:VxV)
#24: FILE: block/vvfat.c:435:
+        number_of_entries=DIV_ROUND_UP(length, 26),i;
                                                   ^

ERROR: code indent should never use tabs
#33: FILE: block/vvfat.c:2426:
+^I    (uint8_t*)cluster, DIV_ROUND_UP(rest_size, 0x200));$

ERROR: "(foo*)" should be "(foo *)"
#33: FILE: block/vvfat.c:2426:
+	    (uint8_t*)cluster, DIV_ROUND_UP(rest_size, 0x200));

total: 4 errors, 0 warnings, 16 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

Checking PATCH 11/31: vnc: use DIV_ROUND_UP...
WARNING: line over 80 characters
#37: FILE: ui/vnc.c:2785:
+        guest_ll = pixman_image_get_width(vd->guest.fb) * (DIV_ROUND_UP(guest_bpp, 8));

total: 0 errors, 1 warnings, 16 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
Checking PATCH 12/31: slirp: use DIV_ROUND_UP...
Checking PATCH 13/31: ui: use DIV_ROUND_UP...
Checking PATCH 14/31: vga: use DIV_ROUND_UP...
Checking PATCH 15/31: virtio-gpu: use DIV_ROUND_UP...
Checking PATCH 16/31: monitor: use DIV_ROUND_UP...
Checking PATCH 17/31: console: use DIV_ROUND_UP...
Checking PATCH 18/31: virtio-serial: use DIV_ROUND_UP...
WARNING: line over 80 characters
#51: FILE: hw/char/virtio-serial-bus.c:1078:
+    vser->ports_map = g_malloc0((DIV_ROUND_UP(vser->serial.max_virtserial_ports, 32))

total: 0 errors, 1 warnings, 32 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
Checking PATCH 19/31: piix: use DIV_ROUND_UP...
Checking PATCH 20/31: q35: use DIV_ROUND_UP...
Checking PATCH 21/31: usb-hub: use DIV_ROUND_UP...
Checking PATCH 22/31: msix: use DIV_ROUND_UP...
Checking PATCH 23/31: ppc: use DIV_ROUND_UP...
Checking PATCH 24/31: i386/dump: use DIV_ROUND_UP...
WARNING: line over 80 characters
#25: FILE: target/i386/arch_dump.c:80:
+    note_size = (DIV_ROUND_UP(sizeof(Elf64_Nhdr), 4) + DIV_ROUND_UP(name_size, 4) +

WARNING: line over 80 characters
#36: FILE: target/i386/arch_dump.c:159:
+    note_size = (DIV_ROUND_UP(sizeof(Elf64_Nhdr), 4) + DIV_ROUND_UP(name_size, 4) +

WARNING: line over 80 characters
#47: FILE: target/i386/arch_dump.c:214:
+    note_size = (DIV_ROUND_UP(sizeof(Elf32_Nhdr), 4) + DIV_ROUND_UP(name_size, 4) +

WARNING: line over 80 characters
#71: FILE: target/i386/arch_dump.c:446:
+    elf_note_size = (DIV_ROUND_UP(note_head_size, 4) + DIV_ROUND_UP(name_size, 4) +

WARNING: line over 80 characters
#73: FILE: target/i386/arch_dump.c:448:
+    qemu_note_size = (DIV_ROUND_UP(note_head_size, 4) + DIV_ROUND_UP(name_size, 4) +

total: 0 errors, 5 warnings, 54 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
Checking PATCH 25/31: kvm: use DIV_ROUND_UP...
Checking PATCH 26/31: decnumber: use DIV_ROUND_UP...
ERROR: code indent should never use tabs
#24: FILE: libdecnumber/decNumber.c:4778:
+^I    *(up-1)+=DIV_ROUND_UP(DECDPUNMAX, 2);$

ERROR: spaces required around that '-' (ctx:VxV)
#24: FILE: libdecnumber/decNumber.c:4778:
+	    *(up-1)+=DIV_ROUND_UP(DECDPUNMAX, 2);
 	        ^

ERROR: spaces required around that '+=' (ctx:VxV)
#24: FILE: libdecnumber/decNumber.c:4778:
+	    *(up-1)+=DIV_ROUND_UP(DECDPUNMAX, 2);
 	           ^

total: 3 errors, 0 warnings, 8 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

Checking PATCH 27/31: i386: introduce ELF_NOTE_SIZE macro...
Checking PATCH 28/31: 9pfs: replace g_malloc()+memcpy() with g_memdup()...
Checking PATCH 29/31: i386: replace g_malloc()+memcpy() with g_memdup()...
Checking PATCH 30/31: test-iov: replace g_malloc()+memcpy() with g_memdup()...
Checking PATCH 31/31: eepro100: replace g_malloc()+memcpy() with g_memdup()...
=== OUTPUT END ===

Test command exited with code: 1


---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-devel@freelists.org