| Message ID | 20231026053115.2066744-2-armbru@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | Enable -Wshadow=local | expand |
On 26/10/2023 07.31, 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: commit bbde656263d (migration/rdma: Fix save_page method to > fail on polling error). > > Enable -Wshadow=local to prevent such issues. Possible thanks to > recent cleanups. Enabling -Wshadow would prevent more issues, but > we're not yet ready for that. > > As usual, the warning is only enabled when the compiler recognizes it. > GCC does, Clang doesn't. > > Some shadowed locals remain in bsd-user. Since BSD prefers Clang, > let's not wait for its cleanup. > > Signed-off-by: Markus Armbruster <armbru@redhat.com> > --- > meson.build | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/meson.build b/meson.build > index dcef8b1e79..89220443b8 100644 > --- a/meson.build > +++ b/meson.build > @@ -462,6 +462,7 @@ warn_flags = [ > '-Wno-tautological-type-limit-compare', > '-Wno-psabi', > '-Wno-gnu-variable-sized-type-not-at-end', > + '-Wshadow=local', > ] Reviewed-by: Thomas Huth <thuth@redhat.com>
On Wed, Oct 25, 2023, 11:31 PM Markus Armbruster <armbru@redhat.com> wrote: > Local variables shadowing other local variables or parameters make the > code needlessly hard to understand. Bugs love to hide in such code. > Evidence: commit bbde656263d (migration/rdma: Fix save_page method to > fail on polling error). > > Enable -Wshadow=local to prevent such issues. Possible thanks to > recent cleanups. Enabling -Wshadow would prevent more issues, but > we're not yet ready for that. > > As usual, the warning is only enabled when the compiler recognizes it. > GCC does, Clang doesn't. > > Some shadowed locals remain in bsd-user. Since BSD prefers Clang, > let's not wait for its cleanup. > > Signed-off-by: Markus Armbruster <armbru@redhat.com> > --- > meson.build | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/meson.build b/meson.build > index dcef8b1e79..89220443b8 100644 > --- a/meson.build > +++ b/meson.build > @@ -462,6 +462,7 @@ warn_flags = [ > '-Wno-tautological-type-limit-compare', > '-Wno-psabi', > '-Wno-gnu-variable-sized-type-not-at-end', > + '-Wshadow=local', > Does this work with clang? I've not had good luck enabling it. Warner ] > > if targetos != 'darwin' > -- > 2.41.0 > >
On 26/10/2023 07.51, Warner Losh wrote: > > > On Wed, Oct 25, 2023, 11:31 PM Markus Armbruster <armbru@redhat.com > <mailto:armbru@redhat.com>> wrote: > > Local variables shadowing other local variables or parameters make the > code needlessly hard to understand. Bugs love to hide in such code. > Evidence: commit bbde656263d (migration/rdma: Fix save_page method to > fail on polling error). > > Enable -Wshadow=local to prevent such issues. Possible thanks to > recent cleanups. Enabling -Wshadow would prevent more issues, but > we're not yet ready for that. > > As usual, the warning is only enabled when the compiler recognizes it. > GCC does, Clang doesn't. > > Some shadowed locals remain in bsd-user. Since BSD prefers Clang, > let's not wait for its cleanup. > > Signed-off-by: Markus Armbruster <armbru@redhat.com > <mailto:armbru@redhat.com>> > --- > meson.build | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/meson.build b/meson.build > index dcef8b1e79..89220443b8 100644 > --- a/meson.build > +++ b/meson.build > @@ -462,6 +462,7 @@ warn_flags = [ > '-Wno-tautological-type-limit-compare', > '-Wno-psabi', > '-Wno-gnu-variable-sized-type-not-at-end', > + '-Wshadow=local', > > > Does this work with clang? I've not had good luck enabling it. The flags are added via cc.get_supported_arguments(warn_flags), so meson checks whether the compiler supports them before blindly adding them to the list. That means it should get ignored with Clang, i.e. we should be ok for the remaining spots in the bsd-user code, assuming that most FreeBSD users will use Clang to compile QEMU. Thomas
On 26/10/23 07:31, 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: commit bbde656263d (migration/rdma: Fix save_page method to > fail on polling error). > > Enable -Wshadow=local to prevent such issues. Possible thanks to > recent cleanups. Enabling -Wshadow would prevent more issues, but > we're not yet ready for that. > > As usual, the warning is only enabled when the compiler recognizes it. > GCC does, Clang doesn't. > > Some shadowed locals remain in bsd-user. Since BSD prefers Clang, > let's not wait for its cleanup. > > Signed-off-by: Markus Armbruster <armbru@redhat.com> > --- > meson.build | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/meson.build b/meson.build > index dcef8b1e79..89220443b8 100644 > --- a/meson.build > +++ b/meson.build > @@ -462,6 +462,7 @@ warn_flags = [ > '-Wno-tautological-type-limit-compare', > '-Wno-psabi', > '-Wno-gnu-variable-sized-type-not-at-end', > + '-Wshadow=local', > ] > > if targetos != 'darwin' Using Clang on Darwin: $ ../configure The Meson build system Version: 1.2.1 Build type: native build Project name: qemu Project version: 8.1.50 C compiler for the host machine: cc (clang 15.0.0 "Apple clang version 15.0.0 (clang-1500.0.40.1)") C linker for the host machine: cc ld64 1015.7 Host machine cpu family: aarch64 Host machine cpu: aarch64 Program sh found: YES (/bin/sh) Objective-C compiler for the host machine: clang (clang 15.0.0) Objective-C linker for the host machine: clang ld64 1015.7 Program bzip2 found: YES (/usr/bin/bzip2) Program iasl found: YES (/opt/homebrew/bin/iasl) Compiler for C supports arguments -fno-pie: YES Compiler for C supports arguments -no-pie: NO Compiler for C supports link arguments -Wl,-z,relro: NO Compiler for C supports link arguments -Wl,-z,now: NO Compiler for C supports link arguments -Wl,--warn-common: NO Compiler for C supports arguments -Wundef: YES Compiler for C supports arguments -Wwrite-strings: YES Compiler for C supports arguments -Wmissing-prototypes: YES Compiler for C supports arguments -Wstrict-prototypes: YES Compiler for C supports arguments -Wredundant-decls: YES Compiler for C supports arguments -Wold-style-declaration: NO Compiler for C supports arguments -Wold-style-definition: YES Compiler for C supports arguments -Wtype-limits: YES Compiler for C supports arguments -Wformat-security: YES Compiler for C supports arguments -Wformat-y2k: YES Compiler for C supports arguments -Winit-self: YES Compiler for C supports arguments -Wignored-qualifiers: YES Compiler for C supports arguments -Wempty-body: YES Compiler for C supports arguments -Wnested-externs: YES Compiler for C supports arguments -Wendif-labels: YES Compiler for C supports arguments -Wexpansion-to-defined: YES Compiler for C supports arguments -Wimplicit-fallthrough=2: NO Compiler for C supports arguments -Wmissing-format-attribute: YES Compiler for C supports arguments -Wno-initializer-overrides: YES Compiler for C supports arguments -Wno-missing-include-dirs: YES Compiler for C supports arguments -Wno-shift-negative-value: YES Compiler for C supports arguments -Wno-string-plus-int: YES Compiler for C supports arguments -Wno-typedef-redefinition: YES Compiler for C supports arguments -Wno-tautological-type-limit-compare: YES Compiler for C supports arguments -Wno-psabi: YES Compiler for C supports arguments -Wno-gnu-variable-sized-type-not-at-end: YES Compiler for C supports arguments -Wshadow=local: NO Compiler for Objective-C supports arguments -Wundef: YES Compiler for Objective-C supports arguments -Wwrite-strings: YES Compiler for Objective-C supports arguments -Wmissing-prototypes: YES Compiler for Objective-C supports arguments -Wstrict-prototypes: YES Compiler for Objective-C supports arguments -Wredundant-decls: YES Compiler for Objective-C supports arguments -Wold-style-declaration: NO Compiler for Objective-C supports arguments -Wold-style-definition: YES Compiler for Objective-C supports arguments -Wtype-limits: YES Compiler for Objective-C supports arguments -Wformat-security: YES Compiler for Objective-C supports arguments -Wformat-y2k: YES Compiler for Objective-C supports arguments -Winit-self: YES Compiler for Objective-C supports arguments -Wignored-qualifiers: YES Compiler for Objective-C supports arguments -Wempty-body: YES Compiler for Objective-C supports arguments -Wnested-externs: YES Compiler for Objective-C supports arguments -Wendif-labels: YES Compiler for Objective-C supports arguments -Wexpansion-to-defined: YES Compiler for Objective-C supports arguments -Wimplicit-fallthrough=2: NO Compiler for Objective-C supports arguments -Wmissing-format-attribute: YES Compiler for Objective-C supports arguments -Wno-initializer-overrides: YES Compiler for Objective-C supports arguments -Wno-missing-include-dirs: YES Compiler for Objective-C supports arguments -Wno-shift-negative-value: YES Compiler for Objective-C supports arguments -Wno-string-plus-int: YES Compiler for Objective-C supports arguments -Wno-typedef-redefinition: YES Compiler for Objective-C supports arguments -Wno-tautological-type-limit-compare: YES Compiler for Objective-C supports arguments -Wno-psabi: YES Compiler for Objective-C supports arguments -Wno-gnu-variable-sized-type-not-at-end: YES Compiler for Objective-C supports arguments -Wshadow=local: NO So: Tested-by: Philippe Mathieu-Daudé <philmd@linaro.org> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org> Now don't blame me for posting patches with trigger shadow=local warnings because I am not testing that locally. I find it a bit unfair to force me rely on CI or other machines rather than my host machine to check for warnings. I'd have rather waited this option support lands first in Clang before enabling this flag. Regards, Phil.
On 26/10/2023 07.58, Philippe Mathieu-Daudé wrote: > On 26/10/23 07:31, 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: commit bbde656263d (migration/rdma: Fix save_page method to >> fail on polling error). >> >> Enable -Wshadow=local to prevent such issues. Possible thanks to >> recent cleanups. Enabling -Wshadow would prevent more issues, but >> we're not yet ready for that. >> >> As usual, the warning is only enabled when the compiler recognizes it. >> GCC does, Clang doesn't. >> >> Some shadowed locals remain in bsd-user. Since BSD prefers Clang, >> let's not wait for its cleanup. >> >> Signed-off-by: Markus Armbruster <armbru@redhat.com> >> --- >> meson.build | 1 + >> 1 file changed, 1 insertion(+) >> >> diff --git a/meson.build b/meson.build >> index dcef8b1e79..89220443b8 100644 >> --- a/meson.build >> +++ b/meson.build >> @@ -462,6 +462,7 @@ warn_flags = [ >> '-Wno-tautological-type-limit-compare', >> '-Wno-psabi', >> '-Wno-gnu-variable-sized-type-not-at-end', >> + '-Wshadow=local', >> ] >> if targetos != 'darwin' > > Using Clang on Darwin: > > $ ../configure > The Meson build system > Version: 1.2.1 > Build type: native build > Project name: qemu > Project version: 8.1.50 > C compiler for the host machine: cc (clang 15.0.0 "Apple clang version > 15.0.0 (clang-1500.0.40.1)") > C linker for the host machine: cc ld64 1015.7 > Host machine cpu family: aarch64 > Host machine cpu: aarch64 > Program sh found: YES (/bin/sh) > Objective-C compiler for the host machine: clang (clang 15.0.0) > Objective-C linker for the host machine: clang ld64 1015.7 > Program bzip2 found: YES (/usr/bin/bzip2) > Program iasl found: YES (/opt/homebrew/bin/iasl) > Compiler for C supports arguments -fno-pie: YES > Compiler for C supports arguments -no-pie: NO > Compiler for C supports link arguments -Wl,-z,relro: NO > Compiler for C supports link arguments -Wl,-z,now: NO > Compiler for C supports link arguments -Wl,--warn-common: NO > Compiler for C supports arguments -Wundef: YES > Compiler for C supports arguments -Wwrite-strings: YES > Compiler for C supports arguments -Wmissing-prototypes: YES > Compiler for C supports arguments -Wstrict-prototypes: YES > Compiler for C supports arguments -Wredundant-decls: YES > Compiler for C supports arguments -Wold-style-declaration: NO > Compiler for C supports arguments -Wold-style-definition: YES > Compiler for C supports arguments -Wtype-limits: YES > Compiler for C supports arguments -Wformat-security: YES > Compiler for C supports arguments -Wformat-y2k: YES > Compiler for C supports arguments -Winit-self: YES > Compiler for C supports arguments -Wignored-qualifiers: YES > Compiler for C supports arguments -Wempty-body: YES > Compiler for C supports arguments -Wnested-externs: YES > Compiler for C supports arguments -Wendif-labels: YES > Compiler for C supports arguments -Wexpansion-to-defined: YES > Compiler for C supports arguments -Wimplicit-fallthrough=2: NO > Compiler for C supports arguments -Wmissing-format-attribute: YES > Compiler for C supports arguments -Wno-initializer-overrides: YES > Compiler for C supports arguments -Wno-missing-include-dirs: YES > Compiler for C supports arguments -Wno-shift-negative-value: YES > Compiler for C supports arguments -Wno-string-plus-int: YES > Compiler for C supports arguments -Wno-typedef-redefinition: YES > Compiler for C supports arguments -Wno-tautological-type-limit-compare: YES > Compiler for C supports arguments -Wno-psabi: YES > Compiler for C supports arguments -Wno-gnu-variable-sized-type-not-at-end: YES > Compiler for C supports arguments -Wshadow=local: NO > Compiler for Objective-C supports arguments -Wundef: YES > Compiler for Objective-C supports arguments -Wwrite-strings: YES > Compiler for Objective-C supports arguments -Wmissing-prototypes: YES > Compiler for Objective-C supports arguments -Wstrict-prototypes: YES > Compiler for Objective-C supports arguments -Wredundant-decls: YES > Compiler for Objective-C supports arguments -Wold-style-declaration: NO > Compiler for Objective-C supports arguments -Wold-style-definition: YES > Compiler for Objective-C supports arguments -Wtype-limits: YES > Compiler for Objective-C supports arguments -Wformat-security: YES > Compiler for Objective-C supports arguments -Wformat-y2k: YES > Compiler for Objective-C supports arguments -Winit-self: YES > Compiler for Objective-C supports arguments -Wignored-qualifiers: YES > Compiler for Objective-C supports arguments -Wempty-body: YES > Compiler for Objective-C supports arguments -Wnested-externs: YES > Compiler for Objective-C supports arguments -Wendif-labels: YES > Compiler for Objective-C supports arguments -Wexpansion-to-defined: YES > Compiler for Objective-C supports arguments -Wimplicit-fallthrough=2: NO > Compiler for Objective-C supports arguments -Wmissing-format-attribute: YES > Compiler for Objective-C supports arguments -Wno-initializer-overrides: YES > Compiler for Objective-C supports arguments -Wno-missing-include-dirs: YES > Compiler for Objective-C supports arguments -Wno-shift-negative-value: YES > Compiler for Objective-C supports arguments -Wno-string-plus-int: YES > Compiler for Objective-C supports arguments -Wno-typedef-redefinition: YES > Compiler for Objective-C supports arguments > -Wno-tautological-type-limit-compare: YES > Compiler for Objective-C supports arguments -Wno-psabi: YES > Compiler for Objective-C supports arguments > -Wno-gnu-variable-sized-type-not-at-end: YES > Compiler for Objective-C supports arguments -Wshadow=local: NO > > So: > > Tested-by: Philippe Mathieu-Daudé <philmd@linaro.org> > Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org> > > Now don't blame me for posting patches with trigger shadow=local > warnings because I am not testing that locally. > > I find it a bit unfair to force me rely on CI or other machines > rather than my host machine to check for warnings. I'd have > rather waited this option support lands first in Clang before > enabling this flag. Huh, that situation is already pre-existing, e.g. with -Wimplicit-fallthrough=2 ... and if you're too afraid, you can always install gcc via homebrew to check. Thomas
On 26/10/23 08:12, Thomas Huth wrote: > On 26/10/2023 07.58, Philippe Mathieu-Daudé wrote: >> On 26/10/23 07:31, 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: commit bbde656263d (migration/rdma: Fix save_page method to >>> fail on polling error). >>> >>> Enable -Wshadow=local to prevent such issues. Possible thanks to >>> recent cleanups. Enabling -Wshadow would prevent more issues, but >>> we're not yet ready for that. >>> >>> As usual, the warning is only enabled when the compiler recognizes it. >>> GCC does, Clang doesn't. >>> >>> Some shadowed locals remain in bsd-user. Since BSD prefers Clang, >>> let's not wait for its cleanup. >>> >>> Signed-off-by: Markus Armbruster <armbru@redhat.com> >>> --- >>> meson.build | 1 + >>> 1 file changed, 1 insertion(+) >>> >>> diff --git a/meson.build b/meson.build >>> index dcef8b1e79..89220443b8 100644 >>> --- a/meson.build >>> +++ b/meson.build >>> @@ -462,6 +462,7 @@ warn_flags = [ >>> '-Wno-tautological-type-limit-compare', >>> '-Wno-psabi', >>> '-Wno-gnu-variable-sized-type-not-at-end', >>> + '-Wshadow=local', >>> ] >>> if targetos != 'darwin' >> >> Using Clang on Darwin: >> >> $ ../configure >> The Meson build system >> Version: 1.2.1 >> Build type: native build >> Project name: qemu >> Project version: 8.1.50 >> C compiler for the host machine: cc (clang 15.0.0 "Apple clang version >> 15.0.0 (clang-1500.0.40.1)") >> C linker for the host machine: cc ld64 1015.7 >> Host machine cpu family: aarch64 >> Host machine cpu: aarch64 >> Program sh found: YES (/bin/sh) >> Objective-C compiler for the host machine: clang (clang 15.0.0) >> Objective-C linker for the host machine: clang ld64 1015.7 >> Compiler for Objective-C supports arguments -Wshadow=local: NO >> >> So: >> >> Tested-by: Philippe Mathieu-Daudé <philmd@linaro.org> >> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org> >> >> Now don't blame me for posting patches with trigger shadow=local >> warnings because I am not testing that locally. >> >> I find it a bit unfair to force me rely on CI or other machines >> rather than my host machine to check for warnings. I'd have >> rather waited this option support lands first in Clang before >> enabling this flag. > > Huh, that situation is already pre-existing, e.g. with > -Wimplicit-fallthrough=2 ... and if you're too afraid, you can always > install gcc via homebrew to check. OK, fine.
Philippe Mathieu-Daudé <philmd@linaro.org> writes: > On 26/10/23 08:12, Thomas Huth wrote: >> On 26/10/2023 07.58, Philippe Mathieu-Daudé wrote: [...] >>> $ ../configure >>> The Meson build system >>> Version: 1.2.1 >>> Build type: native build >>> Project name: qemu >>> Project version: 8.1.50 >>> C compiler for the host machine: cc (clang 15.0.0 "Apple clang version 15.0.0 (clang-1500.0.40.1)") >>> C linker for the host machine: cc ld64 1015.7 >>> Host machine cpu family: aarch64 >>> Host machine cpu: aarch64 >>> Program sh found: YES (/bin/sh) >>> Objective-C compiler for the host machine: clang (clang 15.0.0) >>> Objective-C linker for the host machine: clang ld64 1015.7 > > >>> Compiler for Objective-C supports arguments -Wshadow=local: NO >>> >>> So: >>> >>> Tested-by: Philippe Mathieu-Daudé <philmd@linaro.org> >>> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org> Thanks! >>> Now don't blame me for posting patches with trigger shadow=local >>> warnings because I am not testing that locally. >>> >>> I find it a bit unfair to force me rely on CI or other machines >>> rather than my host machine to check for warnings. I'd have >>> rather waited this option support lands first in Clang before >>> enabling this flag. I'm not forcing anyone just yet, I'm merely posting a patch to solicit feedback :) PRO: It stops the backsliding. Thomas had to fix two new instances already. CON: Developers using only Clang may post patches that fail CI. We don't know how annoying that will be in practice. >> Huh, that situation is already pre-existing, e.g. with -Wimplicit-fallthrough=2 ... and if you're too afraid, you can always install gcc via homebrew to check. > > OK, fine. I suggest to take the patch now, and if the CON turns out to outweigh the PRO, revert it.
On Thu, Oct 26, 2023 at 07:58:42AM +0200, Philippe Mathieu-Daudé wrote: > On 26/10/23 07:31, 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: commit bbde656263d (migration/rdma: Fix save_page method to > > fail on polling error). > > > > Enable -Wshadow=local to prevent such issues. Possible thanks to > > recent cleanups. Enabling -Wshadow would prevent more issues, but > > we're not yet ready for that. > > > > As usual, the warning is only enabled when the compiler recognizes it. > > GCC does, Clang doesn't. > > > > Some shadowed locals remain in bsd-user. Since BSD prefers Clang, > > let's not wait for its cleanup. > > > > Signed-off-by: Markus Armbruster <armbru@redhat.com> > > --- > > meson.build | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/meson.build b/meson.build > > index dcef8b1e79..89220443b8 100644 > > --- a/meson.build > > +++ b/meson.build > > @@ -462,6 +462,7 @@ warn_flags = [ > > '-Wno-tautological-type-limit-compare', > > '-Wno-psabi', > > '-Wno-gnu-variable-sized-type-not-at-end', > > + '-Wshadow=local', > > ] > > if targetos != 'darwin' > > Now don't blame me for posting patches with trigger shadow=local > warnings because I am not testing that locally. > > I find it a bit unfair to force me rely on CI or other machines > rather than my host machine to check for warnings. I'd have > rather waited this option support lands first in Clang before > enabling this flag. QEMU has never required regular contributors to submit code that compiles perfectly on every supported platform. Only that they make a fair effort to have it compile on their platform, and respond to feedback if a reviewer points out a problem for a different platform. Subsystem maintainers though should be ensuring code is warning free on every platform by running through CI before submitting a pull request. This centralization of CI repsonsibilities on maintainers is one of the downsides of our mailing list workflow, as compared to gitforges where the regular contributors would immediately trigger & see CI reports from every merge request they open. With regards, Daniel
diff --git a/meson.build b/meson.build index dcef8b1e79..89220443b8 100644 --- a/meson.build +++ b/meson.build @@ -462,6 +462,7 @@ warn_flags = [ '-Wno-tautological-type-limit-compare', '-Wno-psabi', '-Wno-gnu-variable-sized-type-not-at-end', + '-Wshadow=local', ] if targetos != 'darwin'
Local variables shadowing other local variables or parameters make the code needlessly hard to understand. Bugs love to hide in such code. Evidence: commit bbde656263d (migration/rdma: Fix save_page method to fail on polling error). Enable -Wshadow=local to prevent such issues. Possible thanks to recent cleanups. Enabling -Wshadow would prevent more issues, but we're not yet ready for that. As usual, the warning is only enabled when the compiler recognizes it. GCC does, Clang doesn't. Some shadowed locals remain in bsd-user. Since BSD prefers Clang, let's not wait for its cleanup. Signed-off-by: Markus Armbruster <armbru@redhat.com> --- meson.build | 1 + 1 file changed, 1 insertion(+)