1. Patchew
  2. QEMU
  3. View series

[PATCH] exynos4210_gic: Suppress gcc9 format-truncation warnings

David Gibson posted 1 patch 6 years, 1 month ago
Test checkpatch passed
Test docker-mingw@fedora passed
Test docker-quick@centos7 passed
Test docker-clang@ubuntu failed
Test asan passed
Test FreeBSD passed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20191004025509.3012-1-david@gibson.dropbear.id.au
Maintainers: Peter Maydell <peter.maydell@linaro.org>, Igor Mitsyanko <i.mitsyanko@gmail.com>
There is a newer version of this series
hw/intc/exynos4210_gic.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
[PATCH] exynos4210_gic: Suppress gcc9 format-truncation warnings
Posted by David Gibson 6 years, 1 month ago 
exynos4210_gic_realize() prints the number of cpus into some temporary
buffers, but it only allows 3 bytes space for it.  That's plenty - I'm
pretty sure that existing machines will only ever set this value to 2
(EXYNOS4210_NCPUS).  But the compiler can't really be expected to figure
that out.

Some[*] gcc9 versions therefore emit -Wformat-truncation warnings.  Fix
that by allowing more space in the temporary buffers - these are on stack
very briefly before being essentially strdup()ed inside the memory region
code, so there's not much cost to doing so.

[*] The bizarre thing here, is that I've long gotten these warnings
compiling in a 32-bit x86 container as host - Fedora 30 with
gcc-9.2.1-1.fc30.i686 - but it compiles just fine on my normal x86_64 host
- Fedora 30 with and gcc-9.2.1-1.fc30.x86_64.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 hw/intc/exynos4210_gic.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/intc/exynos4210_gic.c b/hw/intc/exynos4210_gic.c
index a1b699b6ba..2e5e47f9ec 100644
--- a/hw/intc/exynos4210_gic.c
+++ b/hw/intc/exynos4210_gic.c
@@ -290,8 +290,8 @@ static void exynos4210_gic_realize(DeviceState *dev, Error **errp)
     SysBusDevice *sbd = SYS_BUS_DEVICE(obj);
     const char cpu_prefix[] = "exynos4210-gic-alias_cpu";
     const char dist_prefix[] = "exynos4210-gic-alias_dist";
-    char cpu_alias_name[sizeof(cpu_prefix) + 3];
-    char dist_alias_name[sizeof(cpu_prefix) + 3];
+    char cpu_alias_name[sizeof(cpu_prefix) + 10];
+    char dist_alias_name[sizeof(cpu_prefix) + 10];
     SysBusDevice *gicbusdev;
     uint32_t i;
 
-- 
2.21.0
Re: [PATCH] exynos4210_gic: Suppress gcc9 format-truncation warnings
Posted by Philippe Mathieu-Daudé 6 years, 1 month ago 
On 10/4/19 4:55 AM, David Gibson wrote:
> exynos4210_gic_realize() prints the number of cpus into some temporary
> buffers, but it only allows 3 bytes space for it.  That's plenty - I'm
> pretty sure that existing machines will only ever set this value to 2
> (EXYNOS4210_NCPUS).  But the compiler can't really be expected to figure
> that out.
> 
> Some[*] gcc9 versions therefore emit -Wformat-truncation warnings.  Fix
> that by allowing more space in the temporary buffers - these are on stack
> very briefly before being essentially strdup()ed inside the memory region
> code, so there's not much cost to doing so.
> 
> [*] The bizarre thing here, is that I've long gotten these warnings
> compiling in a 32-bit x86 container as host - Fedora 30 with
> gcc-9.2.1-1.fc30.i686 - but it compiles just fine on my normal x86_64 host
> - Fedora 30 with and gcc-9.2.1-1.fc30.x86_64.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>   hw/intc/exynos4210_gic.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/intc/exynos4210_gic.c b/hw/intc/exynos4210_gic.c
> index a1b699b6ba..2e5e47f9ec 100644
> --- a/hw/intc/exynos4210_gic.c
> +++ b/hw/intc/exynos4210_gic.c
> @@ -290,8 +290,8 @@ static void exynos4210_gic_realize(DeviceState *dev, Error **errp)
>       SysBusDevice *sbd = SYS_BUS_DEVICE(obj);
>       const char cpu_prefix[] = "exynos4210-gic-alias_cpu";
>       const char dist_prefix[] = "exynos4210-gic-alias_dist";
> -    char cpu_alias_name[sizeof(cpu_prefix) + 3];
> -    char dist_alias_name[sizeof(cpu_prefix) + 3];
> +    char cpu_alias_name[sizeof(cpu_prefix) + 10];
> +    char dist_alias_name[sizeof(cpu_prefix) + 10];

Hmm magic again... So GCC provides a new warning with no helpful 
definitions about how to clean this :(

We already have:
#define UUID_FMT_LEN 36

What about adding/using UINT32_FMT_LEN?

>       SysBusDevice *gicbusdev;
>       uint32_t i;
>   
>
Re: [PATCH] exynos4210_gic: Suppress gcc9 format-truncation warnings
Posted by Peter Maydell 6 years ago 
On Fri, 4 Oct 2019 at 04:10, David Gibson <david@gibson.dropbear.id.au> wrote:
>
> exynos4210_gic_realize() prints the number of cpus into some temporary
> buffers, but it only allows 3 bytes space for it.  That's plenty - I'm
> pretty sure that existing machines will only ever set this value to 2
> (EXYNOS4210_NCPUS).  But the compiler can't really be expected to figure
> that out.
>
> Some[*] gcc9 versions therefore emit -Wformat-truncation warnings.  Fix
> that by allowing more space in the temporary buffers - these are on stack
> very briefly before being essentially strdup()ed inside the memory region
> code, so there's not much cost to doing so.
>
> [*] The bizarre thing here, is that I've long gotten these warnings
> compiling in a 32-bit x86 container as host - Fedora 30 with
> gcc-9.2.1-1.fc30.i686 - but it compiles just fine on my normal x86_64 host
> - Fedora 30 with and gcc-9.2.1-1.fc30.x86_64.
>
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>  hw/intc/exynos4210_gic.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/hw/intc/exynos4210_gic.c b/hw/intc/exynos4210_gic.c
> index a1b699b6ba..2e5e47f9ec 100644
> --- a/hw/intc/exynos4210_gic.c
> +++ b/hw/intc/exynos4210_gic.c
> @@ -290,8 +290,8 @@ static void exynos4210_gic_realize(DeviceState *dev, Error **errp)
>      SysBusDevice *sbd = SYS_BUS_DEVICE(obj);
>      const char cpu_prefix[] = "exynos4210-gic-alias_cpu";
>      const char dist_prefix[] = "exynos4210-gic-alias_dist";
> -    char cpu_alias_name[sizeof(cpu_prefix) + 3];
> -    char dist_alias_name[sizeof(cpu_prefix) + 3];
> +    char cpu_alias_name[sizeof(cpu_prefix) + 10];
> +    char dist_alias_name[sizeof(cpu_prefix) + 10];
>      SysBusDevice *gicbusdev;
>      uint32_t i;

If we assert() that num_cpu is always <= EXYNOS4210_NCPUS
is that sufficient to clue gcc in that the buffer can't overflow?

thanks
-- PMM
Re: [PATCH] exynos4210_gic: Suppress gcc9 format-truncation warnings
Posted by David Gibson 5 years, 11 months ago 
On Mon, Oct 14, 2019 at 01:51:39PM +0100, Peter Maydell wrote:
> On Fri, 4 Oct 2019 at 04:10, David Gibson <david@gibson.dropbear.id.au> wrote:
> >
> > exynos4210_gic_realize() prints the number of cpus into some temporary
> > buffers, but it only allows 3 bytes space for it.  That's plenty - I'm
> > pretty sure that existing machines will only ever set this value to 2
> > (EXYNOS4210_NCPUS).  But the compiler can't really be expected to figure
> > that out.
> >
> > Some[*] gcc9 versions therefore emit -Wformat-truncation warnings.  Fix
> > that by allowing more space in the temporary buffers - these are on stack
> > very briefly before being essentially strdup()ed inside the memory region
> > code, so there's not much cost to doing so.
> >
> > [*] The bizarre thing here, is that I've long gotten these warnings
> > compiling in a 32-bit x86 container as host - Fedora 30 with
> > gcc-9.2.1-1.fc30.i686 - but it compiles just fine on my normal x86_64 host
> > - Fedora 30 with and gcc-9.2.1-1.fc30.x86_64.
> >
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > ---
> >  hw/intc/exynos4210_gic.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/hw/intc/exynos4210_gic.c b/hw/intc/exynos4210_gic.c
> > index a1b699b6ba..2e5e47f9ec 100644
> > --- a/hw/intc/exynos4210_gic.c
> > +++ b/hw/intc/exynos4210_gic.c
> > @@ -290,8 +290,8 @@ static void exynos4210_gic_realize(DeviceState *dev, Error **errp)
> >      SysBusDevice *sbd = SYS_BUS_DEVICE(obj);
> >      const char cpu_prefix[] = "exynos4210-gic-alias_cpu";
> >      const char dist_prefix[] = "exynos4210-gic-alias_dist";
> > -    char cpu_alias_name[sizeof(cpu_prefix) + 3];
> > -    char dist_alias_name[sizeof(cpu_prefix) + 3];
> > +    char cpu_alias_name[sizeof(cpu_prefix) + 10];
> > +    char dist_alias_name[sizeof(cpu_prefix) + 10];
> >      SysBusDevice *gicbusdev;
> >      uint32_t i;
> 
> If we assert() that num_cpu is always <= EXYNOS4210_NCPUS
> is that sufficient to clue gcc in that the buffer can't overflow?

Interestingly, assert(s->num_cpu <= EXYNOS$210_NCPUS) is *not*
sufficient, but assert(i <= EXYNOS4210_NCPUS) within the loop *is*
enough.  I've updated my patch accordingly.

This isn't 4.2 material, obviously.  Should I just sit on it until 5.0
opens, or does one of you have someplace to stage the patch in the
meanwhile?

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson
Re: [PATCH] exynos4210_gic: Suppress gcc9 format-truncation warnings
Posted by Peter Maydell 5 years, 11 months ago 
On Wed, 20 Nov 2019 at 05:27, David Gibson <david@gibson.dropbear.id.au> wrote:
>
> On Mon, Oct 14, 2019 at 01:51:39PM +0100, Peter Maydell wrote:
> > If we assert() that num_cpu is always <= EXYNOS4210_NCPUS
> > is that sufficient to clue gcc in that the buffer can't overflow?
>
> Interestingly, assert(s->num_cpu <= EXYNOS$210_NCPUS) is *not*
> sufficient, but assert(i <= EXYNOS4210_NCPUS) within the loop *is*
> enough.  I've updated my patch accordingly.
>
> This isn't 4.2 material, obviously.  Should I just sit on it until 5.0
> opens, or does one of you have someplace to stage the patch in the
> meanwhile?

Easy fixes for compiler warnings aren't inherently out of scope
for 4.2. I'm also collecting stuff for 5.0 anyway so I suggest you
just send the patch.

-- PMM
Re: [PATCH] exynos4210_gic: Suppress gcc9 format-truncation warnings
Posted by David Gibson 5 years, 11 months ago 
On Wed, Nov 20, 2019 at 10:31:48AM +0000, Peter Maydell wrote:
> On Wed, 20 Nov 2019 at 05:27, David Gibson <david@gibson.dropbear.id.au> wrote:
> >
> > On Mon, Oct 14, 2019 at 01:51:39PM +0100, Peter Maydell wrote:
> > > If we assert() that num_cpu is always <= EXYNOS4210_NCPUS
> > > is that sufficient to clue gcc in that the buffer can't overflow?
> >
> > Interestingly, assert(s->num_cpu <= EXYNOS$210_NCPUS) is *not*
> > sufficient, but assert(i <= EXYNOS4210_NCPUS) within the loop *is*
> > enough.  I've updated my patch accordingly.
> >
> > This isn't 4.2 material, obviously.  Should I just sit on it until 5.0
> > opens, or does one of you have someplace to stage the patch in the
> > meanwhile?
> 
> Easy fixes for compiler warnings aren't inherently out of scope
> for 4.2. I'm also collecting stuff for 5.0 anyway so I suggest you
> just send the patch.

Ok, done.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

Patchew 2.1-devel-b67e4c2

© 2016 - 2025 Red Hat, Inc.