[Qemu-devel] [PATCH 0/5] arm: five simple memory leak fixes

[Qemu-devel] [PATCH 0/5] arm: five simple memory leak fixes

[Peter Maydell]

I ran 'make check' with the clang sanitizers enabled, which
it turns out these days includes LeakSanitizer. That points
out a fair number of memory leaks, most of which are not
terribly significant. This patchset fixes five of them (there
are plenty more to do).

The "device does timer_new() in its instance_init function"
leak seems to be a common one with multiple other devices
showing up as having the same problem. Switching timer_init()
is probably a nicer fix than just moving timer_new() to realize,
but I wasn't sure if going from VMSTATE_TIMER_PTR to
VMSTATE_TIMER would be a migration compat break.

thanks
-- PMM

Peter Maydell (5):
  target/arm: Free name string in ARMCPRegInfo hashtable entries
  hw/arm/mps2-tz.c: Free mscname string in make_dma()
  hw/sd/sdhci: Don't leak memory region in sdhci_sysbus_realize()
  tests/test-arm-mptimer: Don't leak string memory
  target/arm: Create timers in realize, not init

 hw/arm/mps2-tz.c         |   1 +
 hw/sd/sdhci.c            |   5 +-
 target/arm/cpu.c         |  33 ++++++---
 tests/test-arm-mptimer.c | 153 ++++++++++++++++++---------------------
 4 files changed, 95 insertions(+), 97 deletions(-)

-- 
2.19.2

[Qemu-devel] [PATCH 1/5] target/arm: Free name string in ARMCPRegInfo hashtable entries

[Peter Maydell]

When we add a new entry to the ARMCPRegInfo hash table in
add_cpreg_to_hashtable(), we allocate memory for tehe
ARMCPRegInfo struct itself, and we also g_strdup() the
name string. So the hashtable's value destructor function
must free the name string as well as the struct.

Spotted by clang's leak sanitizer. The leak here is a
small one-off leak at startup, because we don't support
CPU hotplug, and so the only time when we destroy
hash table entries is for the case where ARM_CP_OVERRIDE
means we register a wildcard entry and then override it later.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target/arm/cpu.c | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/target/arm/cpu.c b/target/arm/cpu.c
index 60411f6bfe0..b84a6c0e678 100644
--- a/target/arm/cpu.c
+++ b/target/arm/cpu.c
@@ -642,6 +642,20 @@ uint64_t arm_cpu_mp_affinity(int idx, uint8_t clustersz)
     return (Aff1 << ARM_AFF1_SHIFT) | Aff0;
 }
 
+static void cpreg_hashtable_data_destroy(gpointer data)
+{
+    /*
+     * Destroy function for cpu->cp_regs hashtable data entries.
+     * We must free the name string because it was g_strdup()ed in
+     * add_cpreg_to_hashtable(). It's OK to cast away the 'const'
+     * from r->name because we know we definitely allocated it.
+     */
+    ARMCPRegInfo *r = data;
+
+    g_free((void *)r->name);
+    g_free(r);
+}
+
 static void arm_cpu_initfn(Object *obj)
 {
     CPUState *cs = CPU(obj);
@@ -649,7 +663,7 @@ static void arm_cpu_initfn(Object *obj)
 
     cs->env_ptr = &cpu->env;
     cpu->cp_regs = g_hash_table_new_full(g_int_hash, g_int_equal,
-                                         g_free, g_free);
+                                         g_free, cpreg_hashtable_data_destroy);
 
     QLIST_INIT(&cpu->pre_el_change_hooks);
     QLIST_INIT(&cpu->el_change_hooks);
-- 
2.19.2

Re: [Qemu-devel] [PATCH 1/5] target/arm: Free name string in ARMCPRegInfo hashtable entries

[Richard Henderson]

On 12/4/18 7:29 AM, Peter Maydell wrote:
> When we add a new entry to the ARMCPRegInfo hash table in
> add_cpreg_to_hashtable(), we allocate memory for tehe
> ARMCPRegInfo struct itself, and we also g_strdup() the
> name string. So the hashtable's value destructor function
> must free the name string as well as the struct.
> 
> Spotted by clang's leak sanitizer. The leak here is a
> small one-off leak at startup, because we don't support
> CPU hotplug, and so the only time when we destroy
> hash table entries is for the case where ARM_CP_OVERRIDE
> means we register a wildcard entry and then override it later.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  target/arm/cpu.c | 16 +++++++++++++++-
>  1 file changed, 15 insertions(+), 1 deletion(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

Re: [Qemu-devel] [PATCH 1/5] target/arm: Free name string in ARMCPRegInfo hashtable entries

[Philippe Mathieu-Daudé]

On 4/12/18 14:29, Peter Maydell wrote:
> When we add a new entry to the ARMCPRegInfo hash table in
> add_cpreg_to_hashtable(), we allocate memory for tehe

"for the"?

> ARMCPRegInfo struct itself, and we also g_strdup() the
> name string. So the hashtable's value destructor function
> must free the name string as well as the struct.
> 
> Spotted by clang's leak sanitizer. The leak here is a
> small one-off leak at startup, because we don't support
> CPU hotplug, and so the only time when we destroy
> hash table entries is for the case where ARM_CP_OVERRIDE
> means we register a wildcard entry and then override it later.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

> ---
>  target/arm/cpu.c | 16 +++++++++++++++-
>  1 file changed, 15 insertions(+), 1 deletion(-)
> 
> diff --git a/target/arm/cpu.c b/target/arm/cpu.c
> index 60411f6bfe0..b84a6c0e678 100644
> --- a/target/arm/cpu.c
> +++ b/target/arm/cpu.c
> @@ -642,6 +642,20 @@ uint64_t arm_cpu_mp_affinity(int idx, uint8_t clustersz)
>      return (Aff1 << ARM_AFF1_SHIFT) | Aff0;
>  }
>  
> +static void cpreg_hashtable_data_destroy(gpointer data)
> +{
> +    /*
> +     * Destroy function for cpu->cp_regs hashtable data entries.
> +     * We must free the name string because it was g_strdup()ed in
> +     * add_cpreg_to_hashtable(). It's OK to cast away the 'const'
> +     * from r->name because we know we definitely allocated it.
> +     */
> +    ARMCPRegInfo *r = data;
> +
> +    g_free((void *)r->name);
> +    g_free(r);
> +}
> +
>  static void arm_cpu_initfn(Object *obj)
>  {
>      CPUState *cs = CPU(obj);
> @@ -649,7 +663,7 @@ static void arm_cpu_initfn(Object *obj)
>  
>      cs->env_ptr = &cpu->env;
>      cpu->cp_regs = g_hash_table_new_full(g_int_hash, g_int_equal,
> -                                         g_free, g_free);
> +                                         g_free, cpreg_hashtable_data_destroy);
>  
>      QLIST_INIT(&cpu->pre_el_change_hooks);
>      QLIST_INIT(&cpu->el_change_hooks);
> 

[Qemu-devel] [PATCH 2/5] hw/arm/mps2-tz.c: Free mscname string in make_dma()

[Peter Maydell]

The clang leak sanitizer spots a (one-off, trivial) memory
leak in make_dma() due to a missing free.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/arm/mps2-tz.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/arm/mps2-tz.c b/hw/arm/mps2-tz.c
index 6dd02ae47e8..82b1d020a58 100644
--- a/hw/arm/mps2-tz.c
+++ b/hw/arm/mps2-tz.c
@@ -322,6 +322,7 @@ static MemoryRegion *make_dma(MPS2TZMachineState *mms, void *opaque,
     sysbus_connect_irq(s, 2, qdev_get_gpio_in_named(iotkitdev,
                                                     "EXP_IRQ", 57 + i * 3));
 
+    g_free(mscname);
     return sysbus_mmio_get_region(s, 0);
 }
 
-- 
2.19.2

Re: [Qemu-devel] [PATCH 2/5] hw/arm/mps2-tz.c: Free mscname string in make_dma()

[Richard Henderson]

On 12/4/18 7:29 AM, Peter Maydell wrote:
> The clang leak sanitizer spots a (one-off, trivial) memory
> leak in make_dma() due to a missing free.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  hw/arm/mps2-tz.c | 1 +
>  1 file changed, 1 insertion(+)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

Re: [Qemu-devel] [PATCH 2/5] hw/arm/mps2-tz.c: Free mscname string in make_dma()

[Philippe Mathieu-Daudé]

On 4/12/18 14:29, Peter Maydell wrote:
> The clang leak sanitizer spots a (one-off, trivial) memory
> leak in make_dma() due to a missing free.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

> ---
>  hw/arm/mps2-tz.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/hw/arm/mps2-tz.c b/hw/arm/mps2-tz.c
> index 6dd02ae47e8..82b1d020a58 100644
> --- a/hw/arm/mps2-tz.c
> +++ b/hw/arm/mps2-tz.c
> @@ -322,6 +322,7 @@ static MemoryRegion *make_dma(MPS2TZMachineState *mms, void *opaque,
>      sysbus_connect_irq(s, 2, qdev_get_gpio_in_named(iotkitdev,
>                                                      "EXP_IRQ", 57 + i * 3));
>  
> +    g_free(mscname);
>      return sysbus_mmio_get_region(s, 0);
>  }
>  
> 

[Qemu-devel] [PATCH 3/5] hw/sd/sdhci: Don't leak memory region in sdhci_sysbus_realize()

[Peter Maydell]

In sdhci_sysbus_realize() we override the initialization of
s->iomem that sdhci_common_realize() performs. However we
don't destroy the old memory region before reinitializing
it, which means that the memory allocated for mr->name in
memory_region_do_init() is leaked.

Since sdhci_initfn() already initializes s->io_ops to
&sdhci_mmio_ops, always use that in sdhci_common_realize()
and remove the now-unnecessary reinitialization of the
MMIO region from sdhci_sysbus_realize().

Spotted by clang's leak sanitizer.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/sd/sdhci.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
index 81bbf032794..83f1574ffdc 100644
--- a/hw/sd/sdhci.c
+++ b/hw/sd/sdhci.c
@@ -1371,7 +1371,7 @@ static void sdhci_common_realize(SDHCIState *s, Error **errp)
     s->buf_maxsz = sdhci_get_fifolen(s);
     s->fifo_buffer = g_malloc0(s->buf_maxsz);
 
-    memory_region_init_io(&s->iomem, OBJECT(s), &sdhci_mmio_ops, s, "sdhci",
+    memory_region_init_io(&s->iomem, OBJECT(s), s->io_ops, s, "sdhci",
                           SDHC_REGISTERS_MAP_SIZE);
 }
 
@@ -1565,9 +1565,6 @@ static void sdhci_sysbus_realize(DeviceState *dev, Error ** errp)
 
     sysbus_init_irq(sbd, &s->irq);
 
-    memory_region_init_io(&s->iomem, OBJECT(s), s->io_ops, s, "sdhci",
-            SDHC_REGISTERS_MAP_SIZE);
-
     sysbus_init_mmio(sbd, &s->iomem);
 }
 
-- 
2.19.2

Re: [Qemu-devel] [PATCH 3/5] hw/sd/sdhci: Don't leak memory region in sdhci_sysbus_realize()

[Richard Henderson]

On 12/4/18 7:29 AM, Peter Maydell wrote:
> In sdhci_sysbus_realize() we override the initialization of
> s->iomem that sdhci_common_realize() performs. However we
> don't destroy the old memory region before reinitializing
> it, which means that the memory allocated for mr->name in
> memory_region_do_init() is leaked.
> 
> Since sdhci_initfn() already initializes s->io_ops to
> &sdhci_mmio_ops, always use that in sdhci_common_realize()
> and remove the now-unnecessary reinitialization of the
> MMIO region from sdhci_sysbus_realize().
> 
> Spotted by clang's leak sanitizer.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  hw/sd/sdhci.c | 5 +----
>  1 file changed, 1 insertion(+), 4 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

Re: [Qemu-devel] [PATCH 3/5] hw/sd/sdhci: Don't leak memory region in sdhci_sysbus_realize()

[Philippe Mathieu-Daudé]

On 4/12/18 14:29, Peter Maydell wrote:
> In sdhci_sysbus_realize() we override the initialization of
> s->iomem that sdhci_common_realize() performs. However we
> don't destroy the old memory region before reinitializing
> it, which means that the memory allocated for mr->name in
> memory_region_do_init() is leaked.

Oops

> 
> Since sdhci_initfn() already initializes s->io_ops to
> &sdhci_mmio_ops, always use that in sdhci_common_realize()
> and remove the now-unnecessary reinitialization of the
> MMIO region from sdhci_sysbus_realize().

Thanks for the cleanup!

> 
> Spotted by clang's leak sanitizer.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

> ---
>  hw/sd/sdhci.c | 5 +----
>  1 file changed, 1 insertion(+), 4 deletions(-)
> 
> diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
> index 81bbf032794..83f1574ffdc 100644
> --- a/hw/sd/sdhci.c
> +++ b/hw/sd/sdhci.c
> @@ -1371,7 +1371,7 @@ static void sdhci_common_realize(SDHCIState *s, Error **errp)
>      s->buf_maxsz = sdhci_get_fifolen(s);
>      s->fifo_buffer = g_malloc0(s->buf_maxsz);
>  
> -    memory_region_init_io(&s->iomem, OBJECT(s), &sdhci_mmio_ops, s, "sdhci",
> +    memory_region_init_io(&s->iomem, OBJECT(s), s->io_ops, s, "sdhci",
>                            SDHC_REGISTERS_MAP_SIZE);
>  }
>  
> @@ -1565,9 +1565,6 @@ static void sdhci_sysbus_realize(DeviceState *dev, Error ** errp)
>  
>      sysbus_init_irq(sbd, &s->irq);
>  
> -    memory_region_init_io(&s->iomem, OBJECT(s), s->io_ops, s, "sdhci",
> -            SDHC_REGISTERS_MAP_SIZE);
> -
>      sysbus_init_mmio(sbd, &s->iomem);
>  }
>  
> 

[Qemu-devel] [PATCH 4/5] tests/test-arm-mptimer: Don't leak string memory

[Peter Maydell]

The test-arm-mptimer setup creates a lot of test names using
g_strdup_printf() and never frees them. This is entirely
harmless since it's one-shot test code, but it clutters
up the output from clang's LeakSanitizer. Refactor to
use a helper function so we can free the memory.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 tests/test-arm-mptimer.c | 153 ++++++++++++++++++---------------------
 1 file changed, 69 insertions(+), 84 deletions(-)

diff --git a/tests/test-arm-mptimer.c b/tests/test-arm-mptimer.c
index cb8f2df9141..156a39f50dd 100644
--- a/tests/test-arm-mptimer.c
+++ b/tests/test-arm-mptimer.c
@@ -991,10 +991,25 @@ static void test_timer_zero_load_nonscaled_periodic_to_prescaled_oneshot(void)
     g_assert_cmpuint(timer_get_and_clr_int_sts(), ==, 0);
 }
 
+/*
+ * Add a qtest test that comes in two versions: one with
+ * a timer scaler setting, and one with the timer nonscaled.
+ */
+static void add_scaler_test(const char *str, bool scale,
+                            void (*fn)(const void *))
+{
+    char *name;
+    int *scaler = scale ? &scaled : &nonscaled;
+
+    name = g_strdup_printf("%s=%d", str, *scaler);
+    qtest_add_data_func(name, scaler, fn);
+    g_free(name);
+}
+
 int main(int argc, char **argv)
 {
-    int *scaler = &nonscaled;
     int ret;
+    int scale;
 
     g_test_init(&argc, &argv, NULL);
 
@@ -1012,89 +1027,59 @@ int main(int argc, char **argv)
     qtest_add_func("mptimer/prescaler", test_timer_prescaler);
     qtest_add_func("mptimer/prescaler_on_the_fly", test_timer_prescaler_on_the_fly);
 
-tests_with_prescaler_arg:
-    qtest_add_data_func(
-        g_strdup_printf("mptimer/oneshot scaler=%d", *scaler),
-                        scaler, test_timer_oneshot);
-    qtest_add_data_func(
-        g_strdup_printf("mptimer/pause scaler=%d", *scaler),
-                        scaler, test_timer_pause);
-    qtest_add_data_func(
-        g_strdup_printf("mptimer/reload scaler=%d", *scaler),
-                        scaler, test_timer_reload);
-    qtest_add_data_func(
-        g_strdup_printf("mptimer/periodic scaler=%d", *scaler),
-                        scaler, test_timer_periodic);
-    qtest_add_data_func(
-        g_strdup_printf("mptimer/oneshot_to_periodic scaler=%d", *scaler),
-                        scaler, test_timer_oneshot_to_periodic);
-    qtest_add_data_func(
-        g_strdup_printf("mptimer/periodic_to_oneshot scaler=%d", *scaler),
-                        scaler, test_timer_periodic_to_oneshot);
-    qtest_add_data_func(
-        g_strdup_printf("mptimer/set_oneshot_counter_to_0 scaler=%d", *scaler),
-                        scaler, test_timer_set_oneshot_counter_to_0);
-    qtest_add_data_func(
-        g_strdup_printf("mptimer/set_periodic_counter_to_0 scaler=%d", *scaler),
-                        scaler, test_timer_set_periodic_counter_to_0);
-    qtest_add_data_func(
-        g_strdup_printf("mptimer/noload_oneshot scaler=%d", *scaler),
-                        scaler, test_timer_noload_oneshot);
-    qtest_add_data_func(
-        g_strdup_printf("mptimer/noload_periodic scaler=%d", *scaler),
-                        scaler, test_timer_noload_periodic);
-    qtest_add_data_func(
-        g_strdup_printf("mptimer/zero_load_oneshot scaler=%d", *scaler),
-                        scaler, test_timer_zero_load_oneshot);
-    qtest_add_data_func(
-        g_strdup_printf("mptimer/zero_load_periodic scaler=%d", *scaler),
-                        scaler, test_timer_zero_load_periodic);
-    qtest_add_data_func(
-        g_strdup_printf("mptimer/zero_load_oneshot_to_nonzero scaler=%d", *scaler),
-                        scaler, test_timer_zero_load_oneshot_to_nonzero);
-    qtest_add_data_func(
-        g_strdup_printf("mptimer/zero_load_periodic_to_nonzero scaler=%d", *scaler),
-                        scaler, test_timer_zero_load_periodic_to_nonzero);
-    qtest_add_data_func(
-        g_strdup_printf("mptimer/nonzero_load_oneshot_to_zero scaler=%d", *scaler),
-                        scaler, test_timer_nonzero_load_oneshot_to_zero);
-    qtest_add_data_func(
-        g_strdup_printf("mptimer/nonzero_load_periodic_to_zero scaler=%d", *scaler),
-                        scaler, test_timer_nonzero_load_periodic_to_zero);
-    qtest_add_data_func(
-        g_strdup_printf("mptimer/set_periodic_counter_on_the_fly scaler=%d", *scaler),
-                        scaler, test_timer_set_periodic_counter_on_the_fly);
-    qtest_add_data_func(
-        g_strdup_printf("mptimer/enable_and_set_counter scaler=%d", *scaler),
-                        scaler, test_timer_enable_and_set_counter);
-    qtest_add_data_func(
-        g_strdup_printf("mptimer/set_counter_and_enable scaler=%d", *scaler),
-                        scaler, test_timer_set_counter_and_enable);
-    qtest_add_data_func(
-        g_strdup_printf("mptimer/oneshot_with_counter_0_on_start scaler=%d", *scaler),
-                        scaler, test_timer_oneshot_with_counter_0_on_start);
-    qtest_add_data_func(
-        g_strdup_printf("mptimer/periodic_with_counter_0_on_start scaler=%d", *scaler),
-                        scaler, test_timer_periodic_with_counter_0_on_start);
-    qtest_add_data_func(
-        g_strdup_printf("mptimer/periodic_counter scaler=%d", *scaler),
-                        scaler, test_periodic_counter);
-    qtest_add_data_func(
-        g_strdup_printf("mptimer/set_counter_periodic_with_zero_load scaler=%d", *scaler),
-                        scaler, test_timer_set_counter_periodic_with_zero_load);
-    qtest_add_data_func(
-        g_strdup_printf("mptimer/set_oneshot_load_to_0 scaler=%d", *scaler),
-                        scaler, test_timer_set_oneshot_load_to_0);
-    qtest_add_data_func(
-        g_strdup_printf("mptimer/set_periodic_load_to_0 scaler=%d", *scaler),
-                        scaler, test_timer_set_periodic_load_to_0);
-    qtest_add_data_func(
-        g_strdup_printf("mptimer/zero_load_mode_switch scaler=%d", *scaler),
-                        scaler, test_timer_zero_load_mode_switch);
-
-    if (scaler == &nonscaled) {
-        scaler = &scaled;
-        goto tests_with_prescaler_arg;
+    for (scale = 0; scale < 2; scale++) {
+        add_scaler_test("mptimer/oneshot scaler",
+                        scale, test_timer_oneshot);
+        add_scaler_test("mptimer/pause scaler",
+                        scale, test_timer_pause);
+        add_scaler_test("mptimer/reload scaler",
+                        scale, test_timer_reload);
+        add_scaler_test("mptimer/periodic scaler",
+                        scale, test_timer_periodic);
+        add_scaler_test("mptimer/oneshot_to_periodic scaler",
+                        scale, test_timer_oneshot_to_periodic);
+        add_scaler_test("mptimer/periodic_to_oneshot scaler",
+                        scale, test_timer_periodic_to_oneshot);
+        add_scaler_test("mptimer/set_oneshot_counter_to_0 scaler",
+                        scale, test_timer_set_oneshot_counter_to_0);
+        add_scaler_test("mptimer/set_periodic_counter_to_0 scaler",
+                        scale, test_timer_set_periodic_counter_to_0);
+        add_scaler_test("mptimer/noload_oneshot scaler",
+                        scale, test_timer_noload_oneshot);
+        add_scaler_test("mptimer/noload_periodic scaler",
+                        scale, test_timer_noload_periodic);
+        add_scaler_test("mptimer/zero_load_oneshot scaler",
+                        scale, test_timer_zero_load_oneshot);
+        add_scaler_test("mptimer/zero_load_periodic scaler",
+                        scale, test_timer_zero_load_periodic);
+        add_scaler_test("mptimer/zero_load_oneshot_to_nonzero scaler",
+                        scale, test_timer_zero_load_oneshot_to_nonzero);
+        add_scaler_test("mptimer/zero_load_periodic_to_nonzero scaler",
+                        scale, test_timer_zero_load_periodic_to_nonzero);
+        add_scaler_test("mptimer/nonzero_load_oneshot_to_zero scaler",
+                        scale, test_timer_nonzero_load_oneshot_to_zero);
+        add_scaler_test("mptimer/nonzero_load_periodic_to_zero scaler",
+                        scale, test_timer_nonzero_load_periodic_to_zero);
+        add_scaler_test("mptimer/set_periodic_counter_on_the_fly scaler",
+                        scale, test_timer_set_periodic_counter_on_the_fly);
+        add_scaler_test("mptimer/enable_and_set_counter scaler",
+                        scale, test_timer_enable_and_set_counter);
+        add_scaler_test("mptimer/set_counter_and_enable scaler",
+                        scale, test_timer_set_counter_and_enable);
+        add_scaler_test("mptimer/oneshot_with_counter_0_on_start scaler",
+                        scale, test_timer_oneshot_with_counter_0_on_start);
+        add_scaler_test("mptimer/periodic_with_counter_0_on_start scaler",
+                        scale, test_timer_periodic_with_counter_0_on_start);
+        add_scaler_test("mptimer/periodic_counter scaler",
+                        scale, test_periodic_counter);
+        add_scaler_test("mptimer/set_counter_periodic_with_zero_load scaler",
+                        scale, test_timer_set_counter_periodic_with_zero_load);
+        add_scaler_test("mptimer/set_oneshot_load_to_0 scaler",
+                        scale, test_timer_set_oneshot_load_to_0);
+        add_scaler_test("mptimer/set_periodic_load_to_0 scaler",
+                        scale, test_timer_set_periodic_load_to_0);
+        add_scaler_test("mptimer/zero_load_mode_switch scaler",
+                        scale, test_timer_zero_load_mode_switch);
     }
 
     qtest_start("-machine vexpress-a9");
-- 
2.19.2

Re: [Qemu-devel] [PATCH 4/5] tests/test-arm-mptimer: Don't leak string memory

[Richard Henderson]

On 12/4/18 7:29 AM, Peter Maydell wrote:
> The test-arm-mptimer setup creates a lot of test names using
> g_strdup_printf() and never frees them. This is entirely
> harmless since it's one-shot test code, but it clutters
> up the output from clang's LeakSanitizer. Refactor to
> use a helper function so we can free the memory.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  tests/test-arm-mptimer.c | 153 ++++++++++++++++++---------------------
>  1 file changed, 69 insertions(+), 84 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

Re: [Qemu-devel] [PATCH 4/5] tests/test-arm-mptimer: Don't leak string memory

[Philippe Mathieu-Daudé]

On 4/12/18 14:29, Peter Maydell wrote:
> The test-arm-mptimer setup creates a lot of test names using
> g_strdup_printf() and never frees them. This is entirely
> harmless since it's one-shot test code, but it clutters
> up the output from clang's LeakSanitizer. Refactor to
> use a helper function so we can free the memory.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

> ---
>  tests/test-arm-mptimer.c | 153 ++++++++++++++++++---------------------
>  1 file changed, 69 insertions(+), 84 deletions(-)
> 
> diff --git a/tests/test-arm-mptimer.c b/tests/test-arm-mptimer.c
> index cb8f2df9141..156a39f50dd 100644
> --- a/tests/test-arm-mptimer.c
> +++ b/tests/test-arm-mptimer.c
> @@ -991,10 +991,25 @@ static void test_timer_zero_load_nonscaled_periodic_to_prescaled_oneshot(void)
>      g_assert_cmpuint(timer_get_and_clr_int_sts(), ==, 0);
>  }
>  
> +/*
> + * Add a qtest test that comes in two versions: one with
> + * a timer scaler setting, and one with the timer nonscaled.
> + */
> +static void add_scaler_test(const char *str, bool scale,
> +                            void (*fn)(const void *))
> +{
> +    char *name;
> +    int *scaler = scale ? &scaled : &nonscaled;
> +
> +    name = g_strdup_printf("%s=%d", str, *scaler);
> +    qtest_add_data_func(name, scaler, fn);
> +    g_free(name);
> +}
> +
>  int main(int argc, char **argv)
>  {
> -    int *scaler = &nonscaled;
>      int ret;
> +    int scale;
>  
>      g_test_init(&argc, &argv, NULL);
>  
> @@ -1012,89 +1027,59 @@ int main(int argc, char **argv)
>      qtest_add_func("mptimer/prescaler", test_timer_prescaler);
>      qtest_add_func("mptimer/prescaler_on_the_fly", test_timer_prescaler_on_the_fly);
>  
> -tests_with_prescaler_arg:
> -    qtest_add_data_func(
> -        g_strdup_printf("mptimer/oneshot scaler=%d", *scaler),
> -                        scaler, test_timer_oneshot);
> -    qtest_add_data_func(
> -        g_strdup_printf("mptimer/pause scaler=%d", *scaler),
> -                        scaler, test_timer_pause);
> -    qtest_add_data_func(
> -        g_strdup_printf("mptimer/reload scaler=%d", *scaler),
> -                        scaler, test_timer_reload);
> -    qtest_add_data_func(
> -        g_strdup_printf("mptimer/periodic scaler=%d", *scaler),
> -                        scaler, test_timer_periodic);
> -    qtest_add_data_func(
> -        g_strdup_printf("mptimer/oneshot_to_periodic scaler=%d", *scaler),
> -                        scaler, test_timer_oneshot_to_periodic);
> -    qtest_add_data_func(
> -        g_strdup_printf("mptimer/periodic_to_oneshot scaler=%d", *scaler),
> -                        scaler, test_timer_periodic_to_oneshot);
> -    qtest_add_data_func(
> -        g_strdup_printf("mptimer/set_oneshot_counter_to_0 scaler=%d", *scaler),
> -                        scaler, test_timer_set_oneshot_counter_to_0);
> -    qtest_add_data_func(
> -        g_strdup_printf("mptimer/set_periodic_counter_to_0 scaler=%d", *scaler),
> -                        scaler, test_timer_set_periodic_counter_to_0);
> -    qtest_add_data_func(
> -        g_strdup_printf("mptimer/noload_oneshot scaler=%d", *scaler),
> -                        scaler, test_timer_noload_oneshot);
> -    qtest_add_data_func(
> -        g_strdup_printf("mptimer/noload_periodic scaler=%d", *scaler),
> -                        scaler, test_timer_noload_periodic);
> -    qtest_add_data_func(
> -        g_strdup_printf("mptimer/zero_load_oneshot scaler=%d", *scaler),
> -                        scaler, test_timer_zero_load_oneshot);
> -    qtest_add_data_func(
> -        g_strdup_printf("mptimer/zero_load_periodic scaler=%d", *scaler),
> -                        scaler, test_timer_zero_load_periodic);
> -    qtest_add_data_func(
> -        g_strdup_printf("mptimer/zero_load_oneshot_to_nonzero scaler=%d", *scaler),
> -                        scaler, test_timer_zero_load_oneshot_to_nonzero);
> -    qtest_add_data_func(
> -        g_strdup_printf("mptimer/zero_load_periodic_to_nonzero scaler=%d", *scaler),
> -                        scaler, test_timer_zero_load_periodic_to_nonzero);
> -    qtest_add_data_func(
> -        g_strdup_printf("mptimer/nonzero_load_oneshot_to_zero scaler=%d", *scaler),
> -                        scaler, test_timer_nonzero_load_oneshot_to_zero);
> -    qtest_add_data_func(
> -        g_strdup_printf("mptimer/nonzero_load_periodic_to_zero scaler=%d", *scaler),
> -                        scaler, test_timer_nonzero_load_periodic_to_zero);
> -    qtest_add_data_func(
> -        g_strdup_printf("mptimer/set_periodic_counter_on_the_fly scaler=%d", *scaler),
> -                        scaler, test_timer_set_periodic_counter_on_the_fly);
> -    qtest_add_data_func(
> -        g_strdup_printf("mptimer/enable_and_set_counter scaler=%d", *scaler),
> -                        scaler, test_timer_enable_and_set_counter);
> -    qtest_add_data_func(
> -        g_strdup_printf("mptimer/set_counter_and_enable scaler=%d", *scaler),
> -                        scaler, test_timer_set_counter_and_enable);
> -    qtest_add_data_func(
> -        g_strdup_printf("mptimer/oneshot_with_counter_0_on_start scaler=%d", *scaler),
> -                        scaler, test_timer_oneshot_with_counter_0_on_start);
> -    qtest_add_data_func(
> -        g_strdup_printf("mptimer/periodic_with_counter_0_on_start scaler=%d", *scaler),
> -                        scaler, test_timer_periodic_with_counter_0_on_start);
> -    qtest_add_data_func(
> -        g_strdup_printf("mptimer/periodic_counter scaler=%d", *scaler),
> -                        scaler, test_periodic_counter);
> -    qtest_add_data_func(
> -        g_strdup_printf("mptimer/set_counter_periodic_with_zero_load scaler=%d", *scaler),
> -                        scaler, test_timer_set_counter_periodic_with_zero_load);
> -    qtest_add_data_func(
> -        g_strdup_printf("mptimer/set_oneshot_load_to_0 scaler=%d", *scaler),
> -                        scaler, test_timer_set_oneshot_load_to_0);
> -    qtest_add_data_func(
> -        g_strdup_printf("mptimer/set_periodic_load_to_0 scaler=%d", *scaler),
> -                        scaler, test_timer_set_periodic_load_to_0);
> -    qtest_add_data_func(
> -        g_strdup_printf("mptimer/zero_load_mode_switch scaler=%d", *scaler),
> -                        scaler, test_timer_zero_load_mode_switch);
> -
> -    if (scaler == &nonscaled) {
> -        scaler = &scaled;
> -        goto tests_with_prescaler_arg;
> +    for (scale = 0; scale < 2; scale++) {
> +        add_scaler_test("mptimer/oneshot scaler",
> +                        scale, test_timer_oneshot);
> +        add_scaler_test("mptimer/pause scaler",
> +                        scale, test_timer_pause);
> +        add_scaler_test("mptimer/reload scaler",
> +                        scale, test_timer_reload);
> +        add_scaler_test("mptimer/periodic scaler",
> +                        scale, test_timer_periodic);
> +        add_scaler_test("mptimer/oneshot_to_periodic scaler",
> +                        scale, test_timer_oneshot_to_periodic);
> +        add_scaler_test("mptimer/periodic_to_oneshot scaler",
> +                        scale, test_timer_periodic_to_oneshot);
> +        add_scaler_test("mptimer/set_oneshot_counter_to_0 scaler",
> +                        scale, test_timer_set_oneshot_counter_to_0);
> +        add_scaler_test("mptimer/set_periodic_counter_to_0 scaler",
> +                        scale, test_timer_set_periodic_counter_to_0);
> +        add_scaler_test("mptimer/noload_oneshot scaler",
> +                        scale, test_timer_noload_oneshot);
> +        add_scaler_test("mptimer/noload_periodic scaler",
> +                        scale, test_timer_noload_periodic);
> +        add_scaler_test("mptimer/zero_load_oneshot scaler",
> +                        scale, test_timer_zero_load_oneshot);
> +        add_scaler_test("mptimer/zero_load_periodic scaler",
> +                        scale, test_timer_zero_load_periodic);
> +        add_scaler_test("mptimer/zero_load_oneshot_to_nonzero scaler",
> +                        scale, test_timer_zero_load_oneshot_to_nonzero);
> +        add_scaler_test("mptimer/zero_load_periodic_to_nonzero scaler",
> +                        scale, test_timer_zero_load_periodic_to_nonzero);
> +        add_scaler_test("mptimer/nonzero_load_oneshot_to_zero scaler",
> +                        scale, test_timer_nonzero_load_oneshot_to_zero);
> +        add_scaler_test("mptimer/nonzero_load_periodic_to_zero scaler",
> +                        scale, test_timer_nonzero_load_periodic_to_zero);
> +        add_scaler_test("mptimer/set_periodic_counter_on_the_fly scaler",
> +                        scale, test_timer_set_periodic_counter_on_the_fly);
> +        add_scaler_test("mptimer/enable_and_set_counter scaler",
> +                        scale, test_timer_enable_and_set_counter);
> +        add_scaler_test("mptimer/set_counter_and_enable scaler",
> +                        scale, test_timer_set_counter_and_enable);
> +        add_scaler_test("mptimer/oneshot_with_counter_0_on_start scaler",
> +                        scale, test_timer_oneshot_with_counter_0_on_start);
> +        add_scaler_test("mptimer/periodic_with_counter_0_on_start scaler",
> +                        scale, test_timer_periodic_with_counter_0_on_start);
> +        add_scaler_test("mptimer/periodic_counter scaler",
> +                        scale, test_periodic_counter);
> +        add_scaler_test("mptimer/set_counter_periodic_with_zero_load scaler",
> +                        scale, test_timer_set_counter_periodic_with_zero_load);
> +        add_scaler_test("mptimer/set_oneshot_load_to_0 scaler",
> +                        scale, test_timer_set_oneshot_load_to_0);
> +        add_scaler_test("mptimer/set_periodic_load_to_0 scaler",
> +                        scale, test_timer_set_periodic_load_to_0);
> +        add_scaler_test("mptimer/zero_load_mode_switch scaler",
> +                        scale, test_timer_zero_load_mode_switch);
>      }
>  
>      qtest_start("-machine vexpress-a9");
> 

[Qemu-devel] [PATCH 5/5] target/arm: Create timers in realize, not init

[Peter Maydell]

The timer_new() function allocates memory; this means that
if we call it in the CPU's init method we would need
to provide an instance_finalize method to free it. Defer
the timer creation to the realize function instead.

This fixes a memory leak spotted by clang LeakSanitizer
when a CPU object is created for introspection.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target/arm/cpu.c | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/target/arm/cpu.c b/target/arm/cpu.c
index b84a6c0e678..0e7138c9bfb 100644
--- a/target/arm/cpu.c
+++ b/target/arm/cpu.c
@@ -679,14 +679,6 @@ static void arm_cpu_initfn(Object *obj)
         qdev_init_gpio_in(DEVICE(cpu), arm_cpu_set_irq, 4);
     }
 
-    cpu->gt_timer[GTIMER_PHYS] = timer_new(QEMU_CLOCK_VIRTUAL, GTIMER_SCALE,
-                                                arm_gt_ptimer_cb, cpu);
-    cpu->gt_timer[GTIMER_VIRT] = timer_new(QEMU_CLOCK_VIRTUAL, GTIMER_SCALE,
-                                                arm_gt_vtimer_cb, cpu);
-    cpu->gt_timer[GTIMER_HYP] = timer_new(QEMU_CLOCK_VIRTUAL, GTIMER_SCALE,
-                                                arm_gt_htimer_cb, cpu);
-    cpu->gt_timer[GTIMER_SEC] = timer_new(QEMU_CLOCK_VIRTUAL, GTIMER_SCALE,
-                                                arm_gt_stimer_cb, cpu);
     qdev_init_gpio_out(DEVICE(cpu), cpu->gt_timer_outputs,
                        ARRAY_SIZE(cpu->gt_timer_outputs));
 
@@ -882,6 +874,15 @@ static void arm_cpu_realizefn(DeviceState *dev, Error **errp)
             return;
         }
     }
+
+    cpu->gt_timer[GTIMER_PHYS] = timer_new(QEMU_CLOCK_VIRTUAL, GTIMER_SCALE,
+                                           arm_gt_ptimer_cb, cpu);
+    cpu->gt_timer[GTIMER_VIRT] = timer_new(QEMU_CLOCK_VIRTUAL, GTIMER_SCALE,
+                                           arm_gt_vtimer_cb, cpu);
+    cpu->gt_timer[GTIMER_HYP] = timer_new(QEMU_CLOCK_VIRTUAL, GTIMER_SCALE,
+                                          arm_gt_htimer_cb, cpu);
+    cpu->gt_timer[GTIMER_SEC] = timer_new(QEMU_CLOCK_VIRTUAL, GTIMER_SCALE,
+                                          arm_gt_stimer_cb, cpu);
 #endif
 
     cpu_exec_realizefn(cs, &local_err);
-- 
2.19.2

Re: [Qemu-devel] [PATCH 5/5] target/arm: Create timers in realize, not init

[Richard Henderson]

On 12/4/18 7:29 AM, Peter Maydell wrote:
> The timer_new() function allocates memory; this means that
> if we call it in the CPU's init method we would need
> to provide an instance_finalize method to free it. Defer
> the timer creation to the realize function instead.
> 
> This fixes a memory leak spotted by clang LeakSanitizer
> when a CPU object is created for introspection.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  target/arm/cpu.c | 17 +++++++++--------
>  1 file changed, 9 insertions(+), 8 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~