| Message ID | 20170724182751.18261-23-f4bug@amsat.org |
|---|---|
| State | New |
| Headers | show |
On 24 July 2017 at 19:27, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote: > Reorder to only allocate if required. > > hw/arm/vexpress.c:667:13: warning: Potential leak of memory pointed to by 'flashalias' > > Reported-by: Clang Static Analyzer > Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> > --- > hw/arm/vexpress.c | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/hw/arm/vexpress.c b/hw/arm/vexpress.c > index 528c65ddb6..76c4d84482 100644 > --- a/hw/arm/vexpress.c > +++ b/hw/arm/vexpress.c > @@ -553,8 +553,6 @@ static void vexpress_common_init(MachineState *machine) > MemoryRegion *sysmem = get_system_memory(); > MemoryRegion *vram = g_new(MemoryRegion, 1); > MemoryRegion *sram = g_new(MemoryRegion, 1); > - MemoryRegion *flashalias = g_new(MemoryRegion, 1); > - MemoryRegion *flash0mem; > const hwaddr *map = daughterboard->motherboard_map; > int i; > > @@ -657,6 +655,9 @@ static void vexpress_common_init(MachineState *machine) > } > > if (map[VE_NORFLASHALIAS] != -1) { > + MemoryRegion *flashalias = g_new(MemoryRegion, 1); > + MemoryRegion *flash0mem; > + > /* Map flash 0 as an alias into low memory */ > flash0mem = sysbus_mmio_get_region(SYS_BUS_DEVICE(pflash0), 0); > memory_region_init_alias(flashalias, NULL, "vexpress.flashalias", > -- > 2.13.3 flashalias is no different from any of the other MemoryRegion* variables we allocate here with g_new() -- they all leak, and it doesn't matter because the function runs once and we want the MemoryRegions to survive until the end of the simulation. These days we could in theory clean this up by switching from doing g_new() to having a set of MemoryRegion fields in the VexpressMachineState struct (compare hw/arm/mps2.c). thanks -- PMM
On 07/24/2017 06:11 PM, Peter Maydell wrote: > On 24 July 2017 at 19:27, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote: >> Reorder to only allocate if required. >> >> hw/arm/vexpress.c:667:13: warning: Potential leak of memory pointed to by 'flashalias' >> >> Reported-by: Clang Static Analyzer >> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> >> --- >> hw/arm/vexpress.c | 5 +++-- >> 1 file changed, 3 insertions(+), 2 deletions(-) >> >> diff --git a/hw/arm/vexpress.c b/hw/arm/vexpress.c >> index 528c65ddb6..76c4d84482 100644 >> --- a/hw/arm/vexpress.c >> +++ b/hw/arm/vexpress.c >> @@ -553,8 +553,6 @@ static void vexpress_common_init(MachineState *machine) >> MemoryRegion *sysmem = get_system_memory(); >> MemoryRegion *vram = g_new(MemoryRegion, 1); >> MemoryRegion *sram = g_new(MemoryRegion, 1); >> - MemoryRegion *flashalias = g_new(MemoryRegion, 1); >> - MemoryRegion *flash0mem; >> const hwaddr *map = daughterboard->motherboard_map; >> int i; >> >> @@ -657,6 +655,9 @@ static void vexpress_common_init(MachineState *machine) >> } >> >> if (map[VE_NORFLASHALIAS] != -1) { >> + MemoryRegion *flashalias = g_new(MemoryRegion, 1); >> + MemoryRegion *flash0mem; >> + >> /* Map flash 0 as an alias into low memory */ >> flash0mem = sysbus_mmio_get_region(SYS_BUS_DEVICE(pflash0), 0); >> memory_region_init_alias(flashalias, NULL, "vexpress.flashalias", >> -- >> 2.13.3 > > flashalias is no different from any of the other MemoryRegion* > variables we allocate here with g_new() -- they all > leak, and it doesn't matter because the function > runs once and we want the MemoryRegions to survive > until the end of the simulation. > > These days we could in theory clean this up by > switching from doing g_new() to having a set of > MemoryRegion fields in the VexpressMachineState > struct (compare hw/arm/mps2.c). I agree with the analyzer here, why allocate something you'll never use? However this is not the case here since in your commit 6ec1588e097 both legacy/a9 and aseries/a15 use NORFLASHALIAS to remap their flash at 0. Anyway patch dropped. Thanks, Phil.
diff --git a/hw/arm/vexpress.c b/hw/arm/vexpress.c index 528c65ddb6..76c4d84482 100644 --- a/hw/arm/vexpress.c +++ b/hw/arm/vexpress.c @@ -553,8 +553,6 @@ static void vexpress_common_init(MachineState *machine) MemoryRegion *sysmem = get_system_memory(); MemoryRegion *vram = g_new(MemoryRegion, 1); MemoryRegion *sram = g_new(MemoryRegion, 1); - MemoryRegion *flashalias = g_new(MemoryRegion, 1); - MemoryRegion *flash0mem; const hwaddr *map = daughterboard->motherboard_map; int i; @@ -657,6 +655,9 @@ static void vexpress_common_init(MachineState *machine) } if (map[VE_NORFLASHALIAS] != -1) { + MemoryRegion *flashalias = g_new(MemoryRegion, 1); + MemoryRegion *flash0mem; + /* Map flash 0 as an alias into low memory */ flash0mem = sysbus_mmio_get_region(SYS_BUS_DEVICE(pflash0), 0); memory_region_init_alias(flashalias, NULL, "vexpress.flashalias",
Reorder to only allocate if required. hw/arm/vexpress.c:667:13: warning: Potential leak of memory pointed to by 'flashalias' Reported-by: Clang Static Analyzer Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> --- hw/arm/vexpress.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)