[Qemu-devel] [PATCH] scsi: Address spurious clang warning

[Qemu-devel] [PATCH] scsi: Address spurious clang warning

[John Snow]

Some versions of Clang prior to 6.0 (and some builds of clang after,
such as 6.0.1-2.fc28) fail to recognize { 0 } as a valid initializer
for a struct with subobjects when -Wmissing-braces is enabled.

https://bugs.llvm.org/show_bug.cgi?id=21689 and
https://reviews.llvm.org/rL314499 suggests this should be fixed in 6.0,
but it might not be the case for older versions or downstream versions.

For now, follow the precedent of ebf2a499 and replace the standard { 0 }
with the accepted { } to silence this warning and allow the build to
work under clang 6.0.1-2.fc28, and builds prior to 6.0.

Signed-off-by: John Snow <jsnow@redhat.com>

---

What I am actually less clear on is why this appears to be a problem
only now; since the introduction of { 0 } was in 2.11. It might be
a regression only in the fedora distribution of Clang 6.0.

With apologies to Paolo, who hates these patches.
---
 scsi/qemu-pr-helper.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/scsi/qemu-pr-helper.c b/scsi/qemu-pr-helper.c
index ce40008bfc..e7af637232 100644
--- a/scsi/qemu-pr-helper.c
+++ b/scsi/qemu-pr-helper.c
@@ -236,7 +236,7 @@ static void dm_init(void)
         perror("Cannot open " CONTROL_PATH);
         exit(1);
     }
-    struct dm_ioctl dm = { 0 };
+    struct dm_ioctl dm = { };
     if (!dm_ioctl(DM_VERSION, &dm)) {
         perror("ioctl");
         exit(1);
-- 
2.17.2

Re: [Qemu-devel] [PATCH] scsi: Address spurious clang warning

[Eric Blake]

On 11/27/18 12:49 PM, John Snow wrote:
> Some versions of Clang prior to 6.0 (and some builds of clang after,
> such as 6.0.1-2.fc28) fail to recognize { 0 } as a valid initializer
> for a struct with subobjects when -Wmissing-braces is enabled.
> 
> https://bugs.llvm.org/show_bug.cgi?id=21689 and
> https://reviews.llvm.org/rL314499 suggests this should be fixed in 6.0,
> but it might not be the case for older versions or downstream versions.
> 
> For now, follow the precedent of ebf2a499 and replace the standard { 0 }
> with the accepted { } to silence this warning and allow the build to
> work under clang 6.0.1-2.fc28, and builds prior to 6.0.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> 

Reviewed-by: Eric Blake <eblake@redhat.com>

I'm okay if this goes into -rc3 as a build-fix; I'm also okay if it 
slips to 4.0.

> ---
> 
> What I am actually less clear on is why this appears to be a problem
> only now; since the introduction of { 0 } was in 2.11. It might be
> a regression only in the fedora distribution of Clang 6.0.

Or even a redefinition of struct dm_ioctl in some header where you are 
just now picking up a new struct layout that tickles the Clang issue in 
relation to the previous layout (since it is possible to have two 
structs that are ABI-compatible but where only one of the two has a 
nested substruct).

> +++ b/scsi/qemu-pr-helper.c
> @@ -236,7 +236,7 @@ static void dm_init(void)
>           perror("Cannot open " CONTROL_PATH);
>           exit(1);
>       }
> -    struct dm_ioctl dm = { 0 };
> +    struct dm_ioctl dm = { };

Random thought: would it be worth having "qemu/compiler.h" define a macro:

#if ...broken clang
#define ZERO_INIT {}
#else
#define ZERO_INIT {0}
#endif

and then rewrite all our '= { 0? }' initializers into '= ZERO_INIT'?  Or 
is that aesthetically too ugly?

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [Qemu-devel] [PATCH] scsi: Address spurious clang warning

[Peter Maydell]

On Tue, 27 Nov 2018 at 19:02, Eric Blake <eblake@redhat.com> wrote:
>
> On 11/27/18 12:49 PM, John Snow wrote:
> > Some versions of Clang prior to 6.0 (and some builds of clang after,
> > such as 6.0.1-2.fc28) fail to recognize { 0 } as a valid initializer
> > for a struct with subobjects when -Wmissing-braces is enabled.
> >
> > https://bugs.llvm.org/show_bug.cgi?id=21689 and
> > https://reviews.llvm.org/rL314499 suggests this should be fixed in 6.0,
> > but it might not be the case for older versions or downstream versions.
> >
> > For now, follow the precedent of ebf2a499 and replace the standard { 0 }
> > with the accepted { } to silence this warning and allow the build to
> > work under clang 6.0.1-2.fc28, and builds prior to 6.0.
> >
> > Signed-off-by: John Snow <jsnow@redhat.com>
> >
>
> Reviewed-by: Eric Blake <eblake@redhat.com>
>
> I'm okay if this goes into -rc3 as a build-fix; I'm also okay if it
> slips to 4.0.
>
> > ---
> >
> > What I am actually less clear on is why this appears to be a problem
> > only now; since the introduction of { 0 } was in 2.11. It might be
> > a regression only in the fedora distribution of Clang 6.0.

Upstream clang 6.0 should have this bug fixed (ie it does
not warn about this construct), so it is odd that Fedora's does not.

> Or even a redefinition of struct dm_ioctl in some header where you are
> just now picking up a new struct layout that tickles the Clang issue in
> relation to the previous layout (since it is possible to have two
> structs that are ABI-compatible but where only one of the two has a
> nested substruct).
>
> > +++ b/scsi/qemu-pr-helper.c
> > @@ -236,7 +236,7 @@ static void dm_init(void)
> >           perror("Cannot open " CONTROL_PATH);
> >           exit(1);
> >       }
> > -    struct dm_ioctl dm = { 0 };
> > +    struct dm_ioctl dm = { };
>
> Random thought: would it be worth having "qemu/compiler.h" define a macro:
>
> #if ...broken clang
> #define ZERO_INIT {}
> #else
> #define ZERO_INIT {0}
> #endif
>
> and then rewrite all our '= { 0? }' initializers into '= ZERO_INIT'?  Or
> is that aesthetically too ugly?

I think that's worse than just writing {} everywhere (which
does work on every compiler we need to support, including
older gcc which also used to warn here).

thanks
-- PMM

Re: [Qemu-devel] [PATCH] scsi: Address spurious clang warning

[John Snow]

On 11/27/18 2:02 PM, Eric Blake wrote:
> On 11/27/18 12:49 PM, John Snow wrote:
>> Some versions of Clang prior to 6.0 (and some builds of clang after,
>> such as 6.0.1-2.fc28) fail to recognize { 0 } as a valid initializer
>> for a struct with subobjects when -Wmissing-braces is enabled.
>>
>> https://bugs.llvm.org/show_bug.cgi?id=21689 and
>> https://reviews.llvm.org/rL314499 suggests this should be fixed in 6.0,
>> but it might not be the case for older versions or downstream versions.
>>
>> For now, follow the precedent of ebf2a499 and replace the standard { 0 }
>> with the accepted { } to silence this warning and allow the build to
>> work under clang 6.0.1-2.fc28, and builds prior to 6.0.
>>
>> Signed-off-by: John Snow <jsnow@redhat.com>
>>
> 
> Reviewed-by: Eric Blake <eblake@redhat.com>
> 
> I'm okay if this goes into -rc3 as a build-fix; I'm also okay if it
> slips to 4.0.
> 
>> ---
>>
>> What I am actually less clear on is why this appears to be a problem
>> only now; since the introduction of { 0 } was in 2.11. It might be
>> a regression only in the fedora distribution of Clang 6.0.
> 
> Or even a redefinition of struct dm_ioctl in some header where you are
> just now picking up a new struct layout that tickles the Clang issue in
> relation to the previous layout (since it is possible to have two
> structs that are ABI-compatible but where only one of the two has a
> nested substruct).
> 
>> +++ b/scsi/qemu-pr-helper.c
>> @@ -236,7 +236,7 @@ static void dm_init(void)
>>           perror("Cannot open " CONTROL_PATH);
>>           exit(1);
>>       }
>> -    struct dm_ioctl dm = { 0 };
>> +    struct dm_ioctl dm = { };
> 
> Random thought: would it be worth having "qemu/compiler.h" define a macro:
> 
> #if ...broken clang
> #define ZERO_INIT {}
> #else
> #define ZERO_INIT {0}
> #endif
> 
> and then rewrite all our '= { 0? }' initializers into '= ZERO_INIT'?  Or
> is that aesthetically too ugly?
> 

Obscures perfectly legitimate C code without solving anything, IMO. As
much code as can reflect "naked" C89/C99/GNU99, the better.

--js

Re: [Qemu-devel] [PATCH] scsi: Address spurious clang warning

[Peter Maydell]

On Tue, 27 Nov 2018 at 18:49, John Snow <jsnow@redhat.com> wrote:
>
> Some versions of Clang prior to 6.0 (and some builds of clang after,
> such as 6.0.1-2.fc28) fail to recognize { 0 } as a valid initializer
> for a struct with subobjects when -Wmissing-braces is enabled.
>
> https://bugs.llvm.org/show_bug.cgi?id=21689 and
> https://reviews.llvm.org/rL314499 suggests this should be fixed in 6.0,
> but it might not be the case for older versions or downstream versions.
>
> For now, follow the precedent of ebf2a499 and replace the standard { 0 }
> with the accepted { } to silence this warning and allow the build to
> work under clang 6.0.1-2.fc28, and builds prior to 6.0.
>
> Signed-off-by: John Snow <jsnow@redhat.com>

Applied to master as a build fix for rc3, thanks.

-- PMM