| Message ID | 20131223115622.GA6490@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 23 December 2013 11:56, Michael S. Tsirkin <mst@redhat.com> 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). > 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 <agraf@suse.de> > Cc: Peter Maydell <peter.maydell@linaro.org> > Cc: Richard Henderson <rth@twiddle.net> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > --- > > 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
Il 23/12/2013 13:37, Peter Maydell ha scritto: > 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. Or just change the termination condition from a check on the array size to one on info->name. Paolo
On 23 December 2013 12:50, Paolo Bonzini <pbonzini@redhat.com> wrote: > Il 23/12/2013 13:37, Peter Maydell ha scritto: >> 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. > > Or just change the termination condition from a check on the array size > to one on info->name. That would take it out of line with the equivalent 32 bit ARM code (and also moxie and openrisc for what little that's worth) and be fractionally more tedious to revert later. thanks -- PMM
Am 23.12.2013 13:37, schrieb Peter Maydell: > On 23 December 2013 11:56, Michael S. Tsirkin <mst@redhat.com> 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). No problem noticed with 4.8.1 on today's master either. >> 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 <agraf@suse.de> >> Cc: Peter Maydell <peter.maydell@linaro.org> >> Cc: Richard Henderson <rth@twiddle.net> >> Signed-off-by: Michael S. Tsirkin <mst@redhat.com> >> --- >> >> 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. While I share your sentiment wrt this workaround, what's the status of getting a real 64-bit CPU applied? Isn't the Cortex-A57/A53 CPU independent of whether we have all MPCore etc. pieces in place? That would seem the most elegant solution to me, even if not "usable" yet. Andreas
On 23 December 2013 13:45, Andreas Färber <afaerber@suse.de> wrote: > While I share your sentiment wrt this workaround, what's the status of > getting a real 64-bit CPU applied? Isn't the Cortex-A57/A53 CPU > independent of whether we have all MPCore etc. pieces in place? That > would seem the most elegant solution to me, even if not "usable" yet. It's next on my todo list after we get the usermode instruction emulation wrapped up, so should be sometime in the next few months, as part of the system emulation work. thanks -- PMM
On Mon, Dec 23, 2013 at 12:37:41PM +0000, Peter Maydell wrote: > On 23 December 2013 11:56, Michael S. Tsirkin <mst@redhat.com> 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 <agraf@suse.de> > > Cc: Peter Maydell <peter.maydell@linaro.org> > > Cc: Richard Henderson <rth@twiddle.net> > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > > --- > > > > 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
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);
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. 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 <agraf@suse.de> Cc: Peter Maydell <peter.maydell@linaro.org> Cc: Richard Henderson <rth@twiddle.net> Signed-off-by: Michael S. Tsirkin <mst@redhat.com> --- 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(+)