nbd/server: Silence gcc false positive

[Qemu-devel] [PATCH] nbd/server: Silence gcc false positive

Posted by Eric Blake 5 years, 10 months ago

The code has a while() loop that always initialized 'end', and
the loop always executes at least once (as evidenced by the assert()
just prior to the loop).  But some versions of gcc still complain
that 'end' is used uninitialized, so silence them.

Signed-off-by: Eric Blake <eblake@redhat.com>
---

Peter, do you want to apply this directly as a build fix, or shall
I submit a pull request?

 nbd/server.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/nbd/server.c b/nbd/server.c
index 274604609f4..50ac8bfafc6 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -1937,7 +1937,7 @@ static unsigned int bitmap_to_extents(BdrvDirtyBitmap *bitmap, uint64_t offset,
                                       unsigned int nb_extents,
                                       bool dont_fragment)
 {
-    uint64_t begin = offset, end;
+    uint64_t begin = offset, end = offset;
     uint64_t overall_end = offset + *length;
     unsigned int i = 0;
     BdrvDirtyBitmapIter *it;
@@ -1977,6 +1977,7 @@ static unsigned int bitmap_to_extents(BdrvDirtyBitmap *bitmap, uint64_t offset,

     bdrv_dirty_bitmap_unlock(bitmap);

+    assert(offset > end);
     *length = end - offset;
     return i;
 }
-- 
2.14.4

Re: [Qemu-devel] [PATCH] nbd/server: Silence gcc false positive

Posted by Peter Maydell 5 years, 10 months ago

On 22 June 2018 at 13:58, Eric Blake <eblake@redhat.com> wrote:
> The code has a while() loop that always initialized 'end', and
> the loop always executes at least once (as evidenced by the assert()
> just prior to the loop).  But some versions of gcc still complain
> that 'end' is used uninitialized, so silence them.
>
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
>
> Peter, do you want to apply this directly as a build fix, or shall
> I submit a pull request?

I'll just apply it directly.

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

Re: [Qemu-devel] [PATCH] nbd/server: Silence gcc false positive

Posted by Peter Maydell 5 years, 10 months ago

On 22 June 2018 at 13:59, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 22 June 2018 at 13:58, Eric Blake <eblake@redhat.com> wrote:
>> The code has a while() loop that always initialized 'end', and
>> the loop always executes at least once (as evidenced by the assert()
>> just prior to the loop).  But some versions of gcc still complain
>> that 'end' is used uninitialized, so silence them.
>>
>> Signed-off-by: Eric Blake <eblake@redhat.com>
>> ---
>>
>> Peter, do you want to apply this directly as a build fix, or shall
>> I submit a pull request?
>
> I'll just apply it directly.
>
> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

Now applied to master, thanks.

-- PMM

Re: [Qemu-devel] [PATCH] nbd/server: Silence gcc false positive

Posted by Eric Blake 5 years, 10 months ago

On 06/22/2018 07:58 AM, Eric Blake wrote:
> The code has a while() loop that always initialized 'end', and
> the loop always executes at least once (as evidenced by the assert()
> just prior to the loop).  But some versions of gcc still complain
> that 'end' is used uninitialized, so silence them.
> 
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
> 
> Peter, do you want to apply this directly as a build fix, or shall
> I submit a pull request?
> 

Phooey. I'm obviously under a lot of stress. In my rush to fix the build 
problem, I introduced a logic error that breaks NBD dirty bitmaps. :(

>   nbd/server.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/nbd/server.c b/nbd/server.c
> index 274604609f4..50ac8bfafc6 100644
> --- a/nbd/server.c
> +++ b/nbd/server.c
> @@ -1937,7 +1937,7 @@ static unsigned int bitmap_to_extents(BdrvDirtyBitmap *bitmap, uint64_t offset,
>                                         unsigned int nb_extents,
>                                         bool dont_fragment)
>   {
> -    uint64_t begin = offset, end;
> +    uint64_t begin = offset, end = offset;
>       uint64_t overall_end = offset + *length;
>       unsigned int i = 0;
>       BdrvDirtyBitmapIter *it;
> @@ -1977,6 +1977,7 @@ static unsigned int bitmap_to_extents(BdrvDirtyBitmap *bitmap, uint64_t offset,
> 
>       bdrv_dirty_bitmap_unlock(bitmap);
> 
> +    assert(offset > end);
>       *length = end - offset;

The assertion is backwards; that should be 'offset < end'. :(

Sending another fix. And putting on the brown paper bag of shame.

And bummer that neither 'make check' nor iotests exposes the breakage. 
My RFC hack for adding x-block-status to the client would be useful for 
that purpose.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [Qemu-devel] [PATCH] nbd/server: Silence gcc false positive

Posted by no-reply@patchew.org 5 years, 10 months ago

Hi,

This series seems to have some coding style problems. See output below for
more information:

Type: series
Message-id: 20180622125814.345274-1-eblake@redhat.com
Subject: [Qemu-devel] [PATCH] nbd/server: Silence gcc false positive

=== TEST SCRIPT BEGIN ===
#!/bin/bash

BASE=base
n=1
total=$(git log --oneline $BASE.. | wc -l)
failed=0

git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram

commits="$(git log --format=%H --reverse $BASE..)"
for c in $commits; do
    echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..."
    if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
        failed=1
        echo
    fi
    n=$((n+1))
done

exit $failed
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
 t [tag update]            patchew/20180620120620.12806-1-yongbok.kim@mips.com -> patchew/20180620120620.12806-1-yongbok.kim@mips.com
 * [new tag]               patchew/20180622125814.345274-1-eblake@redhat.com -> patchew/20180622125814.345274-1-eblake@redhat.com
Switched to a new branch 'test'
6489e4b10c Merge remote-tracking branch 'remotes/pmaydell/tags/pull-target-arm-20180622' into staging
10e84d2504 xen: Don't use memory_region_init_ram_nomigrate() in pci_assign_dev_load_option_rom()
aea21afaef vl.c: Don't zero-initialize statics for serial_hds
fea64baeab target/arm: Strict alignment for ARMv6-M and ARMv8-M Baseline
12c583c45b target/arm: Introduce ARM_FEATURE_M_MAIN
42fc936a30 hw/arm/mps2-tz.c: Instantiate MPCs
b03e0d28a2 hw/arm/iotkit: Wire up MPC interrupt lines
985db83c4a hw/arm/iotkit: Instantiate MPC
769d3448af hw/misc/iotkit-secctl.c: Implement SECMPCINTSTATUS
cdf0c49a6a hw/misc/tz_mpc.c: Honour the BLK_LUT settings in translate
7790999ca7 hw/misc/tz-mpc.c: Implement correct blocked-access behaviour
fa6dd4bba5 hw/misc/tz-mpc.c: Implement registers
d3cb43aea7 hw/misc/tz-mpc.c: Implement the Arm TrustZone Memory Protection Controller
80c3703588 xlnx-zynqmp: Swap Cortex-R5 for Cortex-R5F
60e383fc83 target-arm: Add the Cortex-R5F
6603ff9f6b hw/arm/virt: Increase max_cpus to 512
ad37e0623f hw/arm/virt: Use 256MB ECAM region by default
08e38e7a7e hw/arm/virt: Add virt-3.0 machine type
d844e5a9a5 hw/arm/virt: Add a new 256MB ECAM region
82ca568747 hw/arm/virt: Register two redistributor regions when necessary
f3a20d2d88 hw/arm/virt-acpi-build: Advertise one or two GICR structures
0bd011094d hw/arm/virt: GICv3 DT node with one or two redistributor regions
b095e84e4f hw/intc/arm_gicv3_kvm: Get prepared to handle multiple redist regions
77737a0dc0 hw/intc/arm_gicv3: Introduce redist-region-count array property
356069f49b target/arm: Allow KVM device address overwriting
0d9ee1e956 linux-headers: Update to kernel mainline commit b357bf602
e9eb7de541 target-arm: fix a segmentation fault due to illegal memory access
a2bd1a2dce target/arm: Minor cleanup for ARMv6-M 32-bit instructions
787260735f hw/intc/arm_gicv3: fix an extra left-shift when reading IPRIORITYR

=== OUTPUT BEGIN ===
Checking PATCH 1/29: hw/intc/arm_gicv3: fix an extra left-shift when reading IPRIORITYR...
Checking PATCH 2/29: target/arm: Minor cleanup for ARMv6-M 32-bit instructions...
Checking PATCH 3/29: target-arm: fix a segmentation fault due to illegal memory access...
Checking PATCH 4/29: linux-headers: Update to kernel mainline commit b357bf602...
Checking PATCH 5/29: target/arm: Allow KVM device address overwriting...
Checking PATCH 6/29: hw/intc/arm_gicv3: Introduce redist-region-count array property...
Checking PATCH 7/29: hw/intc/arm_gicv3_kvm: Get prepared to handle multiple redist regions...
Checking PATCH 8/29: hw/arm/virt: GICv3 DT node with one or two redistributor regions...
Checking PATCH 9/29: hw/arm/virt-acpi-build: Advertise one or two GICR structures...
Checking PATCH 10/29: hw/arm/virt: Register two redistributor regions when necessary...
Checking PATCH 11/29: hw/arm/virt: Add a new 256MB ECAM region...
Checking PATCH 12/29: hw/arm/virt: Add virt-3.0 machine type...
Checking PATCH 13/29: hw/arm/virt: Use 256MB ECAM region by default...
Checking PATCH 14/29: hw/arm/virt: Increase max_cpus to 512...
Checking PATCH 15/29: target-arm: Add the Cortex-R5F...
Checking PATCH 16/29: xlnx-zynqmp: Swap Cortex-R5 for Cortex-R5F...
Checking PATCH 17/29: hw/misc/tz-mpc.c: Implement the Arm TrustZone Memory Protection Controller...
WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
#86: 
new file mode 100644

total: 0 errors, 1 warnings, 504 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
Checking PATCH 18/29: hw/misc/tz-mpc.c: Implement registers...
Checking PATCH 19/29: hw/misc/tz-mpc.c: Implement correct blocked-access behaviour...
Checking PATCH 20/29: hw/misc/tz_mpc.c: Honour the BLK_LUT settings in translate...
Checking PATCH 21/29: hw/misc/iotkit-secctl.c: Implement SECMPCINTSTATUS...
ERROR: spaces required around that '*' (ctx:VxV)
#92: FILE: hw/misc/iotkit-secctl.c:711:
+    .subsections = (const VMStateDescription*[]) {
                                             ^

total: 1 errors, 0 warnings, 100 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

Checking PATCH 22/29: hw/arm/iotkit: Instantiate MPC...
Checking PATCH 23/29: hw/arm/iotkit: Wire up MPC interrupt lines...
Checking PATCH 24/29: hw/arm/mps2-tz.c: Instantiate MPCs...
Checking PATCH 25/29: target/arm: Introduce ARM_FEATURE_M_MAIN...
Checking PATCH 26/29: target/arm: Strict alignment for ARMv6-M and ARMv8-M Baseline...
Checking PATCH 27/29: vl.c: Don't zero-initialize statics for serial_hds...
Checking PATCH 28/29: xen: Don't use memory_region_init_ram_nomigrate() in pci_assign_dev_load_option_rom()...
Checking PATCH 29/29: Merge remote-tracking branch 'remotes/pmaydell/tags/pull-target-arm-20180622' into staging...
=== OUTPUT END ===

Test command exited with code: 1


---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-devel@redhat.com

Re: [Qemu-devel] [PATCH] nbd/server: Silence gcc false positive

Posted by Peter Maydell 5 years, 10 months ago

Hi Fam -- looks like patchew has got confused somehow...

On 22 June 2018 at 17:31,  <no-reply@patchew.org> wrote:
> Hi,
>
> This series seems to have some coding style problems. See output below for
> more information:
>
> Type: series
> Message-id: 20180622125814.345274-1-eblake@redhat.com
> Subject: [Qemu-devel] [PATCH] nbd/server: Silence gcc false positive

This is the report email for this nbd patch (and it's following
up to the nbd patch email)...

> Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
> From https://github.com/patchew-project/qemu
>  t [tag update]            patchew/20180620120620.12806-1-yongbok.kim@mips.com -> patchew/20180620120620.12806-1-yongbok.kim@mips.com
>  * [new tag]               patchew/20180622125814.345274-1-eblake@redhat.com -> patchew/20180622125814.345274-1-eblake@redhat.com
> Switched to a new branch 'test'
> 6489e4b10c Merge remote-tracking branch 'remotes/pmaydell/tags/pull-target-arm-20180622' into staging
> 10e84d2504 xen: Don't use memory_region_init_ram_nomigrate() in pci_assign_dev_load_option_rom()
> aea21afaef vl.c: Don't zero-initialize statics for serial_hds
> fea64baeab target/arm: Strict alignment for ARMv6-M and ARMv8-M Baseline

...but the patches it seems to actually be testing are from my
latest target-arm pull request ?

> 12c583c45b target/arm: Introduce ARM_FEATURE_M_MAIN
> 42fc936a30 hw/arm/mps2-tz.c: Instantiate MPCs
> b03e0d28a2 hw/arm/iotkit: Wire up MPC interrupt lines
> 985db83c4a hw/arm/iotkit: Instantiate MPC
> 769d3448af hw/misc/iotkit-secctl.c: Implement SECMPCINTSTATUS
> cdf0c49a6a hw/misc/tz_mpc.c: Honour the BLK_LUT settings in translate
> 7790999ca7 hw/misc/tz-mpc.c: Implement correct blocked-access behaviour
> fa6dd4bba5 hw/misc/tz-mpc.c: Implement registers
> d3cb43aea7 hw/misc/tz-mpc.c: Implement the Arm TrustZone Memory Protection Controller
> 80c3703588 xlnx-zynqmp: Swap Cortex-R5 for Cortex-R5F
> 60e383fc83 target-arm: Add the Cortex-R5F
> 6603ff9f6b hw/arm/virt: Increase max_cpus to 512
> ad37e0623f hw/arm/virt: Use 256MB ECAM region by default
> 08e38e7a7e hw/arm/virt: Add virt-3.0 machine type
> d844e5a9a5 hw/arm/virt: Add a new 256MB ECAM region
> 82ca568747 hw/arm/virt: Register two redistributor regions when necessary
> f3a20d2d88 hw/arm/virt-acpi-build: Advertise one or two GICR structures
> 0bd011094d hw/arm/virt: GICv3 DT node with one or two redistributor regions
> b095e84e4f hw/intc/arm_gicv3_kvm: Get prepared to handle multiple redist regions
> 77737a0dc0 hw/intc/arm_gicv3: Introduce redist-region-count array property
> 356069f49b target/arm: Allow KVM device address overwriting
> 0d9ee1e956 linux-headers: Update to kernel mainline commit b357bf602
> e9eb7de541 target-arm: fix a segmentation fault due to illegal memory access
> a2bd1a2dce target/arm: Minor cleanup for ARMv6-M 32-bit instructions
> 787260735f hw/intc/arm_gicv3: fix an extra left-shift when reading IPRIORITYR
>
> === OUTPUT BEGIN ===
> Checking PATCH 1/29: hw/intc/arm_gicv3: fix an extra left-shift when reading IPRIORITYR...
> Checking PATCH 2/29: target/arm: Minor cleanup for ARMv6-M 32-bit instructions...
> Checking PATCH 3/29: target-arm: fix a segmentation fault due to illegal memory access...
> Checking PATCH 4/29: linux-headers: Update to kernel mainline commit b357bf602...
> Checking PATCH 5/29: target/arm: Allow KVM device address overwriting...
> Checking PATCH 6/29: hw/intc/arm_gicv3: Introduce redist-region-count array property...
> Checking PATCH 7/29: hw/intc/arm_gicv3_kvm: Get prepared to handle multiple redist regions...
> Checking PATCH 8/29: hw/arm/virt: GICv3 DT node with one or two redistributor regions...
> Checking PATCH 9/29: hw/arm/virt-acpi-build: Advertise one or two GICR structures...
> Checking PATCH 10/29: hw/arm/virt: Register two redistributor regions when necessary...
> Checking PATCH 11/29: hw/arm/virt: Add a new 256MB ECAM region...
> Checking PATCH 12/29: hw/arm/virt: Add virt-3.0 machine type...
> Checking PATCH 13/29: hw/arm/virt: Use 256MB ECAM region by default...
> Checking PATCH 14/29: hw/arm/virt: Increase max_cpus to 512...
> Checking PATCH 15/29: target-arm: Add the Cortex-R5F...
> Checking PATCH 16/29: xlnx-zynqmp: Swap Cortex-R5 for Cortex-R5F...
> Checking PATCH 17/29: hw/misc/tz-mpc.c: Implement the Arm TrustZone Memory Protection Controller...
> WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
> #86:
> new file mode 100644
>
> total: 0 errors, 1 warnings, 504 lines checked
>
> Your patch has style problems, please review.  If any of these errors
> are false positives report them to the maintainer, see
> CHECKPATCH in MAINTAINERS.
> Checking PATCH 18/29: hw/misc/tz-mpc.c: Implement registers...
> Checking PATCH 19/29: hw/misc/tz-mpc.c: Implement correct blocked-access behaviour...
> Checking PATCH 20/29: hw/misc/tz_mpc.c: Honour the BLK_LUT settings in translate...
> Checking PATCH 21/29: hw/misc/iotkit-secctl.c: Implement SECMPCINTSTATUS...
> ERROR: spaces required around that '*' (ctx:VxV)
> #92: FILE: hw/misc/iotkit-secctl.c:711:
> +    .subsections = (const VMStateDescription*[]) {
>                                              ^
>
> total: 1 errors, 0 warnings, 100 lines checked
>
> Your patch has style problems, please review.  If any of these errors
> are false positives report them to the maintainer, see
> CHECKPATCH in MAINTAINERS.
>
> Checking PATCH 22/29: hw/arm/iotkit: Instantiate MPC...
> Checking PATCH 23/29: hw/arm/iotkit: Wire up MPC interrupt lines...
> Checking PATCH 24/29: hw/arm/mps2-tz.c: Instantiate MPCs...
> Checking PATCH 25/29: target/arm: Introduce ARM_FEATURE_M_MAIN...
> Checking PATCH 26/29: target/arm: Strict alignment for ARMv6-M and ARMv8-M Baseline...
> Checking PATCH 27/29: vl.c: Don't zero-initialize statics for serial_hds...
> Checking PATCH 28/29: xen: Don't use memory_region_init_ram_nomigrate() in pci_assign_dev_load_option_rom()...
> Checking PATCH 29/29: Merge remote-tracking branch 'remotes/pmaydell/tags/pull-target-arm-20180622' into staging...
> === OUTPUT END ===
>
> Test command exited with code: 1

thanks
-- PMM