Re: [Qemu-devel] clang-tidy: use g_new() family of functions

[Marc-André Lureau]

Hi

On Tue, Sep 5, 2017 at 2:56 PM Markus Armbruster <address@hidden> wrote:

> Marc-André Lureau <address@hidden> writes:
>
> > Hi,
> >
> > I have a series of changes generated with clang-tidy qemu [1] pending
> > for review [2].
> >
> > It translates calloc/*malloc*/*realloc() calls to
> > g_new/g_newa/g_new0/g_renew() where the argument is a sizeof(T) [* N].
>
> Only for *type* T, or for arbitrary T?
>
>
If sizeof() argument is a type:
https://github.com/elmarco/clang-tools-extra/blob/master/clang-tidy/qemu/UseGnewCheck.cpp#L65

For type T, we've done the transformation repeatedly with Coccinelle,
> first in commit b45c03f (commit message appended).  Repeatedly, because
> they creep back in.
>
> How does your technique compare to this prior one?
>

Well, for one, I have a lot of trouble running coccinelle, mostly because
it is slow and/or eat all my memory. Afaik, coccinelle also has trouble
parsing the code in various cases. I have also trouble writing coccinelle
semantic patch... And, I gave up.

clang-tidy in comparison, is quite fast (it takes a minute to apply on qemu
code base). I find it easier to write a tidy check, and more friendly
command line / output etc:

/tmp/test.c:3:17: warning: use g_new() instead [qemu-use-gnew]
        int *f = (int*)malloc(sizeof(int) * 2);
                 ~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~
                 g_new(int, 2)

clang-tidy should also respect clang-format rules when modifying the code
(see also
https://lists.nongnu.org/archive/html/qemu-devel/2017-08/msg05762.html).

I also imagine it should be possible to integrate to clang warnings in the
future (similar to libreoffice clang plugins for ex). All in all,
coccinelle or clang-tidy are probably both capable of doing this job, but
clang-tidy is snappier and has more potentials for toolchain integration.

> This is the first commit to give you an idea:
> >
> > diff --git a/hw/timer/arm_timer.c b/hw/timer/arm_timer.c
> > index 98fddd7ac1..75affcb8a6 100644
> > --- a/hw/timer/arm_timer.c
> > +++ b/hw/timer/arm_timer.c
> > @@ -166,7 +166,7 @@ static arm_timer_state *arm_timer_init(uint32_t freq)
> >      arm_timer_state *s;
> >      QEMUBH *bh;
> >
> > -    s = (arm_timer_state *)g_malloc0(sizeof(arm_timer_state));
> > +    s = g_new0(arm_timer_state, 1);
> >      s->freq = freq;
> >      s->control = TIMER_CTRL_IE;
> >
> > g_new() advantages (from glib doc):
> > - the returned pointer is cast to a pointer to the given type.
> > - care is taken to avoid overflow when calculating the size of the
> > allocated block.
> >
> > But it is also shorter&nicer :)
> >
> > I have not included in the first batch the opportunity to also
> > translate: alloc(sizeof(*p) * x) to g_new(typeof(*p), x), since it is
> > arguably not much nicer. But for consistency, I think it would be good
> > to use g_new(). What do you think?
>
> p = g_malloc(sizeof(*p)) is idiomatic, and not obviously inferior to
> p = g_new(T, 1), where T is the type of *p.  Use whatever is easier to
> read, I guess.
>
> But once you add multiplication, g_new() adds something useful: overflow
> protection.  Conversion to g_new() might make sense then.
>
> > I splitted the changes amont the various MAINTAINERS entries, but it
> > is still about 70 patches (without the typeof() changes).
> >
> >
> > [1] https://github.com/elmarco/clang-tools-extra/
> > [2] https://github.com/elmarco/qemu/commits/gnew
>
>
> commit b45c03f585ea9bb1af76c73e82195418c294919d
> Author: Markus Armbruster <address@hidden>
> Date:   Mon Sep 7 10:39:27 2015 +0100
>
>     arm: Use g_new() & friends where that makes obvious sense
>
>     g_new(T, n) is neater than g_malloc(sizeof(T) * n).  It's also safer,
>     for two reasons.  One, it catches multiplication overflowing size_t.
>     Two, it returns T * rather than void *, which lets the compiler catch
>     more type errors.
>
>     This commit only touches allocations with size arguments of the form
>     sizeof(T).
>
>     Coccinelle semantic patch:
>
>         @@
>         type T;
>         @@
>         -g_malloc(sizeof(T))
>         +g_new(T, 1)
>         @@
>         type T;
>         @@
>         -g_try_malloc(sizeof(T))
>         +g_try_new(T, 1)
>

thanks, I actually forgot the g_try_new() family (I thought we didn't use
it!)

        @@
>         type T;
>         @@
>         -g_malloc0(sizeof(T))
>         +g_new0(T, 1)
>         @@
>         type T;
>         @@
>         -g_try_malloc0(sizeof(T))
>         +g_try_new0(T, 1)
>         @@
>         type T;
>         expression n;
>         @@
>         -g_malloc(sizeof(T) * (n))
>         +g_new(T, n)
>         @@
>         type T;
>         expression n;
>         @@
>         -g_try_malloc(sizeof(T) * (n))
>         +g_try_new(T, n)
>         @@
>         type T;
>         expression n;
>         @@
>         -g_malloc0(sizeof(T) * (n))
>         +g_new0(T, n)
>         @@
>         type T;
>         expression n;
>         @@
>         -g_try_malloc0(sizeof(T) * (n))
>         +g_try_new0(T, n)
>         @@
>         type T;
>         expression p, n;
>         @@
>         -g_realloc(p, sizeof(T) * (n))
>         +g_renew(T, p, n)
>         @@
>         type T;
>         expression p, n;
>         @@
>         -g_try_realloc(p, sizeof(T) * (n))
>         +g_try_renew(T, p, n)
>         @@
>         type T;
>         expression n;
>         @@
>         -(T *)g_new(T, n)
>         +g_new(T, n)
>         @@
>         type T;
>         expression n;
>         @@
>         -(T *)g_new0(T, n)
>         +g_new0(T, n)
>         @@
>         type T;
>         expression p, n;
>         @@
>         -(T *)g_renew(T, p, n)
>         +g_renew(T, p, n)
>
>     Signed-off-by: Markus Armbruster <address@hidden>
>     Reviewed-by: Eric Blake <address@hidden>
>     Message-id: address@hidden
>     Signed-off-by: Peter Maydell <address@hidden>
>
-- 
Marc-André Lureau