Re: [Qemu-devel] [PATCH] target-arm: fix build on fedora

[Michael S. Tsirkin]

On Mon, Dec 23, 2013 at 12:37:41PM +0000, Peter Maydell wrote:
> On 23 December 2013 11:56, Michael S. Tsirkin <address@hidden> wrote:
> > commit 5ce4f35781028ce1aee3341e6002f925fdc7aaf3
> >     "target-arm: A64: add set_pc cpu method"
> >
> > introduces an array aarch64_cpus which is zero
> > size if this code is built without CONFIG_USER_ONLY.
> > In particular an attempt to iterate over this array produces a warning:
> >
> >  CC    aarch64-softmmu/target-arm/cpu64.o
> > /scm/qemu/target-arm/cpu64.c: In function ‘aarch64_cpu_register_types’:
> > /scm/qemu/target-arm/cpu64.c:124:5: error: comparison of unsigned
> > expression < 0 is always false [-Werror=type-limits]
> >      for (i = 0; i < ARRAY_SIZE(aarch64_cpus); i++) {
> >      ^
> > cc1: all warnings being treated as errors
> >
> > This is the result of ARRAY_SIZE being an unsigned type,
> > causing i to be promoted to unsigned int as well.
> 
> I guess this is a new gcc warning, since this all builds
> fine for me (gcc 4.6.3).

I see this with gcc 4.8.2 on Fedora 19.

> > As zero size arrays are a gcc extension, it seems
> > cleanest to add a dummy element with NULL name,
> > and test for it during registration.
> >
> > Cc: Alexander Graf <address@hidden>
> > Cc: Peter Maydell <address@hidden>
> > Cc: Richard Henderson <address@hidden>
> > Signed-off-by: Michael S. Tsirkin <address@hidden>
> > ---
> >
> > I have queued this in my tree since it prevents me from
> > being able to build and test properly.
> > Pls review and ack.
> >
> >  target-arm/cpu64.c | 5 +++++
> >  1 file changed, 5 insertions(+)
> >
> > diff --git a/target-arm/cpu64.c b/target-arm/cpu64.c
> > index 04ce879..2efe189 100644
> > --- a/target-arm/cpu64.c
> > +++ b/target-arm/cpu64.c
> > @@ -58,6 +58,7 @@ static const ARMCPUInfo aarch64_cpus[] = {
> >  #ifdef CONFIG_USER_ONLY
> >      { .name = "any",         .initfn = aarch64_any_initfn },
> >  #endif
> > +    { .name = NULL }
> >  };
> >
> >  static void aarch64_cpu_initfn(Object *obj)
> > @@ -100,6 +101,10 @@ static void aarch64_cpu_register(const ARMCPUInfo 
> > *info)
> >          .class_init = info->class_init,
> >      };
> >
> > +    if (!info->name) {
> > +        return;
> > +    }
> > +
> >      type_info.name = g_strdup_printf("%s-" TYPE_ARM_CPU, info->name);
> >      type_register(&type_info);
> >      g_free((void *)type_info.name);
> 
> At a minimum, if we take this approach we should add TODO comments
> to the effect that the NULL terminator and the if() can be removed
> when the first real AArch64 CPU is added.
> 
> I think I'd rather put the if (!info->name) continue into the function
> which is doing the looping over the array.
> 
> thanks
> -- PMM