| Message ID | 20240221180751.190489-1-thuth@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | hw/sparc/leon3: Fix wrong usage of DO_UPCAST macro | expand |
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>
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> >
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!
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! >
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! >>
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);
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(-)