hw/sparc/leon3: Fix wrong usage of DO_UPCAST macro

[PATCH] hw/sparc/leon3: Fix wrong usage of DO_UPCAST macro

Posted by Thomas Huth 1 year, 11 months ago

leon3.c currently fails to compile with some compilers when the -Wvla
option has been enabled:

 ../hw/sparc/leon3.c: In function ‘leon3_cpu_reset’:
 ../hw/sparc/leon3.c:153:5: error: ISO C90 forbids variable length array
  ‘offset_must_be_zero’ [-Werror=vla]
   153 |     ResetData *s = (ResetData *)DO_UPCAST(ResetData, info[id], info);
       |     ^~~~~~~~~
 cc1: all warnings being treated as errors

Looking at this code, the DO_UPCAST macro is indeed used in a wrong way
here: DO_UPCAST is supposed to check that the second parameter is the
first entry of the struct that the first parameter indicates, but since
we use and index into the info[] array, this of course cannot work.

The intention here was likely rather to use the container_of() macro
instead, so switch the code accordingly.

Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 hw/sparc/leon3.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/sparc/leon3.c b/hw/sparc/leon3.c
index 4873b59b6c..6aaa04cb19 100644
--- a/hw/sparc/leon3.c
+++ b/hw/sparc/leon3.c
@@ -150,7 +150,7 @@ static void leon3_cpu_reset(void *opaque)
 {
     struct CPUResetData *info = (struct CPUResetData *) opaque;
     int id = info->id;
-    ResetData *s = (ResetData *)DO_UPCAST(ResetData, info[id], info);
+    ResetData *s = container_of(info, ResetData, info[id]);
     CPUState *cpu = CPU(s->info[id].cpu);
     CPUSPARCState *env = cpu_env(cpu);
 
-- 
2.43.2

Re: [PATCH] hw/sparc/leon3: Fix wrong usage of DO_UPCAST macro

Posted by Philippe Mathieu-Daudé 1 year, 11 months ago

On 21/2/24 19:07, Thomas Huth wrote:
> leon3.c currently fails to compile with some compilers when the -Wvla
> option has been enabled:
> 
>   ../hw/sparc/leon3.c: In function ‘leon3_cpu_reset’:
>   ../hw/sparc/leon3.c:153:5: error: ISO C90 forbids variable length array
>    ‘offset_must_be_zero’ [-Werror=vla]
>     153 |     ResetData *s = (ResetData *)DO_UPCAST(ResetData, info[id], info);
>         |     ^~~~~~~~~
>   cc1: all warnings being treated as errors
> 
> Looking at this code, the DO_UPCAST macro is indeed used in a wrong way
> here: DO_UPCAST is supposed to check that the second parameter is the
> first entry of the struct that the first parameter indicates, but since
> we use and index into the info[] array, this of course cannot work.
> 
> The intention here was likely rather to use the container_of() macro
> instead, so switch the code accordingly.
> 
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>   hw/sparc/leon3.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH] hw/sparc/leon3: Fix wrong usage of DO_UPCAST macro

Posted by Philippe Mathieu-Daudé 1 year, 11 months ago

On 21/2/24 19:47, Philippe Mathieu-Daudé wrote:
> On 21/2/24 19:07, Thomas Huth wrote:
>> leon3.c currently fails to compile with some compilers when the -Wvla
>> option has been enabled:
>>
>>   ../hw/sparc/leon3.c: In function ‘leon3_cpu_reset’:
>>   ../hw/sparc/leon3.c:153:5: error: ISO C90 forbids variable length array
>>    ‘offset_must_be_zero’ [-Werror=vla]
>>     153 |     ResetData *s = (ResetData *)DO_UPCAST(ResetData, 
>> info[id], info);
>>         |     ^~~~~~~~~
>>   cc1: all warnings being treated as errors
>>
>> Looking at this code, the DO_UPCAST macro is indeed used in a wrong way
>> here: DO_UPCAST is supposed to check that the second parameter is the
>> first entry of the struct that the first parameter indicates, but since
>> we use and index into the info[] array, this of course cannot work.
>>
>> The intention here was likely rather to use the container_of() macro
>> instead, so switch the code accordingly.

Fixes: d65aba8286 ("hw/sparc/leon3: implement multiprocessor")

>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>> ---
>>   hw/sparc/leon3.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>

Re: [PATCH] hw/sparc/leon3: Fix wrong usage of DO_UPCAST macro

Posted by Philippe Mathieu-Daudé 1 year, 11 months ago

On 21/2/24 19:49, Philippe Mathieu-Daudé wrote:
> On 21/2/24 19:47, Philippe Mathieu-Daudé wrote:
>> On 21/2/24 19:07, Thomas Huth wrote:
>>> leon3.c currently fails to compile with some compilers when the -Wvla
>>> option has been enabled:
>>>
>>>   ../hw/sparc/leon3.c: In function ‘leon3_cpu_reset’:
>>>   ../hw/sparc/leon3.c:153:5: error: ISO C90 forbids variable length 
>>> array
>>>    ‘offset_must_be_zero’ [-Werror=vla]
>>>     153 |     ResetData *s = (ResetData *)DO_UPCAST(ResetData, 
>>> info[id], info);
>>>         |     ^~~~~~~~~
>>>   cc1: all warnings being treated as errors
>>>
>>> Looking at this code, the DO_UPCAST macro is indeed used in a wrong way
>>> here: DO_UPCAST is supposed to check that the second parameter is the
>>> first entry of the struct that the first parameter indicates, but since
>>> we use and index into the info[] array, this of course cannot work.
>>>
>>> The intention here was likely rather to use the container_of() macro
>>> instead, so switch the code accordingly.
> 
> Fixes: d65aba8286 ("hw/sparc/leon3: implement multiprocessor")
> 
>>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>>> ---
>>>   hw/sparc/leon3.c | 2 +-
>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>>

Patch queued!

Re: [PATCH] hw/sparc/leon3: Fix wrong usage of DO_UPCAST macro

Posted by Clément Chigot 1 year, 11 months ago

Hi Philippe, Thomas

Thanks for handling that !
And I do confirm that it does not trigger any obvious regression on our side.

Thanks,
Clément

On Thu, Feb 22, 2024 at 8:46 AM Philippe Mathieu-Daudé
<philmd@linaro.org> wrote:
>
> On 21/2/24 19:49, Philippe Mathieu-Daudé wrote:
> > On 21/2/24 19:47, Philippe Mathieu-Daudé wrote:
> >> On 21/2/24 19:07, Thomas Huth wrote:
> >>> leon3.c currently fails to compile with some compilers when the -Wvla
> >>> option has been enabled:
> >>>
> >>>   ../hw/sparc/leon3.c: In function ‘leon3_cpu_reset’:
> >>>   ../hw/sparc/leon3.c:153:5: error: ISO C90 forbids variable length
> >>> array
> >>>    ‘offset_must_be_zero’ [-Werror=vla]
> >>>     153 |     ResetData *s = (ResetData *)DO_UPCAST(ResetData,
> >>> info[id], info);
> >>>         |     ^~~~~~~~~
> >>>   cc1: all warnings being treated as errors
> >>>
> >>> Looking at this code, the DO_UPCAST macro is indeed used in a wrong way
> >>> here: DO_UPCAST is supposed to check that the second parameter is the
> >>> first entry of the struct that the first parameter indicates, but since
> >>> we use and index into the info[] array, this of course cannot work.
> >>>
> >>> The intention here was likely rather to use the container_of() macro
> >>> instead, so switch the code accordingly.
> >
> > Fixes: d65aba8286 ("hw/sparc/leon3: implement multiprocessor")
> >
> >>> Signed-off-by: Thomas Huth <thuth@redhat.com>
> >>> ---
> >>>   hw/sparc/leon3.c | 2 +-
> >>>   1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> >>
>
> Patch queued!
>

Re: [PATCH] hw/sparc/leon3: Fix wrong usage of DO_UPCAST macro

Posted by Philippe Mathieu-Daudé 1 year, 11 months ago

On 22/2/24 09:26, Clément Chigot wrote:
> Hi Philippe, Thomas
> 
> Thanks for handling that !
> And I do confirm that it does not trigger any obvious regression on our side.

Since I screwed my hw-misc pull request, I'll add your Tested-by to the
commit before respin.

> 
> Thanks,
> Clément
> 
> On Thu, Feb 22, 2024 at 8:46 AM Philippe Mathieu-Daudé
> <philmd@linaro.org> wrote:
>>
>> On 21/2/24 19:49, Philippe Mathieu-Daudé wrote:
>>> On 21/2/24 19:47, Philippe Mathieu-Daudé wrote:
>>>> On 21/2/24 19:07, Thomas Huth wrote:
>>>>> leon3.c currently fails to compile with some compilers when the -Wvla
>>>>> option has been enabled:
>>>>>
>>>>>    ../hw/sparc/leon3.c: In function ‘leon3_cpu_reset’:
>>>>>    ../hw/sparc/leon3.c:153:5: error: ISO C90 forbids variable length
>>>>> array
>>>>>     ‘offset_must_be_zero’ [-Werror=vla]
>>>>>      153 |     ResetData *s = (ResetData *)DO_UPCAST(ResetData,
>>>>> info[id], info);
>>>>>          |     ^~~~~~~~~
>>>>>    cc1: all warnings being treated as errors
>>>>>
>>>>> Looking at this code, the DO_UPCAST macro is indeed used in a wrong way
>>>>> here: DO_UPCAST is supposed to check that the second parameter is the
>>>>> first entry of the struct that the first parameter indicates, but since
>>>>> we use and index into the info[] array, this of course cannot work.
>>>>>
>>>>> The intention here was likely rather to use the container_of() macro
>>>>> instead, so switch the code accordingly.
>>>
>>> Fixes: d65aba8286 ("hw/sparc/leon3: implement multiprocessor")
>>>
>>>>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>>>>> ---
>>>>>    hw/sparc/leon3.c | 2 +-
>>>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>>>>
>>
>> Patch queued!
>>