qemu-trivial
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Marc-André Lureau
Subject: Re: [Qemu-trivial] [Qemu-devel] clang-tidy: use g_new() family of functions
Date: Tue, 05 Sep 2017 13:29:48 +0000

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

reply via email to

[Prev in Thread] Current Thread [Next in Thread]