1. Patchew
  2. QEMU
  3. [PATCH v2 00/22] (few more) Steps towards enabling -Wshadow
  4. View patch

[RFC PATCH v2 22/22] softmmu/physmem: Clean up local variable shadowing

Philippe Mathieu-Daudé posted 22 patches 2 years, 5 months ago
Maintainers: Richard Henderson <richard.henderson@linaro.org>, Paolo Bonzini <pbonzini@redhat.com>, "Daniel P. Berrangé" <berrange@redhat.com>, Beniamino Galvani <b.galvani@gmail.com>, Peter Maydell <peter.maydell@linaro.org>, Strahinja Jankovic <strahinja.p.jankovic@gmail.com>, "Cédric Le Goater" <clg@kaod.org>, Andrew Jeffery <andrew@aj.id.au>, Joel Stanley <joel@jms.id.au>, John Snow <jsnow@redhat.com>, Laurent Vivier <laurent@vivier.eu>, "Edgar E. Iglesias" <edgar.iglesias@gmail.com>, Chris Wulff <crwulff@gmail.com>, Marek Vasut <marex@denx.de>, Alistair Francis <alistair.francis@wdc.com>, David Gibson <david@gibson.dropbear.id.au>, Dmitry Fleytman <dmitry.fleytman@gmail.com>, Akihiko Odaki <akihiko.odaki@daynix.com>, Jason Wang <jasowang@redhat.com>, "Alex Bennée" <alex.bennee@linaro.org>, Peter Xu <peterx@redhat.com>, David Hildenbrand <david@redhat.com>, "Philippe Mathieu-Daudé" <philmd@linaro.org>, Alexander Graf <agraf@csgraf.de>, Aurelien Jarno <aurelien@aurel32.net>, Jiaxun Yang <jiaxun.yang@flygoat.com>, Aleksandar Rikalo <aleksandar.rikalo@syrmia.com>, Bastian Koppelmann <kbastian@mail.uni-paderborn.de>, Coiby Xu <Coiby.Xu@gmail.com>
There is a newer version of this series
[RFC PATCH v2 22/22] softmmu/physmem: Clean up local variable shadowing
Posted by Philippe Mathieu-Daudé 2 years, 5 months ago 
Fix:

  softmmu/physmem.c: In function ‘cpu_physical_memory_snapshot_and_clear_dirty’:
  softmmu/physmem.c:916:27: warning: declaration of ‘offset’ shadows a parameter [-Wshadow=compatible-local]
    916 |             unsigned long offset = page % DIRTY_MEMORY_BLOCK_SIZE;
        |                           ^~~~~~
  softmmu/physmem.c:892:31: note: shadowed declaration is here
    892 |     (MemoryRegion *mr, hwaddr offset, hwaddr length, unsigned client)
        |                        ~~~~~~~^~~~~~

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
RFC: Please double-check how 'offset' is used few lines later.
---
 softmmu/physmem.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/softmmu/physmem.c b/softmmu/physmem.c
index 18277ddd67..db5b628a60 100644
--- a/softmmu/physmem.c
+++ b/softmmu/physmem.c
@@ -913,16 +913,16 @@ DirtyBitmapSnapshot *cpu_physical_memory_snapshot_and_clear_dirty
 
         while (page < end) {
             unsigned long idx = page / DIRTY_MEMORY_BLOCK_SIZE;
-            unsigned long offset = page % DIRTY_MEMORY_BLOCK_SIZE;
+            unsigned long ofs = page % DIRTY_MEMORY_BLOCK_SIZE;
             unsigned long num = MIN(end - page,
-                                    DIRTY_MEMORY_BLOCK_SIZE - offset);
+                                    DIRTY_MEMORY_BLOCK_SIZE - ofs);
 
-            assert(QEMU_IS_ALIGNED(offset, (1 << BITS_PER_LEVEL)));
+            assert(QEMU_IS_ALIGNED(ofs, (1 << BITS_PER_LEVEL)));
             assert(QEMU_IS_ALIGNED(num,    (1 << BITS_PER_LEVEL)));
-            offset >>= BITS_PER_LEVEL;
+            ofs >>= BITS_PER_LEVEL;
 
             bitmap_copy_and_clear_atomic(snap->dirty + dest,
-                                         blocks->blocks[idx] + offset,
+                                         blocks->blocks[idx] + ofs,
                                          num);
             page += num;
             dest += num >> BITS_PER_LEVEL;
-- 
2.41.0
Re: [RFC PATCH v2 22/22] softmmu/physmem: Clean up local variable shadowing
Posted by Daniel P. Berrangé 2 years, 5 months ago 
On Mon, Sep 04, 2023 at 06:12:34PM +0200, Philippe Mathieu-Daudé wrote:
> Fix:
> 
>   softmmu/physmem.c: In function ‘cpu_physical_memory_snapshot_and_clear_dirty’:
>   softmmu/physmem.c:916:27: warning: declaration of ‘offset’ shadows a parameter [-Wshadow=compatible-local]
>     916 |             unsigned long offset = page % DIRTY_MEMORY_BLOCK_SIZE;
>         |                           ^~~~~~
>   softmmu/physmem.c:892:31: note: shadowed declaration is here
>     892 |     (MemoryRegion *mr, hwaddr offset, hwaddr length, unsigned client)
>         |                        ~~~~~~~^~~~~~
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
> RFC: Please double-check how 'offset' is used few lines later.

I don't see an issue - those lines are in an outer scope, so won't
be accessing the 'offset' you've changed, they'll be the parameter
instead. If you want to sanity check though, presumably the asm
dissassembly for this method should be the same before/after this
change

> ---
>  softmmu/physmem.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>

> 
> diff --git a/softmmu/physmem.c b/softmmu/physmem.c
> index 18277ddd67..db5b628a60 100644
> --- a/softmmu/physmem.c
> +++ b/softmmu/physmem.c
> @@ -913,16 +913,16 @@ DirtyBitmapSnapshot *cpu_physical_memory_snapshot_and_clear_dirty
>  
>          while (page < end) {
>              unsigned long idx = page / DIRTY_MEMORY_BLOCK_SIZE;
> -            unsigned long offset = page % DIRTY_MEMORY_BLOCK_SIZE;
> +            unsigned long ofs = page % DIRTY_MEMORY_BLOCK_SIZE;
>              unsigned long num = MIN(end - page,
> -                                    DIRTY_MEMORY_BLOCK_SIZE - offset);
> +                                    DIRTY_MEMORY_BLOCK_SIZE - ofs);
>  
> -            assert(QEMU_IS_ALIGNED(offset, (1 << BITS_PER_LEVEL)));
> +            assert(QEMU_IS_ALIGNED(ofs, (1 << BITS_PER_LEVEL)));
>              assert(QEMU_IS_ALIGNED(num,    (1 << BITS_PER_LEVEL)));
> -            offset >>= BITS_PER_LEVEL;
> +            ofs >>= BITS_PER_LEVEL;
>  
>              bitmap_copy_and_clear_atomic(snap->dirty + dest,
> -                                         blocks->blocks[idx] + offset,
> +                                         blocks->blocks[idx] + ofs,
>                                           num);
>              page += num;
>              dest += num >> BITS_PER_LEVEL;
> -- 
> 2.41.0
> 
> 

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|
Re: [RFC PATCH v2 22/22] softmmu/physmem: Clean up local variable shadowing
Posted by Peter Xu 2 years, 5 months ago 
On Mon, Sep 04, 2023 at 05:31:30PM +0100, Daniel P. Berrangé wrote:
> On Mon, Sep 04, 2023 at 06:12:34PM +0200, Philippe Mathieu-Daudé wrote:
> > Fix:
> > 
> >   softmmu/physmem.c: In function ‘cpu_physical_memory_snapshot_and_clear_dirty’:
> >   softmmu/physmem.c:916:27: warning: declaration of ‘offset’ shadows a parameter [-Wshadow=compatible-local]
> >     916 |             unsigned long offset = page % DIRTY_MEMORY_BLOCK_SIZE;
> >         |                           ^~~~~~
> >   softmmu/physmem.c:892:31: note: shadowed declaration is here
> >     892 |     (MemoryRegion *mr, hwaddr offset, hwaddr length, unsigned client)
> >         |                        ~~~~~~~^~~~~~
> > 
> > Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> > ---
> > RFC: Please double-check how 'offset' is used few lines later.
> 
> I don't see an issue - those lines are in an outer scope, so won't
> be accessing the 'offset' you've changed, they'll be the parameter
> instead. If you want to sanity check though, presumably the asm
> dissassembly for this method should be the same before/after this
> change

(and if it didn't do so then it's a bug..)

> 
> > ---
> >  softmmu/physmem.c | 10 +++++-----
> >  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>

Reviewed-by: Peter Xu <peterx@redhat.com>

-- 
Peter Xu

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.