qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
* [v3] Help wanted for enabling -Wshadow=local
@ 2023-10-06 14:45 Markus Armbruster
  2023-10-06 16:18 ` Thomas Huth
  2023-10-06 18:41 ` Markus Armbruster
  0 siblings, 2 replies; 7+ messages in thread
From: Markus Armbruster @ 2023-10-06 14:45 UTC (permalink / raw)
  To: qemu-devel; +Cc: Gerd Hoffmann, Marc-André Lureau

Local variables shadowing other local variables or parameters make the
code needlessly hard to understand.  Bugs love to hide in such code.
Evidence: "[PATCH v3 1/7] migration/rdma: Fix save_page method to fail
on polling error".

Enabling -Wshadow would prevent bugs like this one.  But we have to
clean up all the offenders first.

Quite a few people responded to my calls for help.  Thank you so much!

I'm collecting patches in my git repo at
https://repo.or.cz/qemu/armbru.git in branch shadow-next.  All but the
last two are in a pending pull request.

My test build is down to seven files with warnings.  "[PATCH v2 0/3]
hexagon: GETPC() and shadowing fixes" takes care of four, but it needs a
rebase.

Remaining three:

    In file included from ../hw/display/virtio-gpu-virgl.c:19:
    ../hw/display/virtio-gpu-virgl.c: In function ‘virgl_cmd_submit_3d’:
    /work/armbru/qemu/include/hw/virtio/virtio-gpu.h:228:16: warning: declaration of ‘s’ shadows a previous local [-Wshadow=compatible-local]
      228 |         size_t s;                                                       \
          |                ^
    ../hw/display/virtio-gpu-virgl.c:215:5: note: in expansion of macro ‘VIRTIO_GPU_FILL_CMD’
      215 |     VIRTIO_GPU_FILL_CMD(cs);
          |     ^~~~~~~~~~~~~~~~~~~
    ../hw/display/virtio-gpu-virgl.c:213:12: note: shadowed declaration is here
      213 |     size_t s;
          |            ^

    In file included from ../contrib/vhost-user-gpu/virgl.h:18,
                     from ../contrib/vhost-user-gpu/virgl.c:17:
    ../contrib/vhost-user-gpu/virgl.c: In function ‘virgl_cmd_submit_3d’:
    ../contrib/vhost-user-gpu/vugpu.h:167:16: warning: declaration of ‘s’ shadows a previous local [-Wshadow=compatible-local]
      167 |         size_t s;                                               \
          |                ^
    ../contrib/vhost-user-gpu/virgl.c:203:5: note: in expansion of macro ‘VUGPU_FILL_CMD’
      203 |     VUGPU_FILL_CMD(cs);
          |     ^~~~~~~~~~~~~~
    ../contrib/vhost-user-gpu/virgl.c:201:12: note: shadowed declaration is here
      201 |     size_t s;
          |            ^

    ../contrib/vhost-user-gpu/vhost-user-gpu.c: In function ‘vg_resource_flush’:
    ../contrib/vhost-user-gpu/vhost-user-gpu.c:837:29: warning: declaration of ‘i’ shadows a previous local [-Wshadow=local]
      837 |             pixman_image_t *i =
          |                             ^
    ../contrib/vhost-user-gpu/vhost-user-gpu.c:757:9: note: shadowed declaration is here
      757 |     int i;
          |         ^

Gerd, Marc-André, or anybody else?

More warnings may lurk in code my test build doesn't compile.  Need a
full CI build with -Wshadow=local to find them.  Anybody care to kick
one off?



^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [v3] Help wanted for enabling -Wshadow=local
  2023-10-06 14:45 [v3] Help wanted for enabling -Wshadow=local Markus Armbruster
@ 2023-10-06 16:18 ` Thomas Huth
  2023-10-06 17:55   ` Thomas Huth
  2023-10-06 18:41 ` Markus Armbruster
  1 sibling, 1 reply; 7+ messages in thread
From: Thomas Huth @ 2023-10-06 16:18 UTC (permalink / raw)
  To: Markus Armbruster, qemu-devel; +Cc: Gerd Hoffmann, Marc-André Lureau

On 06/10/2023 16.45, Markus Armbruster wrote:
> Local variables shadowing other local variables or parameters make the
> code needlessly hard to understand.  Bugs love to hide in such code.
> Evidence: "[PATCH v3 1/7] migration/rdma: Fix save_page method to fail
> on polling error".
> 
> Enabling -Wshadow would prevent bugs like this one.  But we have to
> clean up all the offenders first.
> 
> Quite a few people responded to my calls for help.  Thank you so much!
> 
> I'm collecting patches in my git repo at
> https://repo.or.cz/qemu/armbru.git in branch shadow-next.  All but the
> last two are in a pending pull request.
> 
> My test build is down to seven files with warnings.  "[PATCH v2 0/3]
> hexagon: GETPC() and shadowing fixes" takes care of four, but it needs a
> rebase.
> 
> Remaining three:
> 
>      In file included from ../hw/display/virtio-gpu-virgl.c:19:
>      ../hw/display/virtio-gpu-virgl.c: In function ‘virgl_cmd_submit_3d’:
>      /work/armbru/qemu/include/hw/virtio/virtio-gpu.h:228:16: warning: declaration of ‘s’ shadows a previous local [-Wshadow=compatible-local]
>        228 |         size_t s;                                                       \
>            |                ^
>      ../hw/display/virtio-gpu-virgl.c:215:5: note: in expansion of macro ‘VIRTIO_GPU_FILL_CMD’
>        215 |     VIRTIO_GPU_FILL_CMD(cs);
>            |     ^~~~~~~~~~~~~~~~~~~
>      ../hw/display/virtio-gpu-virgl.c:213:12: note: shadowed declaration is here
>        213 |     size_t s;
>            |            ^
> 
>      In file included from ../contrib/vhost-user-gpu/virgl.h:18,
>                       from ../contrib/vhost-user-gpu/virgl.c:17:
>      ../contrib/vhost-user-gpu/virgl.c: In function ‘virgl_cmd_submit_3d’:
>      ../contrib/vhost-user-gpu/vugpu.h:167:16: warning: declaration of ‘s’ shadows a previous local [-Wshadow=compatible-local]
>        167 |         size_t s;                                               \
>            |                ^
>      ../contrib/vhost-user-gpu/virgl.c:203:5: note: in expansion of macro ‘VUGPU_FILL_CMD’
>        203 |     VUGPU_FILL_CMD(cs);
>            |     ^~~~~~~~~~~~~~
>      ../contrib/vhost-user-gpu/virgl.c:201:12: note: shadowed declaration is here
>        201 |     size_t s;
>            |            ^
> 
>      ../contrib/vhost-user-gpu/vhost-user-gpu.c: In function ‘vg_resource_flush’:
>      ../contrib/vhost-user-gpu/vhost-user-gpu.c:837:29: warning: declaration of ‘i’ shadows a previous local [-Wshadow=local]
>        837 |             pixman_image_t *i =
>            |                             ^
>      ../contrib/vhost-user-gpu/vhost-user-gpu.c:757:9: note: shadowed declaration is here
>        757 |     int i;
>            |         ^
> 
> Gerd, Marc-André, or anybody else?
> 
> More warnings may lurk in code my test build doesn't compile.  Need a
> full CI build with -Wshadow=local to find them.  Anybody care to kick
> one off?

I ran a build here (with -Werror enabled, so that it's easier to see where 
it breaks):

  https://gitlab.com/thuth/qemu/-/pipelines/1028023489

... but I didn't see any additional spots in the logs beside the ones that 
you already listed.

  Thomas




^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [v3] Help wanted for enabling -Wshadow=local
  2023-10-06 16:18 ` Thomas Huth
@ 2023-10-06 17:55   ` Thomas Huth
  2023-10-06 19:08     ` Warner Losh
  0 siblings, 1 reply; 7+ messages in thread
From: Thomas Huth @ 2023-10-06 17:55 UTC (permalink / raw)
  To: Markus Armbruster, qemu-devel
  Cc: Gerd Hoffmann, Marc-André Lureau, Warner Losh

On 06/10/2023 18.18, Thomas Huth wrote:
> On 06/10/2023 16.45, Markus Armbruster wrote:
>> Local variables shadowing other local variables or parameters make the
>> code needlessly hard to understand.  Bugs love to hide in such code.
>> Evidence: "[PATCH v3 1/7] migration/rdma: Fix save_page method to fail
>> on polling error".
>>
>> Enabling -Wshadow would prevent bugs like this one.  But we have to
>> clean up all the offenders first.
>>
>> Quite a few people responded to my calls for help.  Thank you so much!
>>
>> I'm collecting patches in my git repo at
>> https://repo.or.cz/qemu/armbru.git in branch shadow-next.  All but the
>> last two are in a pending pull request.
>>
>> My test build is down to seven files with warnings.  "[PATCH v2 0/3]
>> hexagon: GETPC() and shadowing fixes" takes care of four, but it needs a
>> rebase.
>>
>> Remaining three:
>>
>>      In file included from ../hw/display/virtio-gpu-virgl.c:19:
>>      ../hw/display/virtio-gpu-virgl.c: In function ‘virgl_cmd_submit_3d’:
>>      /work/armbru/qemu/include/hw/virtio/virtio-gpu.h:228:16: warning: 
>> declaration of ‘s’ shadows a previous local [-Wshadow=compatible-local]
>>        228 |         size_t 
>> s;                                                       \
>>            |                ^
>>      ../hw/display/virtio-gpu-virgl.c:215:5: note: in expansion of macro 
>> ‘VIRTIO_GPU_FILL_CMD’
>>        215 |     VIRTIO_GPU_FILL_CMD(cs);
>>            |     ^~~~~~~~~~~~~~~~~~~
>>      ../hw/display/virtio-gpu-virgl.c:213:12: note: shadowed declaration 
>> is here
>>        213 |     size_t s;
>>            |            ^
>>
>>      In file included from ../contrib/vhost-user-gpu/virgl.h:18,
>>                       from ../contrib/vhost-user-gpu/virgl.c:17:
>>      ../contrib/vhost-user-gpu/virgl.c: In function ‘virgl_cmd_submit_3d’:
>>      ../contrib/vhost-user-gpu/vugpu.h:167:16: warning: declaration of ‘s’ 
>> shadows a previous local [-Wshadow=compatible-local]
>>        167 |         size_t 
>> s;                                               \
>>            |                ^
>>      ../contrib/vhost-user-gpu/virgl.c:203:5: note: in expansion of macro 
>> ‘VUGPU_FILL_CMD’
>>        203 |     VUGPU_FILL_CMD(cs);
>>            |     ^~~~~~~~~~~~~~
>>      ../contrib/vhost-user-gpu/virgl.c:201:12: note: shadowed declaration 
>> is here
>>        201 |     size_t s;
>>            |            ^
>>
>>      ../contrib/vhost-user-gpu/vhost-user-gpu.c: In function 
>> ‘vg_resource_flush’:
>>      ../contrib/vhost-user-gpu/vhost-user-gpu.c:837:29: warning: 
>> declaration of ‘i’ shadows a previous local [-Wshadow=local]
>>        837 |             pixman_image_t *i =
>>            |                             ^
>>      ../contrib/vhost-user-gpu/vhost-user-gpu.c:757:9: note: shadowed 
>> declaration is here
>>        757 |     int i;
>>            |         ^
>>
>> Gerd, Marc-André, or anybody else?
>>
>> More warnings may lurk in code my test build doesn't compile.  Need a
>> full CI build with -Wshadow=local to find them.  Anybody care to kick
>> one off?
> 
> I ran a build here (with -Werror enabled, so that it's easier to see where 
> it breaks):
> 
>   https://gitlab.com/thuth/qemu/-/pipelines/1028023489
> 
> ... but I didn't see any additional spots in the logs beside the ones that 
> you already listed.

After adding two more patches to fix the above warnings, things look pretty 
good:

  https://gitlab.com/thuth/qemu/-/pipelines/1028413030

There are just some warnings left in the BSD code, as Warner already 
mentioned in his reply to v2 of your mail:

  https://gitlab.com/thuth/qemu/-/jobs/5241420713

  Thomas



^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [v3] Help wanted for enabling -Wshadow=local
  2023-10-06 14:45 [v3] Help wanted for enabling -Wshadow=local Markus Armbruster
  2023-10-06 16:18 ` Thomas Huth
@ 2023-10-06 18:41 ` Markus Armbruster
  1 sibling, 0 replies; 7+ messages in thread
From: Markus Armbruster @ 2023-10-06 18:41 UTC (permalink / raw)
  To: qemu-devel; +Cc: Gerd Hoffmann, Marc-André Lureau

Markus Armbruster <armbru@redhat.com> writes:

> Local variables shadowing other local variables or parameters make the
> code needlessly hard to understand.  Bugs love to hide in such code.
> Evidence: "[PATCH v3 1/7] migration/rdma: Fix save_page method to fail
> on polling error".
>
> Enabling -Wshadow would prevent bugs like this one.  But we have to
> clean up all the offenders first.
>
> Quite a few people responded to my calls for help.  Thank you so much!
>
> I'm collecting patches in my git repo at
> https://repo.or.cz/qemu/armbru.git in branch shadow-next.  All but the
> last two are in a pending pull request.
>
> My test build is down to seven files with warnings.  "[PATCH v2 0/3]
> hexagon: GETPC() and shadowing fixes" takes care of four, but it needs a
> rebase.
>
> Remaining three:
>
>     In file included from ../hw/display/virtio-gpu-virgl.c:19:
>     ../hw/display/virtio-gpu-virgl.c: In function ‘virgl_cmd_submit_3d’:
>     /work/armbru/qemu/include/hw/virtio/virtio-gpu.h:228:16: warning: declaration of ‘s’ shadows a previous local [-Wshadow=compatible-local]
>       228 |         size_t s;                                                       \
>           |                ^
>     ../hw/display/virtio-gpu-virgl.c:215:5: note: in expansion of macro ‘VIRTIO_GPU_FILL_CMD’
>       215 |     VIRTIO_GPU_FILL_CMD(cs);
>           |     ^~~~~~~~~~~~~~~~~~~
>     ../hw/display/virtio-gpu-virgl.c:213:12: note: shadowed declaration is here
>       213 |     size_t s;
>           |            ^
>
>     In file included from ../contrib/vhost-user-gpu/virgl.h:18,
>                      from ../contrib/vhost-user-gpu/virgl.c:17:
>     ../contrib/vhost-user-gpu/virgl.c: In function ‘virgl_cmd_submit_3d’:
>     ../contrib/vhost-user-gpu/vugpu.h:167:16: warning: declaration of ‘s’ shadows a previous local [-Wshadow=compatible-local]
>       167 |         size_t s;                                               \
>           |                ^
>     ../contrib/vhost-user-gpu/virgl.c:203:5: note: in expansion of macro ‘VUGPU_FILL_CMD’
>       203 |     VUGPU_FILL_CMD(cs);
>           |     ^~~~~~~~~~~~~~
>     ../contrib/vhost-user-gpu/virgl.c:201:12: note: shadowed declaration is here
>       201 |     size_t s;
>           |            ^
>
>     ../contrib/vhost-user-gpu/vhost-user-gpu.c: In function ‘vg_resource_flush’:
>     ../contrib/vhost-user-gpu/vhost-user-gpu.c:837:29: warning: declaration of ‘i’ shadows a previous local [-Wshadow=local]
>       837 |             pixman_image_t *i =
>           |                             ^
>     ../contrib/vhost-user-gpu/vhost-user-gpu.c:757:9: note: shadowed declaration is here
>       757 |     int i;
>           |         ^
>
> Gerd, Marc-André, or anybody else?

Thomas posted patches:

    [PATCH] hw/virtio/virtio-gpu: Fix compiler warning when compiling with -Wshadow
    [PATCH] contrib/vhost-user-gpu: Fix compiler warning when compiling with -Wshadow

> More warnings may lurk in code my test build doesn't compile.  Need a
> full CI build with -Wshadow=local to find them.  Anybody care to kick
> one off?

Thomas did; see his reply downthread.

Thank you, Thomas!



^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [v3] Help wanted for enabling -Wshadow=local
  2023-10-06 17:55   ` Thomas Huth
@ 2023-10-06 19:08     ` Warner Losh
  2023-10-09  6:24       ` Markus Armbruster
  2023-10-09  7:50       ` Thomas Huth
  0 siblings, 2 replies; 7+ messages in thread
From: Warner Losh @ 2023-10-06 19:08 UTC (permalink / raw)
  To: Thomas Huth
  Cc: Markus Armbruster, QEMU Developers, Gerd Hoffmann,
	Marc-André Lureau

[-- Attachment #1: Type: text/plain, Size: 4305 bytes --]

On Fri, Oct 6, 2023, 11:55 AM Thomas Huth <thuth@redhat.com> wrote:

> On 06/10/2023 18.18, Thomas Huth wrote:
> > On 06/10/2023 16.45, Markus Armbruster wrote:
> >> Local variables shadowing other local variables or parameters make the
> >> code needlessly hard to understand.  Bugs love to hide in such code.
> >> Evidence: "[PATCH v3 1/7] migration/rdma: Fix save_page method to fail
> >> on polling error".
> >>
> >> Enabling -Wshadow would prevent bugs like this one.  But we have to
> >> clean up all the offenders first.
> >>
> >> Quite a few people responded to my calls for help.  Thank you so much!
> >>
> >> I'm collecting patches in my git repo at
> >> https://repo.or.cz/qemu/armbru.git in branch shadow-next.  All but the
> >> last two are in a pending pull request.
> >>
> >> My test build is down to seven files with warnings.  "[PATCH v2 0/3]
> >> hexagon: GETPC() and shadowing fixes" takes care of four, but it needs a
> >> rebase.
> >>
> >> Remaining three:
> >>
> >>      In file included from ../hw/display/virtio-gpu-virgl.c:19:
> >>      ../hw/display/virtio-gpu-virgl.c: In function
> ‘virgl_cmd_submit_3d’:
> >>      /work/armbru/qemu/include/hw/virtio/virtio-gpu.h:228:16: warning:
> >> declaration of ‘s’ shadows a previous local [-Wshadow=compatible-local]
> >>        228 |         size_t
> >> s;                                                       \
> >>            |                ^
> >>      ../hw/display/virtio-gpu-virgl.c:215:5: note: in expansion of
> macro
> >> ‘VIRTIO_GPU_FILL_CMD’
> >>        215 |     VIRTIO_GPU_FILL_CMD(cs);
> >>            |     ^~~~~~~~~~~~~~~~~~~
> >>      ../hw/display/virtio-gpu-virgl.c:213:12: note: shadowed
> declaration
> >> is here
> >>        213 |     size_t s;
> >>            |            ^
> >>
> >>      In file included from ../contrib/vhost-user-gpu/virgl.h:18,
> >>                       from ../contrib/vhost-user-gpu/virgl.c:17:
> >>      ../contrib/vhost-user-gpu/virgl.c: In function
> ‘virgl_cmd_submit_3d’:
> >>      ../contrib/vhost-user-gpu/vugpu.h:167:16: warning: declaration of
> ‘s’
> >> shadows a previous local [-Wshadow=compatible-local]
> >>        167 |         size_t
> >> s;                                               \
> >>            |                ^
> >>      ../contrib/vhost-user-gpu/virgl.c:203:5: note: in expansion of
> macro
> >> ‘VUGPU_FILL_CMD’
> >>        203 |     VUGPU_FILL_CMD(cs);
> >>            |     ^~~~~~~~~~~~~~
> >>      ../contrib/vhost-user-gpu/virgl.c:201:12: note: shadowed
> declaration
> >> is here
> >>        201 |     size_t s;
> >>            |            ^
> >>
> >>      ../contrib/vhost-user-gpu/vhost-user-gpu.c: In function
> >> ‘vg_resource_flush’:
> >>      ../contrib/vhost-user-gpu/vhost-user-gpu.c:837:29: warning:
> >> declaration of ‘i’ shadows a previous local [-Wshadow=local]
> >>        837 |             pixman_image_t *i =
> >>            |                             ^
> >>      ../contrib/vhost-user-gpu/vhost-user-gpu.c:757:9: note: shadowed
> >> declaration is here
> >>        757 |     int i;
> >>            |         ^
> >>
> >> Gerd, Marc-André, or anybody else?
> >>
> >> More warnings may lurk in code my test build doesn't compile.  Need a
> >> full CI build with -Wshadow=local to find them.  Anybody care to kick
> >> one off?
> >
> > I ran a build here (with -Werror enabled, so that it's easier to see
> where
> > it breaks):
> >
> >   https://gitlab.com/thuth/qemu/-/pipelines/1028023489
> >
> > ... but I didn't see any additional spots in the logs beside the ones
> that
> > you already listed.
>
> After adding two more patches to fix the above warnings, things look
> pretty
> good:
>
>   https://gitlab.com/thuth/qemu/-/pipelines/1028413030
>
> There are just some warnings left in the BSD code, as Warner already
> mentioned in his reply to v2 of your mail:
>
>   https://gitlab.com/thuth/qemu/-/jobs/5241420713


I think I have fixes for these. I need to merge what just landed into
bsd-user fork, rebase, test, the apply them to qemu master branch, retest
and send them off...

My illness has hung on longer than I thought so I'm still behind...

Warner


>   Thomas
>
>

[-- Attachment #2: Type: text/html, Size: 6429 bytes --]

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [v3] Help wanted for enabling -Wshadow=local
  2023-10-06 19:08     ` Warner Losh
@ 2023-10-09  6:24       ` Markus Armbruster
  2023-10-09  7:50       ` Thomas Huth
  1 sibling, 0 replies; 7+ messages in thread
From: Markus Armbruster @ 2023-10-09  6:24 UTC (permalink / raw)
  To: Warner Losh
  Cc: Thomas Huth, QEMU Developers, Gerd Hoffmann,
	Marc-André Lureau

Warner Losh <imp@bsdimp.com> writes:

> On Fri, Oct 6, 2023, 11:55 AM Thomas Huth <thuth@redhat.com> wrote:
>
>> On 06/10/2023 18.18, Thomas Huth wrote:
>> > On 06/10/2023 16.45, Markus Armbruster wrote:
>> >> Local variables shadowing other local variables or parameters make the
>> >> code needlessly hard to understand.  Bugs love to hide in such code.
>> >> Evidence: "[PATCH v3 1/7] migration/rdma: Fix save_page method to fail
>> >> on polling error".
>> >>
>> >> Enabling -Wshadow would prevent bugs like this one.  But we have to
>> >> clean up all the offenders first.
>> >>
>> >> Quite a few people responded to my calls for help.  Thank you so much!
>> >>
>> >> I'm collecting patches in my git repo at
>> >> https://repo.or.cz/qemu/armbru.git in branch shadow-next.  All but the
>> >> last two are in a pending pull request.

[...]

>> >> More warnings may lurk in code my test build doesn't compile.  Need a
>> >> full CI build with -Wshadow=local to find them.  Anybody care to kick
>> >> one off?
>> >
>> > I ran a build here (with -Werror enabled, so that it's easier to see where
>> > it breaks):
>> >
>> >   https://gitlab.com/thuth/qemu/-/pipelines/1028023489
>> >
>> > ... but I didn't see any additional spots in the logs beside the ones that
>> > you already listed.
>>
>> After adding two more patches to fix the above warnings, things look
>> pretty
>> good:
>>
>>   https://gitlab.com/thuth/qemu/-/pipelines/1028413030
>>
>> There are just some warnings left in the BSD code, as Warner already
>> mentioned in his reply to v2 of your mail:
>>
>>   https://gitlab.com/thuth/qemu/-/jobs/5241420713
>
>
> I think I have fixes for these. I need to merge what just landed into
> bsd-user fork, rebase, test, the apply them to qemu master branch, retest
> and send them off...
>
> My illness has hung on longer than I thought so I'm still behind...

Get well, and looking forward to your patches!



^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [v3] Help wanted for enabling -Wshadow=local
  2023-10-06 19:08     ` Warner Losh
  2023-10-09  6:24       ` Markus Armbruster
@ 2023-10-09  7:50       ` Thomas Huth
  1 sibling, 0 replies; 7+ messages in thread
From: Thomas Huth @ 2023-10-09  7:50 UTC (permalink / raw)
  To: Warner Losh
  Cc: Markus Armbruster, QEMU Developers, Gerd Hoffmann,
	Marc-André Lureau

On 06/10/2023 21.08, Warner Losh wrote:
> 
> 
> On Fri, Oct 6, 2023, 11:55 AM Thomas Huth <thuth@redhat.com 
> <mailto:thuth@redhat.com>> wrote:
> 
>     On 06/10/2023 18.18, Thomas Huth wrote:
>      > On 06/10/2023 16.45, Markus Armbruster wrote:
>      >> Local variables shadowing other local variables or parameters make the
>      >> code needlessly hard to understand.  Bugs love to hide in such code.
>      >> Evidence: "[PATCH v3 1/7] migration/rdma: Fix save_page method to fail
>      >> on polling error".
>      >>
>      >> Enabling -Wshadow would prevent bugs like this one.  But we have to
>      >> clean up all the offenders first.
>      >>
>      >> Quite a few people responded to my calls for help.  Thank you so much!
>      >>
>      >> I'm collecting patches in my git repo at
>      >> https://repo.or.cz/qemu/armbru.git
>     <https://repo.or.cz/qemu/armbru.git> in branch shadow-next.  All but the
>      >> last two are in a pending pull request.
>      >>
>      >> My test build is down to seven files with warnings.  "[PATCH v2 0/3]
>      >> hexagon: GETPC() and shadowing fixes" takes care of four, but it needs a
>      >> rebase.
>      >>
>      >> Remaining three:
>      >>
>      >>      In file included from ../hw/display/virtio-gpu-virgl.c:19:
>      >>      ../hw/display/virtio-gpu-virgl.c: In function
>     ‘virgl_cmd_submit_3d’:
>      >>      /work/armbru/qemu/include/hw/virtio/virtio-gpu.h:228:16: warning:
>      >> declaration of ‘s’ shadows a previous local [-Wshadow=compatible-local]
>      >>        228 |         size_t
>      >> s;                                                       \
>      >>            |                ^
>      >>      ../hw/display/virtio-gpu-virgl.c:215:5: note: in expansion of
>     macro
>      >> ‘VIRTIO_GPU_FILL_CMD’
>      >>        215 |     VIRTIO_GPU_FILL_CMD(cs);
>      >>            |     ^~~~~~~~~~~~~~~~~~~
>      >>      ../hw/display/virtio-gpu-virgl.c:213:12: note: shadowed
>     declaration
>      >> is here
>      >>        213 |     size_t s;
>      >>            |            ^
>      >>
>      >>      In file included from ../contrib/vhost-user-gpu/virgl.h:18,
>      >>                       from ../contrib/vhost-user-gpu/virgl.c:17:
>      >>      ../contrib/vhost-user-gpu/virgl.c: In function
>     ‘virgl_cmd_submit_3d’:
>      >>      ../contrib/vhost-user-gpu/vugpu.h:167:16: warning: declaration
>     of ‘s’
>      >> shadows a previous local [-Wshadow=compatible-local]
>      >>        167 |         size_t
>      >> s;                                               \
>      >>            |                ^
>      >>      ../contrib/vhost-user-gpu/virgl.c:203:5: note: in expansion of
>     macro
>      >> ‘VUGPU_FILL_CMD’
>      >>        203 |     VUGPU_FILL_CMD(cs);
>      >>            |     ^~~~~~~~~~~~~~
>      >>      ../contrib/vhost-user-gpu/virgl.c:201:12: note: shadowed
>     declaration
>      >> is here
>      >>        201 |     size_t s;
>      >>            |            ^
>      >>
>      >>      ../contrib/vhost-user-gpu/vhost-user-gpu.c: In function
>      >> ‘vg_resource_flush’:
>      >>      ../contrib/vhost-user-gpu/vhost-user-gpu.c:837:29: warning:
>      >> declaration of ‘i’ shadows a previous local [-Wshadow=local]
>      >>        837 |             pixman_image_t *i =
>      >>            |                             ^
>      >>      ../contrib/vhost-user-gpu/vhost-user-gpu.c:757:9: note: shadowed
>      >> declaration is here
>      >>        757 |     int i;
>      >>            |         ^
>      >>
>      >> Gerd, Marc-André, or anybody else?
>      >>
>      >> More warnings may lurk in code my test build doesn't compile.  Need a
>      >> full CI build with -Wshadow=local to find them.  Anybody care to kick
>      >> one off?
>      >
>      > I ran a build here (with -Werror enabled, so that it's easier to see
>     where
>      > it breaks):
>      >
>      > https://gitlab.com/thuth/qemu/-/pipelines/1028023489
>     <https://gitlab.com/thuth/qemu/-/pipelines/1028023489>
>      >
>      > ... but I didn't see any additional spots in the logs beside the ones
>     that
>      > you already listed.
> 
>     After adding two more patches to fix the above warnings, things look pretty
>     good:
> 
>     https://gitlab.com/thuth/qemu/-/pipelines/1028413030
>     <https://gitlab.com/thuth/qemu/-/pipelines/1028413030>
> 
>     There are just some warnings left in the BSD code, as Warner already
>     mentioned in his reply to v2 of your mail:
> 
>     https://gitlab.com/thuth/qemu/-/jobs/5241420713
>     <https://gitlab.com/thuth/qemu/-/jobs/5241420713>
> 
> 
> I think I have fixes for these. I need to merge what just landed into 
> bsd-user fork, rebase, test, the apply them to qemu master branch, retest 
> and send them off...
> 
> My illness has hung on longer than I thought so I'm still behind...

Get well soon again! ... and no worries about the -Wshadow=local patches in 
the BSD code, there is no hurry - The BSDs are using Clang by default, so 
that option won't get enabled by default there anyway yet - I had to switch 
to GCC in the CI pipeline to trigger those, and I guess only very few people 
will use GCC to compile QEMU on FreeBSD.

  Thomas



^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2023-10-09  7:50 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-10-06 14:45 [v3] Help wanted for enabling -Wshadow=local Markus Armbruster
2023-10-06 16:18 ` Thomas Huth
2023-10-06 17:55   ` Thomas Huth
2023-10-06 19:08     ` Warner Losh
2023-10-09  6:24       ` Markus Armbruster
2023-10-09  7:50       ` Thomas Huth
2023-10-06 18:41 ` Markus Armbruster

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).