On Fri, Oct 6, 2023, 11:55 AM Thomas Huth <th...@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 > >
