[kvm-unit-tests PATCH] s390x/intercept: Fix problem with bad compiler warning

[kvm-unit-tests PATCH] s390x/intercept: Fix problem with bad compiler warning

[Thomas Huth]

The intercept test currently can not be compiled with GCC 4.8 anymore.
It generates the following warning (which is fatal due to -Werror):

s390x/intercept.c: In function ‘test_stidp’:
s390x/intercept.c:111:9: error: missing initializer for field ‘version’ of ‘struct cpuid’ [-Werror=missing-field-initializers]
  struct cpuid id = {};
         ^
Fix it by using a "0" as intializer here.

Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 NB: We could also remove the -Wextra from the CFLAGS instead. IMHO
 using -Wextra together with -Werror is just like playing Russian roulette.
 Since -Wextra is some kind of "compiler warning playground" for the GCC
 folks, you never know which compiler version will trigger an unexpected
 (and often also unfounded) warning here, so using this together with -Werror
 is just a nuisance.

 s390x/intercept.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/s390x/intercept.c b/s390x/intercept.c
index 9766289..9fe86cf 100644
--- a/s390x/intercept.c
+++ b/s390x/intercept.c
@@ -108,7 +108,7 @@ static void test_stap(void)
 /* Test the STORE CPU ID instruction */
 static void test_stidp(void)
 {
-	struct cpuid id = {};
+	struct cpuid id = { 0 };
 
 	asm volatile ("stidp %0\n" : "+Q"(id));
 	report("type set", id.type);
-- 
1.8.3.1

Re: [kvm-unit-tests PATCH] s390x/intercept: Fix problem with bad compiler warning

[David Hildenbrand]

On 27.06.2017 06:18, Thomas Huth wrote:
> The intercept test currently can not be compiled with GCC 4.8 anymore.
> It generates the following warning (which is fatal due to -Werror):
> 
> s390x/intercept.c: In function ‘test_stidp’:
> s390x/intercept.c:111:9: error: missing initializer for field ‘version’ of ‘struct cpuid’ [-Werror=missing-field-initializers]
>   struct cpuid id = {};
>          ^
> Fix it by using a "0" as intializer here.
> 
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>  NB: We could also remove the -Wextra from the CFLAGS instead. IMHO
>  using -Wextra together with -Werror is just like playing Russian roulette.
>  Since -Wextra is some kind of "compiler warning playground" for the GCC
>  folks, you never know which compiler version will trigger an unexpected
>  (and often also unfounded) warning here, so using this together with -Werror
>  is just a nuisance.

I agree, this is really not deterministic.

> 
>  s390x/intercept.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/s390x/intercept.c b/s390x/intercept.c
> index 9766289..9fe86cf 100644
> --- a/s390x/intercept.c
> +++ b/s390x/intercept.c
> @@ -108,7 +108,7 @@ static void test_stap(void)
>  /* Test the STORE CPU ID instruction */
>  static void test_stidp(void)
>  {
> -	struct cpuid id = {};
> +	struct cpuid id = { 0 };
>  
>  	asm volatile ("stidp %0\n" : "+Q"(id));
>  	report("type set", id.type);
> 

arm and powerpc also use -Wextra, maybe we should remove this then for all.

Whatever you prefer.

Reviewed-by: David Hildenbrand <david@redhat.com>

-- 

Thanks,

David

Re: [kvm-unit-tests PATCH] s390x/intercept: Fix problem with bad compiler warning

[Thomas Huth]

On 27.06.2017 10:24, David Hildenbrand wrote:
> On 27.06.2017 06:18, Thomas Huth wrote:
>> The intercept test currently can not be compiled with GCC 4.8 anymore.
>> It generates the following warning (which is fatal due to -Werror):
>>
>> s390x/intercept.c: In function ‘test_stidp’:
>> s390x/intercept.c:111:9: error: missing initializer for field ‘version’ of ‘struct cpuid’ [-Werror=missing-field-initializers]
>>   struct cpuid id = {};
>>          ^
>> Fix it by using a "0" as intializer here.
>>
>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>> ---
>>  NB: We could also remove the -Wextra from the CFLAGS instead. IMHO
>>  using -Wextra together with -Werror is just like playing Russian roulette.
>>  Since -Wextra is some kind of "compiler warning playground" for the GCC
>>  folks, you never know which compiler version will trigger an unexpected
>>  (and often also unfounded) warning here, so using this together with -Werror
>>  is just a nuisance.
> 
> I agree, this is really not deterministic.
> 
>>
>>  s390x/intercept.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/s390x/intercept.c b/s390x/intercept.c
>> index 9766289..9fe86cf 100644
>> --- a/s390x/intercept.c
>> +++ b/s390x/intercept.c
>> @@ -108,7 +108,7 @@ static void test_stap(void)
>>  /* Test the STORE CPU ID instruction */
>>  static void test_stidp(void)
>>  {
>> -	struct cpuid id = {};
>> +	struct cpuid id = { 0 };
>>  
>>  	asm volatile ("stidp %0\n" : "+Q"(id));
>>  	report("type set", id.type);
>>
> 
> arm and powerpc also use -Wextra, maybe we should remove this then for all.
> 
> Whatever you prefer.

True ... maybe Drew and Laurent can also comment on whether they like
-Wextra or not ... if we all agree, then we can remove it, otherwise
let's try to go with this patch first (in the hope that we won't hit the
next problem too soon).

> Reviewed-by: David Hildenbrand <david@redhat.com>

Thanks!

 Thomas

Re: [kvm-unit-tests PATCH] s390x/intercept: Fix problem with bad compiler warning

[Laurent Vivier]

On 27/06/2017 10:33, Thomas Huth wrote:
> On 27.06.2017 10:24, David Hildenbrand wrote:
>> On 27.06.2017 06:18, Thomas Huth wrote:
>>> The intercept test currently can not be compiled with GCC 4.8 anymore.
>>> It generates the following warning (which is fatal due to -Werror):
>>>
>>> s390x/intercept.c: In function ‘test_stidp’:
>>> s390x/intercept.c:111:9: error: missing initializer for field ‘version’ of ‘struct cpuid’ [-Werror=missing-field-initializers]
>>>   struct cpuid id = {};
>>>          ^
>>> Fix it by using a "0" as intializer here.
>>>
>>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>>> ---
>>>  NB: We could also remove the -Wextra from the CFLAGS instead. IMHO
>>>  using -Wextra together with -Werror is just like playing Russian roulette.
>>>  Since -Wextra is some kind of "compiler warning playground" for the GCC
>>>  folks, you never know which compiler version will trigger an unexpected
>>>  (and often also unfounded) warning here, so using this together with -Werror
>>>  is just a nuisance.
>>
>> I agree, this is really not deterministic.
>>
>>>
>>>  s390x/intercept.c | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/s390x/intercept.c b/s390x/intercept.c
>>> index 9766289..9fe86cf 100644
>>> --- a/s390x/intercept.c
>>> +++ b/s390x/intercept.c
>>> @@ -108,7 +108,7 @@ static void test_stap(void)
>>>  /* Test the STORE CPU ID instruction */
>>>  static void test_stidp(void)
>>>  {
>>> -	struct cpuid id = {};
>>> +	struct cpuid id = { 0 };
>>>  
>>>  	asm volatile ("stidp %0\n" : "+Q"(id));
>>>  	report("type set", id.type);
>>>
>>
>> arm and powerpc also use -Wextra, maybe we should remove this then for all.
>>
>> Whatever you prefer.
> 
> True ... maybe Drew and Laurent can also comment on whether they like
> -Wextra or not ... if we all agree, then we can remove it, otherwise
> let's try to go with this patch first (in the hope that we won't hit the
> next problem too soon).

I like -Wextra :)

My opinion is more checking we have, less error we have.
The extra cost of adding some initializers is nothing.
But we can consider this as details, so do as you want...

Thanks,
Laurent

Re: [kvm-unit-tests PATCH] s390x/intercept: Fix problem with bad compiler warning

[Thomas Huth]

On 27.06.2017 10:53, Laurent Vivier wrote:
> On 27/06/2017 10:33, Thomas Huth wrote:
>> On 27.06.2017 10:24, David Hildenbrand wrote:
>>> On 27.06.2017 06:18, Thomas Huth wrote:
>>>> The intercept test currently can not be compiled with GCC 4.8 anymore.
>>>> It generates the following warning (which is fatal due to -Werror):
>>>>
>>>> s390x/intercept.c: In function ‘test_stidp’:
>>>> s390x/intercept.c:111:9: error: missing initializer for field ‘version’ of ‘struct cpuid’ [-Werror=missing-field-initializers]
>>>>   struct cpuid id = {};
>>>>          ^
>>>> Fix it by using a "0" as intializer here.
>>>>
>>>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>>>> ---
>>>>  NB: We could also remove the -Wextra from the CFLAGS instead. IMHO
>>>>  using -Wextra together with -Werror is just like playing Russian roulette.
>>>>  Since -Wextra is some kind of "compiler warning playground" for the GCC
>>>>  folks, you never know which compiler version will trigger an unexpected
>>>>  (and often also unfounded) warning here, so using this together with -Werror
>>>>  is just a nuisance.
>>>
>>> I agree, this is really not deterministic.
>>>
>>>>
>>>>  s390x/intercept.c | 2 +-
>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/s390x/intercept.c b/s390x/intercept.c
>>>> index 9766289..9fe86cf 100644
>>>> --- a/s390x/intercept.c
>>>> +++ b/s390x/intercept.c
>>>> @@ -108,7 +108,7 @@ static void test_stap(void)
>>>>  /* Test the STORE CPU ID instruction */
>>>>  static void test_stidp(void)
>>>>  {
>>>> -	struct cpuid id = {};
>>>> +	struct cpuid id = { 0 };
>>>>  
>>>>  	asm volatile ("stidp %0\n" : "+Q"(id));
>>>>  	report("type set", id.type);
>>>>
>>>
>>> arm and powerpc also use -Wextra, maybe we should remove this then for all.
>>>
>>> Whatever you prefer.
>>
>> True ... maybe Drew and Laurent can also comment on whether they like
>> -Wextra or not ... if we all agree, then we can remove it, otherwise
>> let's try to go with this patch first (in the hope that we won't hit the
>> next problem too soon).
> 
> I like -Wextra :)
> 
> My opinion is more checking we have, less error we have.

The problem is that -Wextra often produces unfounded or even wrong
warnings, like in this case. I'd prefer to add the individual parameters
that rather always make sense instead, like -Wtype-limits for example.

 Thomas

Re: [kvm-unit-tests PATCH] s390x/intercept: Fix problem with bad compiler warning

[Laurent Vivier]

On 27/06/2017 11:03, Thomas Huth wrote:
> On 27.06.2017 10:53, Laurent Vivier wrote:
>> On 27/06/2017 10:33, Thomas Huth wrote:
>>> On 27.06.2017 10:24, David Hildenbrand wrote:
>>>> On 27.06.2017 06:18, Thomas Huth wrote:
>>>>> The intercept test currently can not be compiled with GCC 4.8 anymore.
>>>>> It generates the following warning (which is fatal due to -Werror):
>>>>>
>>>>> s390x/intercept.c: In function ‘test_stidp’:
>>>>> s390x/intercept.c:111:9: error: missing initializer for field ‘version’ of ‘struct cpuid’ [-Werror=missing-field-initializers]
>>>>>   struct cpuid id = {};
>>>>>          ^
>>>>> Fix it by using a "0" as intializer here.
>>>>>
>>>>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>>>>> ---
>>>>>  NB: We could also remove the -Wextra from the CFLAGS instead. IMHO
>>>>>  using -Wextra together with -Werror is just like playing Russian roulette.
>>>>>  Since -Wextra is some kind of "compiler warning playground" for the GCC
>>>>>  folks, you never know which compiler version will trigger an unexpected
>>>>>  (and often also unfounded) warning here, so using this together with -Werror
>>>>>  is just a nuisance.
>>>>
>>>> I agree, this is really not deterministic.
>>>>
>>>>>
>>>>>  s390x/intercept.c | 2 +-
>>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/s390x/intercept.c b/s390x/intercept.c
>>>>> index 9766289..9fe86cf 100644
>>>>> --- a/s390x/intercept.c
>>>>> +++ b/s390x/intercept.c
>>>>> @@ -108,7 +108,7 @@ static void test_stap(void)
>>>>>  /* Test the STORE CPU ID instruction */
>>>>>  static void test_stidp(void)
>>>>>  {
>>>>> -	struct cpuid id = {};
>>>>> +	struct cpuid id = { 0 };
>>>>>  
>>>>>  	asm volatile ("stidp %0\n" : "+Q"(id));
>>>>>  	report("type set", id.type);
>>>>>
>>>>
>>>> arm and powerpc also use -Wextra, maybe we should remove this then for all.
>>>>
>>>> Whatever you prefer.
>>>
>>> True ... maybe Drew and Laurent can also comment on whether they like
>>> -Wextra or not ... if we all agree, then we can remove it, otherwise
>>> let's try to go with this patch first (in the hope that we won't hit the
>>> next problem too soon).
>>
>> I like -Wextra :)
>>
>> My opinion is more checking we have, less error we have.
> 
> The problem is that -Wextra often produces unfounded or even wrong
> warnings, like in this case. I'd prefer to add the individual parameters
> that rather always make sense instead, like -Wtype-limits for example.

I agree, removing -Wextra and adding individual parameters seems to be a
good solution.

Laurent

Re: [kvm-unit-tests PATCH] s390x/intercept: Fix problem with bad compiler warning

[Andrew Jones]

On Tue, Jun 27, 2017 at 11:28:54AM +0200, Laurent Vivier wrote:
> On 27/06/2017 11:03, Thomas Huth wrote:
> > On 27.06.2017 10:53, Laurent Vivier wrote:
> >> On 27/06/2017 10:33, Thomas Huth wrote:
> >>> On 27.06.2017 10:24, David Hildenbrand wrote:
> >>>> On 27.06.2017 06:18, Thomas Huth wrote:
> >>>>> The intercept test currently can not be compiled with GCC 4.8 anymore.
> >>>>> It generates the following warning (which is fatal due to -Werror):
> >>>>>
> >>>>> s390x/intercept.c: In function ‘test_stidp’:
> >>>>> s390x/intercept.c:111:9: error: missing initializer for field ‘version’ of ‘struct cpuid’ [-Werror=missing-field-initializers]
> >>>>>   struct cpuid id = {};
> >>>>>          ^
> >>>>> Fix it by using a "0" as intializer here.
> >>>>>
> >>>>> Signed-off-by: Thomas Huth <thuth@redhat.com>
> >>>>> ---
> >>>>>  NB: We could also remove the -Wextra from the CFLAGS instead. IMHO
> >>>>>  using -Wextra together with -Werror is just like playing Russian roulette.
> >>>>>  Since -Wextra is some kind of "compiler warning playground" for the GCC
> >>>>>  folks, you never know which compiler version will trigger an unexpected
> >>>>>  (and often also unfounded) warning here, so using this together with -Werror
> >>>>>  is just a nuisance.
> >>>>
> >>>> I agree, this is really not deterministic.
> >>>>
> >>>>>
> >>>>>  s390x/intercept.c | 2 +-
> >>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>>>>
> >>>>> diff --git a/s390x/intercept.c b/s390x/intercept.c
> >>>>> index 9766289..9fe86cf 100644
> >>>>> --- a/s390x/intercept.c
> >>>>> +++ b/s390x/intercept.c
> >>>>> @@ -108,7 +108,7 @@ static void test_stap(void)
> >>>>>  /* Test the STORE CPU ID instruction */
> >>>>>  static void test_stidp(void)
> >>>>>  {
> >>>>> -	struct cpuid id = {};
> >>>>> +	struct cpuid id = { 0 };
> >>>>>  
> >>>>>  	asm volatile ("stidp %0\n" : "+Q"(id));
> >>>>>  	report("type set", id.type);
> >>>>>
> >>>>
> >>>> arm and powerpc also use -Wextra, maybe we should remove this then for all.
> >>>>
> >>>> Whatever you prefer.
> >>>
> >>> True ... maybe Drew and Laurent can also comment on whether they like
> >>> -Wextra or not ... if we all agree, then we can remove it, otherwise
> >>> let's try to go with this patch first (in the hope that we won't hit the
> >>> next problem too soon).
> >>
> >> I like -Wextra :)
> >>
> >> My opinion is more checking we have, less error we have.
> > 
> > The problem is that -Wextra often produces unfounded or even wrong
> > warnings, like in this case. I'd prefer to add the individual parameters
> > that rather always make sense instead, like -Wtype-limits for example.
> 
> I agree, removing -Wextra and adding individual parameters seems to be a
> good solution.

Fine by me. I'll happily review whatever somebody posts :-)

Thanks,
drew

Re: [kvm-unit-tests PATCH] s390x/intercept: Fix problem with bad compiler warning

[Paolo Bonzini]

On 27/06/2017 13:32, Andrew Jones wrote:
>>> The problem is that -Wextra often produces unfounded or even wrong
>>> warnings, like in this case. I'd prefer to add the individual parameters
>>> that rather always make sense instead, like -Wtype-limits for example.
>> I agree, removing -Wextra and adding individual parameters seems to be a
>> good solution.
> Fine by me. I'll happily review whatever somebody posts :-)

And I'll happily apply a patch that just removes -Wextra in the
meanwhile. :)

Paolo