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

[Andreas Färber]

Am 23.12.2013 13:37, schrieb Peter Maydell:
> 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).

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 <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.

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

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg