[PATCH 0/1] Enable -Wshadow=local

[PATCH 0/1] Enable -Wshadow=local

[Markus Armbruster]

Requires Brian's pull request and two patches from Thomas to compile:

    [PULL 0/2] hex queue - GETPC() fixes, shadowing fixes
    [PATCH v2] block/snapshot: Fix compiler warning with -Wshadow=local
    [PATCH v2] migration/ram: Fix compilation with -Wshadow=local

Stefan, the PR was posted a week ago; anything blocking it?

Warner, I believe not waiting for your cleanup of bsd-user is fine.
Please holler if it isn't.

Based-on: <20231019021733.2258592-1-bcain@quicinc.com>
Based-on: <20231023175038.111607-1-thuth@redhat.com>
Based-on: <20231024092220.55305-1-thuth@redhat.com>

Markus Armbruster (1):
  meson: Enable -Wshadow=local

 meson.build | 1 +
 1 file changed, 1 insertion(+)

-- 
2.41.0

[PATCH 1/1] meson: Enable -Wshadow=local

[Markus Armbruster]

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'
-- 
2.41.0

Re: [PATCH 1/1] meson: Enable -Wshadow=local

[Thomas Huth]

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>

Re: [PATCH 1/1] meson: Enable -Wshadow=local

[Warner Losh]

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

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
>
>

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

Re: [PATCH 0/1] Enable -Wshadow=local

[Warner Losh]

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

On Wed, Oct 25, 2023, 11:31 PM Markus Armbruster <armbru@redhat.com> wrote:

> Requires Brian's pull request and two patches from Thomas to compile:
>
>     [PULL 0/2] hex queue - GETPC() fixes, shadowing fixes
>     [PATCH v2] block/snapshot: Fix compiler warning with -Wshadow=local
>     [PATCH v2] migration/ram: Fix compilation with -Wshadow=local
>
> Stefan, the PR was posted a week ago; anything blocking it?
>
> Warner, I believe not waiting for your cleanup of bsd-user is fine.
> Please holler if it isn't.
>

If it's not enabled by default for Clang, then sure. It's only one small
change at this point, but i was ill for a few weeks (much longer than i
thought I'd be) and am still catching up.

Warner

 <20231019021733.2258592-1-bcain@quicinc.com>

> Based-on: <20231023175038.111607-1-thuth@redhat.com>
> Based-on: <20231024092220.55305-1-thuth@redhat.com>
>
> Markus Armbruster (1):
>   meson: Enable -Wshadow=local
>
>  meson.build | 1 +
>  1 file changed, 1 insertion(+)
>
> --
> 2.41.0
>
>

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

Re: [PATCH 1/1] meson: Enable -Wshadow=local

[Thomas Huth]

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

Re: [PATCH 1/1] meson: Enable -Wshadow=local

[Philippe Mathieu-Daudé]

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.

Re: [PATCH 1/1] meson: Enable -Wshadow=local

[Thomas Huth]

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

Re: [PATCH 1/1] meson: Enable -Wshadow=local

[Philippe Mathieu-Daudé]

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.

Re: [PATCH 1/1] meson: Enable -Wshadow=local

[Markus Armbruster]

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.

Re: [PATCH 1/1] meson: Enable -Wshadow=local

[Daniel P. Berrangé]

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
-- 
|: 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: [PATCH 0/1] Enable -Wshadow=local

[Stefan Hajnoczi]

On Thu, 26 Oct 2023 at 14:32, Markus Armbruster <armbru@redhat.com> wrote:
>
> Requires Brian's pull request and two patches from Thomas to compile:
>
>     [PULL 0/2] hex queue - GETPC() fixes, shadowing fixes
>     [PATCH v2] block/snapshot: Fix compiler warning with -Wshadow=local
>     [PATCH v2] migration/ram: Fix compilation with -Wshadow=local
>
> Stefan, the PR was posted a week ago; anything blocking it?

It's not in a pull request, so I won't see it. I don't have tooling
that can spot individual patch series that need to go into
qemu.git/master, so I rely on being emailed about them.

Would you like me to merge this patch series into qemu.git/master?

Stefan

> Warner, I believe not waiting for your cleanup of bsd-user is fine.
> Please holler if it isn't.
>
> Based-on: <20231019021733.2258592-1-bcain@quicinc.com>
> Based-on: <20231023175038.111607-1-thuth@redhat.com>
> Based-on: <20231024092220.55305-1-thuth@redhat.com>
>
> Markus Armbruster (1):
>   meson: Enable -Wshadow=local
>
>  meson.build | 1 +
>  1 file changed, 1 insertion(+)
>
> --
> 2.41.0
>
>

RE: [PATCH 0/1] Enable -Wshadow=local

[Brian Cain]

> -----Original Message-----
> From: Stefan Hajnoczi <stefanha@gmail.com>
> Sent: Thursday, October 26, 2023 7:52 PM
> To: Markus Armbruster <armbru@redhat.com>
> Cc: qemu-devel@nongnu.org; pbonzini@redhat.com;
> marcandre.lureau@redhat.com; berrange@redhat.com; thuth@redhat.com;
> philmd@linaro.org; Brian Cain <bcain@quicinc.com>; imp@bsdimp.com;
> stefanha@redhat.com
> Subject: Re: [PATCH 0/1] Enable -Wshadow=local
> 
> WARNING: This email originated from outside of Qualcomm. Please be wary of
> any links or attachments, and do not enable macros.
> 
> On Thu, 26 Oct 2023 at 14:32, Markus Armbruster <armbru@redhat.com>
> wrote:
> >
> > Requires Brian's pull request and two patches from Thomas to compile:
> >
> >     [PULL 0/2] hex queue - GETPC() fixes, shadowing fixes
> >     [PATCH v2] block/snapshot: Fix compiler warning with -Wshadow=local
> >     [PATCH v2] migration/ram: Fix compilation with -Wshadow=local
> >
> > Stefan, the PR was posted a week ago; anything blocking it?
> 
> It's not in a pull request, so I won't see it. I don't have tooling
> that can spot individual patch series that need to go into
> qemu.git/master, so I rely on being emailed about them.

My mistake -- I thought I had emailed you.  But I see now that I likely used the wrong email address.

> 
> Would you like me to merge this patch series into qemu.git/master?
> 
> Stefan
> 
> > Warner, I believe not waiting for your cleanup of bsd-user is fine.
> > Please holler if it isn't.
> >
> > Based-on: <20231019021733.2258592-1-bcain@quicinc.com>
> > Based-on: <20231023175038.111607-1-thuth@redhat.com>
> > Based-on: <20231024092220.55305-1-thuth@redhat.com>
> >
> > Markus Armbruster (1):
> >   meson: Enable -Wshadow=local
> >
> >  meson.build | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > --
> > 2.41.0
> >
> >

Re: [PATCH 0/1] Enable -Wshadow=local

[Markus Armbruster]

Stefan Hajnoczi <stefanha@gmail.com> writes:

> On Thu, 26 Oct 2023 at 14:32, Markus Armbruster <armbru@redhat.com> wrote:
>>
>> Requires Brian's pull request and two patches from Thomas to compile:
>>
>>     [PULL 0/2] hex queue - GETPC() fixes, shadowing fixes
>>     [PATCH v2] block/snapshot: Fix compiler warning with -Wshadow=local
>>     [PATCH v2] migration/ram: Fix compilation with -Wshadow=local
>>
>> Stefan, the PR was posted a week ago; anything blocking it?
>
> It's not in a pull request, so I won't see it. I don't have tooling
> that can spot individual patch series that need to go into
> qemu.git/master, so I rely on being emailed about them.

I'm inquiring about this one:

    https://lore.kernel.org/qemu-devel/20231019021733.2258592-1-bcain@quicinc.com/

Looks like a PR to me.

> Would you like me to merge this patch series into qemu.git/master?

Yes, I'd like you merge Brian's PR I linked to.

[...]

Re: [PATCH 0/1] Enable -Wshadow=local

[Stefan Hajnoczi]

On Fri, 27 Oct 2023 at 13:42, Markus Armbruster <armbru@redhat.com> wrote:
>
> Stefan Hajnoczi <stefanha@gmail.com> writes:
>
> > On Thu, 26 Oct 2023 at 14:32, Markus Armbruster <armbru@redhat.com> wrote:
> >>
> >> Requires Brian's pull request and two patches from Thomas to compile:
> >>
> >>     [PULL 0/2] hex queue - GETPC() fixes, shadowing fixes
> >>     [PATCH v2] block/snapshot: Fix compiler warning with -Wshadow=local
> >>     [PATCH v2] migration/ram: Fix compilation with -Wshadow=local
> >>
> >> Stefan, the PR was posted a week ago; anything blocking it?
> >
> > It's not in a pull request, so I won't see it. I don't have tooling
> > that can spot individual patch series that need to go into
> > qemu.git/master, so I rely on being emailed about them.
>
> I'm inquiring about this one:
>
>     https://lore.kernel.org/qemu-devel/20231019021733.2258592-1-bcain@quicinc.com/
>
> Looks like a PR to me.
>
> > Would you like me to merge this patch series into qemu.git/master?
>
> Yes, I'd like you merge Brian's PR I linked to.

Sorry, I missed that because of a bug in the 'patches' tool that I
use. The PR is running through CI now.

I've fixed the 'patches' tool to decode emails with
Content-Transfer-Encoding: base64 now:
https://github.com/stefanha/patches/commit/35531a8668f551356c019f68670fcb154535ccd3

Stefan