| Message ID | 20200526185132.1652355-1-eblake@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | or1k: Fix compilation hiccup | expand |
Patchew URL: https://patchew.org/QEMU/20200526185132.1652355-1-eblake@redhat.com/
Hi,
This series seems to have some coding style problems. See output below for
more information:
Message-id: 20200526185132.1652355-1-eblake@redhat.com
Subject: [PATCH] or1k: Fix compilation hiccup
Type: series
=== TEST SCRIPT BEGIN ===
#!/bin/bash
git rev-parse base > /dev/null || exit 0
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
./scripts/checkpatch.pl --mailback base..
=== TEST SCRIPT END ===
Switched to a new branch 'test'
d96d2fb or1k: Fix compilation hiccup
=== OUTPUT BEGIN ===
ERROR: spaces required around that '*' (ctx:WxV)
#33: FILE: hw/openrisc/openrisc_sim.c:132:
+ qemu_irq *cpu_irqs[2] = {};
^
total: 1 errors, 0 warnings, 8 lines checked
Commit d96d2fbbc5db (or1k: Fix compilation hiccup) has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
=== OUTPUT END ===
Test command exited with code: 1
The full log is available at
http://patchew.org/logs/20200526185132.1652355-1-eblake@redhat.com/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com
On 5/26/20 6:21 PM, no-reply@patchew.org wrote: > Patchew URL: https://patchew.org/QEMU/20200526185132.1652355-1-eblake@redhat.com/ > > > > Hi, > > This series seems to have some coding style problems. See output below for > more information: > > === OUTPUT BEGIN === > ERROR: spaces required around that '*' (ctx:WxV) > #33: FILE: hw/openrisc/openrisc_sim.c:132: > + qemu_irq *cpu_irqs[2] = {}; > ^ > > total: 1 errors, 0 warnings, 8 lines checked > > Commit d96d2fbbc5db (or1k: Fix compilation hiccup) has style problems, please review. If any of these errors > are false positives report them to the maintainer, see > CHECKPATCH in MAINTAINERS. False positive, due to 'qemu_irq' not following the normal naming conventions for typedefs.
On 26/05/2020 20.51, Eric Blake wrote: > On my Fedora 32 machine, gcc 10.1.1 at -O2 (the default for a bare > './configure') has a false-positive complaint: > > CC or1k-softmmu/hw/openrisc/openrisc_sim.o > /home/eblake/qemu/hw/openrisc/openrisc_sim.c: In function ‘openrisc_sim_init’: > /home/eblake/qemu/hw/openrisc/openrisc_sim.c:87:42: error: ‘cpu_irqs[0]’ may be used uninitialized in this function [-Werror=maybe-uninitialized] > 87 | sysbus_connect_irq(s, i, cpu_irqs[i][irq_pin]); > | ~~~~~~~~^~~ > > Initializing both pointers of cpu_irqs[] to NULL is sufficient to shut > up the compiler, even though they are definitely assigned in > openrisc_sim_init() prior to the inlined call to > openrisc_sim_ompic_init() containing the line in question. > > Signed-off-by: Eric Blake <eblake@redhat.com> > --- > hw/openrisc/openrisc_sim.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/hw/openrisc/openrisc_sim.c b/hw/openrisc/openrisc_sim.c > index d08ce6181199..95011a8015b4 100644 > --- a/hw/openrisc/openrisc_sim.c > +++ b/hw/openrisc/openrisc_sim.c > @@ -129,7 +129,7 @@ static void openrisc_sim_init(MachineState *machine) > const char *kernel_filename = machine->kernel_filename; > OpenRISCCPU *cpu = NULL; > MemoryRegion *ram; > - qemu_irq *cpu_irqs[2]; > + qemu_irq *cpu_irqs[2] = {}; > qemu_irq serial_irq; > int n; > unsigned int smp_cpus = machine->smp.cpus; > Reviewed-by: Thomas Huth <thuth@redhat.com>
On 5/26/20 8:51 PM, Eric Blake wrote: > On my Fedora 32 machine, gcc 10.1.1 at -O2 (the default for a bare > './configure') has a false-positive complaint: > > CC or1k-softmmu/hw/openrisc/openrisc_sim.o > /home/eblake/qemu/hw/openrisc/openrisc_sim.c: In function ‘openrisc_sim_init’: > /home/eblake/qemu/hw/openrisc/openrisc_sim.c:87:42: error: ‘cpu_irqs[0]’ may be used uninitialized in this function [-Werror=maybe-uninitialized] > 87 | sysbus_connect_irq(s, i, cpu_irqs[i][irq_pin]); > | ~~~~~~~~^~~ > > Initializing both pointers of cpu_irqs[] to NULL is sufficient to shut > up the compiler, even though they are definitely assigned in > openrisc_sim_init() prior to the inlined call to > openrisc_sim_ompic_init() containing the line in question. > > Signed-off-by: Eric Blake <eblake@redhat.com> > --- > hw/openrisc/openrisc_sim.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/hw/openrisc/openrisc_sim.c b/hw/openrisc/openrisc_sim.c > index d08ce6181199..95011a8015b4 100644 > --- a/hw/openrisc/openrisc_sim.c > +++ b/hw/openrisc/openrisc_sim.c > @@ -129,7 +129,7 @@ static void openrisc_sim_init(MachineState *machine) > const char *kernel_filename = machine->kernel_filename; > OpenRISCCPU *cpu = NULL; > MemoryRegion *ram; > - qemu_irq *cpu_irqs[2]; > + qemu_irq *cpu_irqs[2] = {}; Ah I now remembered why this warning sound familiar: https://bugs.launchpad.net/qemu/+bug/1874073 Peter suggested a different fix, I tested it, and was expecting Martin Liska to post an updated patch. > qemu_irq serial_irq; > int n; > unsigned int smp_cpus = machine->smp.cpus; >
On 2020-05-26 at 20:51 CEST, Eric Blake wrote... > On my Fedora 32 machine, gcc 10.1.1 at -O2 (the default for a bare > './configure') has a false-positive complaint: > > CC or1k-softmmu/hw/openrisc/openrisc_sim.o > /home/eblake/qemu/hw/openrisc/openrisc_sim.c: In function ‘openrisc_sim_init’: > /home/eblake/qemu/hw/openrisc/openrisc_sim.c:87:42: error: ‘cpu_irqs[0]’ may be used uninitialized in this function [-Werror=maybe-uninitialized] > 87 | sysbus_connect_irq(s, i, cpu_irqs[i][irq_pin]); > | ~~~~~~~~^~~ > > Initializing both pointers of cpu_irqs[] to NULL is sufficient to shut > up the compiler, even though they are definitely assigned in > openrisc_sim_init() prior to the inlined call to > openrisc_sim_ompic_init() containing the line in question. > > Signed-off-by: Eric Blake <eblake@redhat.com> > --- > hw/openrisc/openrisc_sim.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/hw/openrisc/openrisc_sim.c b/hw/openrisc/openrisc_sim.c > index d08ce6181199..95011a8015b4 100644 > --- a/hw/openrisc/openrisc_sim.c > +++ b/hw/openrisc/openrisc_sim.c > @@ -129,7 +129,7 @@ static void openrisc_sim_init(MachineState *machine) > const char *kernel_filename = machine->kernel_filename; > OpenRISCCPU *cpu = NULL; > MemoryRegion *ram; > - qemu_irq *cpu_irqs[2]; > + qemu_irq *cpu_irqs[2] = {}; Why is the value [2] correct here? The loop that initializes loops over machine->smp.cpus. Is it always less than 2 on this machine? > qemu_irq serial_irq; > int n; > unsigned int smp_cpus = machine->smp.cpus; -- Cheers, Christophe de Dinechin (IRC c3d)
On Fri, 29 May 2020 at 17:23, Christophe de Dinechin <dinechin@redhat.com> wrote: > On 2020-05-26 at 20:51 CEST, Eric Blake wrote... > > diff --git a/hw/openrisc/openrisc_sim.c b/hw/openrisc/openrisc_sim.c > > index d08ce6181199..95011a8015b4 100644 > > --- a/hw/openrisc/openrisc_sim.c > > +++ b/hw/openrisc/openrisc_sim.c > > @@ -129,7 +129,7 @@ static void openrisc_sim_init(MachineState *machine) > > const char *kernel_filename = machine->kernel_filename; > > OpenRISCCPU *cpu = NULL; > > MemoryRegion *ram; > > - qemu_irq *cpu_irqs[2]; > > + qemu_irq *cpu_irqs[2] = {}; > > Why is the value [2] correct here? The loop that initializes loops over > machine->smp.cpus. Is it always less than 2 on this machine? Yes: openrisc_sim_machine_init() sets mc->max_cpus = 2. My suggestion of adding an assert() is essentially telling the compiler that indeed smp_cpus must always be in the range [1,2], which we can tell but it can't. thanks -- PMM
Peter Maydell <peter.maydell@linaro.org> writes: > On Fri, 29 May 2020 at 17:23, Christophe de Dinechin > <dinechin@redhat.com> wrote: >> On 2020-05-26 at 20:51 CEST, Eric Blake wrote... >> > diff --git a/hw/openrisc/openrisc_sim.c b/hw/openrisc/openrisc_sim.c >> > index d08ce6181199..95011a8015b4 100644 >> > --- a/hw/openrisc/openrisc_sim.c >> > +++ b/hw/openrisc/openrisc_sim.c >> > @@ -129,7 +129,7 @@ static void openrisc_sim_init(MachineState *machine) >> > const char *kernel_filename = machine->kernel_filename; >> > OpenRISCCPU *cpu = NULL; >> > MemoryRegion *ram; >> > - qemu_irq *cpu_irqs[2]; >> > + qemu_irq *cpu_irqs[2] = {}; >> >> Why is the value [2] correct here? The loop that initializes loops over >> machine->smp.cpus. Is it always less than 2 on this machine? > > Yes: openrisc_sim_machine_init() sets mc->max_cpus = 2. > My suggestion of adding an assert() is essentially telling the > compiler that indeed smp_cpus must always be in the range [1,2], > which we can tell but it can't. Do we have a proper patch for this on the list?
Markus Armbruster <armbru@redhat.com> writes: > Peter Maydell <peter.maydell@linaro.org> writes: > >> On Fri, 29 May 2020 at 17:23, Christophe de Dinechin >> <dinechin@redhat.com> wrote: >>> On 2020-05-26 at 20:51 CEST, Eric Blake wrote... >>> > diff --git a/hw/openrisc/openrisc_sim.c b/hw/openrisc/openrisc_sim.c >>> > index d08ce6181199..95011a8015b4 100644 >>> > --- a/hw/openrisc/openrisc_sim.c >>> > +++ b/hw/openrisc/openrisc_sim.c >>> > @@ -129,7 +129,7 @@ static void openrisc_sim_init(MachineState *machine) >>> > const char *kernel_filename = machine->kernel_filename; >>> > OpenRISCCPU *cpu = NULL; >>> > MemoryRegion *ram; >>> > - qemu_irq *cpu_irqs[2]; >>> > + qemu_irq *cpu_irqs[2] = {}; >>> >>> Why is the value [2] correct here? The loop that initializes loops over >>> machine->smp.cpus. Is it always less than 2 on this machine? >> >> Yes: openrisc_sim_machine_init() sets mc->max_cpus = 2. >> My suggestion of adding an assert() is essentially telling the >> compiler that indeed smp_cpus must always be in the range [1,2], >> which we can tell but it can't. > > Do we have a proper patch for this on the list? Apparently not. Philippe did try Peter's suggestion, found it works, but then posted it only to Launchpad. Philippe, please post to the list, so we can finally get this fixed.
On 6/8/20 8:03 AM, Markus Armbruster wrote: > Markus Armbruster <armbru@redhat.com> writes: > >> Peter Maydell <peter.maydell@linaro.org> writes: >> >>> On Fri, 29 May 2020 at 17:23, Christophe de Dinechin >>> <dinechin@redhat.com> wrote: >>>> On 2020-05-26 at 20:51 CEST, Eric Blake wrote... >>>>> diff --git a/hw/openrisc/openrisc_sim.c b/hw/openrisc/openrisc_sim.c >>>>> index d08ce6181199..95011a8015b4 100644 >>>>> --- a/hw/openrisc/openrisc_sim.c >>>>> +++ b/hw/openrisc/openrisc_sim.c >>>>> @@ -129,7 +129,7 @@ static void openrisc_sim_init(MachineState *machine) >>>>> const char *kernel_filename = machine->kernel_filename; >>>>> OpenRISCCPU *cpu = NULL; >>>>> MemoryRegion *ram; >>>>> - qemu_irq *cpu_irqs[2]; >>>>> + qemu_irq *cpu_irqs[2] = {}; >>>> >>>> Why is the value [2] correct here? The loop that initializes loops over >>>> machine->smp.cpus. Is it always less than 2 on this machine? >>> >>> Yes: openrisc_sim_machine_init() sets mc->max_cpus = 2. >>> My suggestion of adding an assert() is essentially telling the >>> compiler that indeed smp_cpus must always be in the range [1,2], >>> which we can tell but it can't. >> >> Do we have a proper patch for this on the list? > > Apparently not. > > Philippe did try Peter's suggestion, found it works, but then posted it > only to Launchpad. Philippe, please post to the list, so we can finally > get this fixed. Sorry since it was Eric finding, I didn't understood I had to post it. Will do.
Philippe Mathieu-Daudé <philmd@redhat.com> writes: > On 6/8/20 8:03 AM, Markus Armbruster wrote: >> Markus Armbruster <armbru@redhat.com> writes: >> >>> Peter Maydell <peter.maydell@linaro.org> writes: >>> >>>> On Fri, 29 May 2020 at 17:23, Christophe de Dinechin >>>> <dinechin@redhat.com> wrote: >>>>> On 2020-05-26 at 20:51 CEST, Eric Blake wrote... >>>>>> diff --git a/hw/openrisc/openrisc_sim.c b/hw/openrisc/openrisc_sim.c >>>>>> index d08ce6181199..95011a8015b4 100644 >>>>>> --- a/hw/openrisc/openrisc_sim.c >>>>>> +++ b/hw/openrisc/openrisc_sim.c >>>>>> @@ -129,7 +129,7 @@ static void openrisc_sim_init(MachineState *machine) >>>>>> const char *kernel_filename = machine->kernel_filename; >>>>>> OpenRISCCPU *cpu = NULL; >>>>>> MemoryRegion *ram; >>>>>> - qemu_irq *cpu_irqs[2]; >>>>>> + qemu_irq *cpu_irqs[2] = {}; >>>>> >>>>> Why is the value [2] correct here? The loop that initializes loops over >>>>> machine->smp.cpus. Is it always less than 2 on this machine? >>>> >>>> Yes: openrisc_sim_machine_init() sets mc->max_cpus = 2. >>>> My suggestion of adding an assert() is essentially telling the >>>> compiler that indeed smp_cpus must always be in the range [1,2], >>>> which we can tell but it can't. >>> >>> Do we have a proper patch for this on the list? >> >> Apparently not. >> >> Philippe did try Peter's suggestion, found it works, but then posted it >> only to Launchpad. Philippe, please post to the list, so we can finally >> get this fixed. > > Sorry since it was Eric finding, I didn't understood I had to post it. > Will do. You didn't *have* to, but it'll help if you do :)
On 6/8/20 4:15 AM, Markus Armbruster wrote: >>>>> Yes: openrisc_sim_machine_init() sets mc->max_cpus = 2. >>>>> My suggestion of adding an assert() is essentially telling the >>>>> compiler that indeed smp_cpus must always be in the range [1,2], >>>>> which we can tell but it can't. >>>> >>>> Do we have a proper patch for this on the list? >>> >>> Apparently not. >>> >>> Philippe did try Peter's suggestion, found it works, but then posted it >>> only to Launchpad. Philippe, please post to the list, so we can finally >>> get this fixed. >> >> Sorry since it was Eric finding, I didn't understood I had to post it. >> Will do. > > You didn't *have* to, but it'll help if you do :) Now at https://lists.gnu.org/archive/html/qemu-devel/2020-06/msg01779.html
diff --git a/hw/openrisc/openrisc_sim.c b/hw/openrisc/openrisc_sim.c index d08ce6181199..95011a8015b4 100644 --- a/hw/openrisc/openrisc_sim.c +++ b/hw/openrisc/openrisc_sim.c @@ -129,7 +129,7 @@ static void openrisc_sim_init(MachineState *machine) const char *kernel_filename = machine->kernel_filename; OpenRISCCPU *cpu = NULL; MemoryRegion *ram; - qemu_irq *cpu_irqs[2]; + qemu_irq *cpu_irqs[2] = {}; qemu_irq serial_irq; int n; unsigned int smp_cpus = machine->smp.cpus;
On my Fedora 32 machine, gcc 10.1.1 at -O2 (the default for a bare './configure') has a false-positive complaint: CC or1k-softmmu/hw/openrisc/openrisc_sim.o /home/eblake/qemu/hw/openrisc/openrisc_sim.c: In function ‘openrisc_sim_init’: /home/eblake/qemu/hw/openrisc/openrisc_sim.c:87:42: error: ‘cpu_irqs[0]’ may be used uninitialized in this function [-Werror=maybe-uninitialized] 87 | sysbus_connect_irq(s, i, cpu_irqs[i][irq_pin]); | ~~~~~~~~^~~ Initializing both pointers of cpu_irqs[] to NULL is sufficient to shut up the compiler, even though they are definitely assigned in openrisc_sim_init() prior to the inlined call to openrisc_sim_ompic_init() containing the line in question. Signed-off-by: Eric Blake <eblake@redhat.com> --- hw/openrisc/openrisc_sim.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)