linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH]  amdgpu/gmc : fix compile warning
@ 2018-09-14 10:05 Peng Hao
  2018-10-04 18:52 ` Guenter Roeck
  0 siblings, 1 reply; 18+ messages in thread
From: Peng Hao @ 2018-09-14 10:05 UTC (permalink / raw)
  To: alexander.deucher, christian.koenig, David1.Zhou, airlied
  Cc: amd-gfx, dri-devel, linux-kernel, Peng Hao

drivers/gpu/drm/amd/amdgpu/gmc_v8_0.c:
    In function ‘gmc_v8_0_process_interrupt’:
drivers/gpu/drm/amd/amdgpu/gmc_v8_0.c:1447:10:
    warning: missing braces around initializer [-Wmissing-braces]

Signed-off-by: Peng Hao <peng.hao2@zte.com.cn>
---
 drivers/gpu/drm/amd/amdgpu/gmc_v8_0.c | 2 +-
 drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/gmc_v8_0.c b/drivers/gpu/drm/amd/amdgpu/gmc_v8_0.c
index 9333109..55f4755 100644
--- a/drivers/gpu/drm/amd/amdgpu/gmc_v8_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/gmc_v8_0.c
@@ -1444,7 +1444,7 @@ static int gmc_v8_0_process_interrupt(struct amdgpu_device *adev,
 		gmc_v8_0_set_fault_enable_default(adev, false);
 
 	if (printk_ratelimit()) {
-		struct amdgpu_task_info task_info = { 0 };
+		struct amdgpu_task_info task_info = { {0} };
 
 		amdgpu_vm_get_task_info(adev, entry->pasid, &task_info);
 
diff --git a/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c b/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
index 72f8018..e8b78c5 100644
--- a/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
@@ -259,7 +259,7 @@ static int gmc_v9_0_process_interrupt(struct amdgpu_device *adev,
 	}
 
 	if (printk_ratelimit()) {
-		struct amdgpu_task_info task_info = { 0 };
+		struct amdgpu_task_info task_info = { {0} };
 
 		amdgpu_vm_get_task_info(adev, entry->pasid, &task_info);
 
-- 
1.8.3.1


^ permalink raw reply related	[flat|nested] 18+ messages in thread

* Re: [PATCH]  amdgpu/gmc : fix compile warning
  2018-09-14 10:05 [PATCH] amdgpu/gmc : fix compile warning Peng Hao
@ 2018-10-04 18:52 ` Guenter Roeck
  2018-10-05  8:14   ` Koenig, Christian
  0 siblings, 1 reply; 18+ messages in thread
From: Guenter Roeck @ 2018-10-04 18:52 UTC (permalink / raw)
  To: Peng Hao
  Cc: alexander.deucher, christian.koenig, David1.Zhou, airlied,
	amd-gfx, dri-devel, linux-kernel

Hi,

On Fri, Sep 14, 2018 at 06:05:52PM +0800, Peng Hao wrote:
> drivers/gpu/drm/amd/amdgpu/gmc_v8_0.c:
>     In function ‘gmc_v8_0_process_interrupt’:
> drivers/gpu/drm/amd/amdgpu/gmc_v8_0.c:1447:10:
>     warning: missing braces around initializer [-Wmissing-braces]
> 
> Signed-off-by: Peng Hao <peng.hao2@zte.com.cn>

Was there any feedback on this patch ? The problem does affect us,
and we'll need a fix.

> ---
>  drivers/gpu/drm/amd/amdgpu/gmc_v8_0.c | 2 +-
>  drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/gmc_v8_0.c b/drivers/gpu/drm/amd/amdgpu/gmc_v8_0.c
> index 9333109..55f4755 100644
> --- a/drivers/gpu/drm/amd/amdgpu/gmc_v8_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/gmc_v8_0.c
> @@ -1444,7 +1444,7 @@ static int gmc_v8_0_process_interrupt(struct amdgpu_device *adev,
>  		gmc_v8_0_set_fault_enable_default(adev, false);
>  
>  	if (printk_ratelimit()) {
> -		struct amdgpu_task_info task_info = { 0 };
> +		struct amdgpu_task_info task_info = { {0} };
>  

I wondered if
		struct amdgpu_task_info task_info = { };
would be better.

Thanks,
Guenter

>  		amdgpu_vm_get_task_info(adev, entry->pasid, &task_info);
>  
> diff --git a/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c b/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
> index 72f8018..e8b78c5 100644
> --- a/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
> @@ -259,7 +259,7 @@ static int gmc_v9_0_process_interrupt(struct amdgpu_device *adev,
>  	}
>  
>  	if (printk_ratelimit()) {
> -		struct amdgpu_task_info task_info = { 0 };
> +		struct amdgpu_task_info task_info = { {0} };
>  
>  		amdgpu_vm_get_task_info(adev, entry->pasid, &task_info);
>  

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH] amdgpu/gmc : fix compile warning
  2018-10-04 18:52 ` Guenter Roeck
@ 2018-10-05  8:14   ` Koenig, Christian
  2018-10-05  8:38     ` Guenter Roeck
  0 siblings, 1 reply; 18+ messages in thread
From: Koenig, Christian @ 2018-10-05  8:14 UTC (permalink / raw)
  To: Guenter Roeck, Peng Hao
  Cc: Deucher, Alexander, Zhou, David(ChunMing),
	airlied, amd-gfx, dri-devel, linux-kernel

Am 04.10.2018 um 20:52 schrieb Guenter Roeck:
> Hi,
>
> On Fri, Sep 14, 2018 at 06:05:52PM +0800, Peng Hao wrote:
>> drivers/gpu/drm/amd/amdgpu/gmc_v8_0.c:
>>      In function ‘gmc_v8_0_process_interrupt’:
>> drivers/gpu/drm/amd/amdgpu/gmc_v8_0.c:1447:10:
>>      warning: missing braces around initializer [-Wmissing-braces]
>>
>> Signed-off-by: Peng Hao <peng.hao2@zte.com.cn>
> Was there any feedback on this patch ? The problem does affect us,
> and we'll need a fix.

Well as discussed using "{ { 0 } }" is as wrong as using "{ 0 }".

We should either use only "{ }" or even better make nails with heads and 
use memset().

Christian.

>
>> ---
>>   drivers/gpu/drm/amd/amdgpu/gmc_v8_0.c | 2 +-
>>   drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c | 2 +-
>>   2 files changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/gmc_v8_0.c b/drivers/gpu/drm/amd/amdgpu/gmc_v8_0.c
>> index 9333109..55f4755 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/gmc_v8_0.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/gmc_v8_0.c
>> @@ -1444,7 +1444,7 @@ static int gmc_v8_0_process_interrupt(struct amdgpu_device *adev,
>>   		gmc_v8_0_set_fault_enable_default(adev, false);
>>   
>>   	if (printk_ratelimit()) {
>> -		struct amdgpu_task_info task_info = { 0 };
>> +		struct amdgpu_task_info task_info = { {0} };
>>   
> I wondered if
> 		struct amdgpu_task_info task_info = { };
> would be better.
>
> Thanks,
> Guenter
>
>>   		amdgpu_vm_get_task_info(adev, entry->pasid, &task_info);
>>   
>> diff --git a/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c b/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
>> index 72f8018..e8b78c5 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
>> @@ -259,7 +259,7 @@ static int gmc_v9_0_process_interrupt(struct amdgpu_device *adev,
>>   	}
>>   
>>   	if (printk_ratelimit()) {
>> -		struct amdgpu_task_info task_info = { 0 };
>> +		struct amdgpu_task_info task_info = { {0} };
>>   
>>   		amdgpu_vm_get_task_info(adev, entry->pasid, &task_info);
>>   


^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH] amdgpu/gmc : fix compile warning
  2018-10-05  8:14   ` Koenig, Christian
@ 2018-10-05  8:38     ` Guenter Roeck
  2018-10-08  8:00       ` Christian König
  0 siblings, 1 reply; 18+ messages in thread
From: Guenter Roeck @ 2018-10-05  8:38 UTC (permalink / raw)
  To: Koenig, Christian, Peng Hao
  Cc: Deucher, Alexander, Zhou, David(ChunMing),
	airlied, amd-gfx, dri-devel, linux-kernel

On 10/05/2018 01:14 AM, Koenig, Christian wrote:
> Am 04.10.2018 um 20:52 schrieb Guenter Roeck:
>> Hi,
>>
>> On Fri, Sep 14, 2018 at 06:05:52PM +0800, Peng Hao wrote:
>>> drivers/gpu/drm/amd/amdgpu/gmc_v8_0.c:
>>>       In function ‘gmc_v8_0_process_interrupt’:
>>> drivers/gpu/drm/amd/amdgpu/gmc_v8_0.c:1447:10:
>>>       warning: missing braces around initializer [-Wmissing-braces]
>>>
>>> Signed-off-by: Peng Hao <peng.hao2@zte.com.cn>
>> Was there any feedback on this patch ? The problem does affect us,
>> and we'll need a fix.
> 
> Well as discussed using "{ { 0 } }" is as wrong as using "{ 0 }".
> 

Ah, sorry, I must have missed the discussion.

It is for sure not the best solution, but at least it compiles, and it seems
to be proliferating.

$ git grep "{ *{ *0 *} *}" | wc
     144    1180   11802
$ git grep "{ *{ *0 *} *}" drivers/gpu/drm/amd/ | wc
      50     459    5239

> We should either use only "{ }" or even better make nails with heads and
> use memset().

I'd rather leave it up to the compiler to decide what is most efficient.

Guenter

> 
> Christian.
> 
>>
>>> ---
>>>    drivers/gpu/drm/amd/amdgpu/gmc_v8_0.c | 2 +-
>>>    drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c | 2 +-
>>>    2 files changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/gmc_v8_0.c b/drivers/gpu/drm/amd/amdgpu/gmc_v8_0.c
>>> index 9333109..55f4755 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/gmc_v8_0.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/gmc_v8_0.c
>>> @@ -1444,7 +1444,7 @@ static int gmc_v8_0_process_interrupt(struct amdgpu_device *adev,
>>>    		gmc_v8_0_set_fault_enable_default(adev, false);
>>>    
>>>    	if (printk_ratelimit()) {
>>> -		struct amdgpu_task_info task_info = { 0 };
>>> +		struct amdgpu_task_info task_info = { {0} };
>>>    
>> I wondered if
>> 		struct amdgpu_task_info task_info = { };
>> would be better.
>>
>> Thanks,
>> Guenter
>>
>>>    		amdgpu_vm_get_task_info(adev, entry->pasid, &task_info);
>>>    
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c b/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
>>> index 72f8018..e8b78c5 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
>>> @@ -259,7 +259,7 @@ static int gmc_v9_0_process_interrupt(struct amdgpu_device *adev,
>>>    	}
>>>    
>>>    	if (printk_ratelimit()) {
>>> -		struct amdgpu_task_info task_info = { 0 };
>>> +		struct amdgpu_task_info task_info = { {0} };
>>>    
>>>    		amdgpu_vm_get_task_info(adev, entry->pasid, &task_info);
>>>    
> 


^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH] amdgpu/gmc : fix compile warning
  2018-10-05  8:38     ` Guenter Roeck
@ 2018-10-08  8:00       ` Christian König
  2018-10-08 13:33         ` Guenter Roeck
  0 siblings, 1 reply; 18+ messages in thread
From: Christian König @ 2018-10-08  8:00 UTC (permalink / raw)
  To: Guenter Roeck, Koenig, Christian, Peng Hao
  Cc: airlied, linux-kernel, dri-devel, amd-gfx, Deucher, Alexander

Am 05.10.2018 um 10:38 schrieb Guenter Roeck:
> On 10/05/2018 01:14 AM, Koenig, Christian wrote:
>> Am 04.10.2018 um 20:52 schrieb Guenter Roeck:
>>> Hi,
>>>
>>> On Fri, Sep 14, 2018 at 06:05:52PM +0800, Peng Hao wrote:
>>>> drivers/gpu/drm/amd/amdgpu/gmc_v8_0.c:
>>>>       In function ‘gmc_v8_0_process_interrupt’:
>>>> drivers/gpu/drm/amd/amdgpu/gmc_v8_0.c:1447:10:
>>>>       warning: missing braces around initializer [-Wmissing-braces]
>>>>
>>>> Signed-off-by: Peng Hao <peng.hao2@zte.com.cn>
>>> Was there any feedback on this patch ? The problem does affect us,
>>> and we'll need a fix.
>>
>> Well as discussed using "{ { 0 } }" is as wrong as using "{ 0 }".
>>
>
> Ah, sorry, I must have missed the discussion.
>
> It is for sure not the best solution, but at least it compiles, and it 
> seems
> to be proliferating.

Yeah, and exactly that's the problem. As the discussion showed "{ { 0 } 
}" is buggy because it tells the compiler to only initialize the first 
member of the structure, but not all of it.

That is incorrect and rather dangerous cause it can lead to unforeseen 
results and should probably trigger a warning.

>
> $ git grep "{ *{ *0 *} *}" | wc
>     144    1180   11802
> $ git grep "{ *{ *0 *} *}" drivers/gpu/drm/amd/ | wc
>      50     459    5239
>
>> We should either use only "{ }" or even better make nails with heads and
>> use memset().
>
> I'd rather leave it up to the compiler to decide what is most efficient.

And I would rather prefer to have a working driver :)

Christian.

>
> Guenter
>
>>
>> Christian.
>>
>>>
>>>> ---
>>>>    drivers/gpu/drm/amd/amdgpu/gmc_v8_0.c | 2 +-
>>>>    drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c | 2 +-
>>>>    2 files changed, 2 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/gmc_v8_0.c 
>>>> b/drivers/gpu/drm/amd/amdgpu/gmc_v8_0.c
>>>> index 9333109..55f4755 100644
>>>> --- a/drivers/gpu/drm/amd/amdgpu/gmc_v8_0.c
>>>> +++ b/drivers/gpu/drm/amd/amdgpu/gmc_v8_0.c
>>>> @@ -1444,7 +1444,7 @@ static int gmc_v8_0_process_interrupt(struct 
>>>> amdgpu_device *adev,
>>>>            gmc_v8_0_set_fault_enable_default(adev, false);
>>>>           if (printk_ratelimit()) {
>>>> -        struct amdgpu_task_info task_info = { 0 };
>>>> +        struct amdgpu_task_info task_info = { {0} };
>>> I wondered if
>>>         struct amdgpu_task_info task_info = { };
>>> would be better.
>>>
>>> Thanks,
>>> Guenter
>>>
>>>> amdgpu_vm_get_task_info(adev, entry->pasid, &task_info);
>>>>    diff --git a/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c 
>>>> b/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
>>>> index 72f8018..e8b78c5 100644
>>>> --- a/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
>>>> +++ b/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
>>>> @@ -259,7 +259,7 @@ static int gmc_v9_0_process_interrupt(struct 
>>>> amdgpu_device *adev,
>>>>        }
>>>>           if (printk_ratelimit()) {
>>>> -        struct amdgpu_task_info task_info = { 0 };
>>>> +        struct amdgpu_task_info task_info = { {0} };
>>>>               amdgpu_vm_get_task_info(adev, entry->pasid, &task_info);
>>
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel


^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH] amdgpu/gmc : fix compile warning
  2018-10-08  8:00       ` Christian König
@ 2018-10-08 13:33         ` Guenter Roeck
  2018-10-08 13:47           ` Koenig, Christian
  0 siblings, 1 reply; 18+ messages in thread
From: Guenter Roeck @ 2018-10-08 13:33 UTC (permalink / raw)
  To: christian.koenig, Peng Hao
  Cc: airlied, linux-kernel, dri-devel, amd-gfx, Deucher, Alexander

On 10/08/2018 01:00 AM, Christian König wrote:
> Am 05.10.2018 um 10:38 schrieb Guenter Roeck:
>> On 10/05/2018 01:14 AM, Koenig, Christian wrote:
>>> Am 04.10.2018 um 20:52 schrieb Guenter Roeck:
>>>> Hi,
>>>>
>>>> On Fri, Sep 14, 2018 at 06:05:52PM +0800, Peng Hao wrote:
>>>>> drivers/gpu/drm/amd/amdgpu/gmc_v8_0.c:
>>>>>       In function ‘gmc_v8_0_process_interrupt’:
>>>>> drivers/gpu/drm/amd/amdgpu/gmc_v8_0.c:1447:10:
>>>>>       warning: missing braces around initializer [-Wmissing-braces]
>>>>>
>>>>> Signed-off-by: Peng Hao <peng.hao2@zte.com.cn>
>>>> Was there any feedback on this patch ? The problem does affect us,
>>>> and we'll need a fix.
>>>
>>> Well as discussed using "{ { 0 } }" is as wrong as using "{ 0 }".
>>>
>>
>> Ah, sorry, I must have missed the discussion.
>>
>> It is for sure not the best solution, but at least it compiles, and it seems
>> to be proliferating.
> 
> Yeah, and exactly that's the problem. As the discussion showed "{ { 0 } }" is buggy because it tells the compiler to only initialize the first member of the structure, but not all of it.
> 
> That is incorrect and rather dangerous cause it can lead to unforeseen results and should probably trigger a warning.
> 
>>
>> $ git grep "{ *{ *0 *} *}" | wc
>>     144    1180   11802
>> $ git grep "{ *{ *0 *} *}" drivers/gpu/drm/amd/ | wc
>>      50     459    5239
>>
>>> We should either use only "{ }" or even better make nails with heads and
>>> use memset().
>>
>> I'd rather leave it up to the compiler to decide what is most efficient.
> 
> And I would rather prefer to have a working driver :)
> 

So { } isn't correct either ?

One thing I found missing in the discussion was the reference to the C standard.
The C99 standard states in section 6.7.8 (Initialization) clause 19: "... all
subobjects that are not initialized explicitly shall be initialized implicitly
the same as objects that have static storage duration". Clause 21 makes further
reference to partial initialization, suggesting the same. Various online
resources, including the gcc documentation, all state the same. I don't find
any reference to a partial initialization which would leave members of a structure
undefined. It would be interesting for me to understand how and why this does
not apply here.

In this context, it is interesting that the other 48 instances of the
{ { 0 } } initialization in the same driver don't raise similar concerns,
nor seemed to have caused any operational problems.

Anyway, I fixed up the code in our tree (with { }), so I'll leave it
up to you folks to decide what if anything to do about it.

Thanks,
Guenter

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH] amdgpu/gmc : fix compile warning
  2018-10-08 13:33         ` Guenter Roeck
@ 2018-10-08 13:47           ` Koenig, Christian
  2018-10-08 14:10             ` Guenter Roeck
  0 siblings, 1 reply; 18+ messages in thread
From: Koenig, Christian @ 2018-10-08 13:47 UTC (permalink / raw)
  To: Guenter Roeck, Peng Hao
  Cc: airlied, linux-kernel, dri-devel, amd-gfx, Deucher, Alexander

Am 08.10.2018 um 15:33 schrieb Guenter Roeck:
> On 10/08/2018 01:00 AM, Christian König wrote:
>> Am 05.10.2018 um 10:38 schrieb Guenter Roeck:
>>> On 10/05/2018 01:14 AM, Koenig, Christian wrote:
>>>> Am 04.10.2018 um 20:52 schrieb Guenter Roeck:
>>>>> Hi,
>>>>>
>>>>> On Fri, Sep 14, 2018 at 06:05:52PM +0800, Peng Hao wrote:
>>>>>> drivers/gpu/drm/amd/amdgpu/gmc_v8_0.c:
>>>>>>       In function ‘gmc_v8_0_process_interrupt’:
>>>>>> drivers/gpu/drm/amd/amdgpu/gmc_v8_0.c:1447:10:
>>>>>>       warning: missing braces around initializer [-Wmissing-braces]
>>>>>>
>>>>>> Signed-off-by: Peng Hao <peng.hao2@zte.com.cn>
>>>>> Was there any feedback on this patch ? The problem does affect us,
>>>>> and we'll need a fix.
>>>>
>>>> Well as discussed using "{ { 0 } }" is as wrong as using "{ 0 }".
>>>>
>>>
>>> Ah, sorry, I must have missed the discussion.
>>>
>>> It is for sure not the best solution, but at least it compiles, and 
>>> it seems
>>> to be proliferating.
>>
>> Yeah, and exactly that's the problem. As the discussion showed "{ { 0 
>> } }" is buggy because it tells the compiler to only initialize the 
>> first member of the structure, but not all of it.
>>
>> That is incorrect and rather dangerous cause it can lead to 
>> unforeseen results and should probably trigger a warning.
>>
>>>
>>> $ git grep "{ *{ *0 *} *}" | wc
>>>     144    1180   11802
>>> $ git grep "{ *{ *0 *} *}" drivers/gpu/drm/amd/ | wc
>>>      50     459    5239
>>>
>>>> We should either use only "{ }" or even better make nails with 
>>>> heads and
>>>> use memset().
>>>
>>> I'd rather leave it up to the compiler to decide what is most 
>>> efficient.
>>
>> And I would rather prefer to have a working driver :)
>>
>
> So { } isn't correct either ?

Yes, initializing structures with { } is known to be problematic as well.

It doesn't necessary initialize all bytes when you have padding causing 
random failures when structures are memcmp().

>
> One thing I found missing in the discussion was the reference to the C 
> standard.
> The C99 standard states in section 6.7.8 (Initialization) clause 19: 
> "... all
> subobjects that are not initialized explicitly shall be initialized 
> implicitly
> the same as objects that have static storage duration". Clause 21 
> makes further
> reference to partial initialization, suggesting the same. Various online
> resources, including the gcc documentation, all state the same. I 
> don't find
> any reference to a partial initialization which would leave members of 
> a structure
> undefined. It would be interesting for me to understand how and why 
> this does
> not apply here.
>
> In this context, it is interesting that the other 48 instances of the
> { { 0 } } initialization in the same driver don't raise similar concerns,
> nor seemed to have caused any operational problems.

Feel free to provide patches to replace those with memset().

>
> Anyway, I fixed up the code in our tree (with { }), so I'll leave it
> up to you folks to decide what if anything to do about it.

Well considering the known problems with {} initialization I'm certainly 
rejecting all patches which turns memset() into {}.

Regards,
Christian.

>
> Thanks,
> Guenter


^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH] amdgpu/gmc : fix compile warning
  2018-10-08 13:47           ` Koenig, Christian
@ 2018-10-08 14:10             ` Guenter Roeck
  2018-10-08 15:57               ` Deucher, Alexander
  0 siblings, 1 reply; 18+ messages in thread
From: Guenter Roeck @ 2018-10-08 14:10 UTC (permalink / raw)
  To: Koenig, Christian, Peng Hao
  Cc: airlied, linux-kernel, dri-devel, amd-gfx, Deucher, Alexander

On 10/08/2018 06:47 AM, Koenig, Christian wrote:
> Am 08.10.2018 um 15:33 schrieb Guenter Roeck:
>> On 10/08/2018 01:00 AM, Christian König wrote:
>>> Am 05.10.2018 um 10:38 schrieb Guenter Roeck:
>>>> On 10/05/2018 01:14 AM, Koenig, Christian wrote:
>>>>> Am 04.10.2018 um 20:52 schrieb Guenter Roeck:
>>>>>> Hi,
>>>>>>
>>>>>> On Fri, Sep 14, 2018 at 06:05:52PM +0800, Peng Hao wrote:
>>>>>>> drivers/gpu/drm/amd/amdgpu/gmc_v8_0.c:
>>>>>>>        In function ‘gmc_v8_0_process_interrupt’:
>>>>>>> drivers/gpu/drm/amd/amdgpu/gmc_v8_0.c:1447:10:
>>>>>>>        warning: missing braces around initializer [-Wmissing-braces]
>>>>>>>
>>>>>>> Signed-off-by: Peng Hao <peng.hao2@zte.com.cn>
>>>>>> Was there any feedback on this patch ? The problem does affect us,
>>>>>> and we'll need a fix.
>>>>>
>>>>> Well as discussed using "{ { 0 } }" is as wrong as using "{ 0 }".
>>>>>
>>>>
>>>> Ah, sorry, I must have missed the discussion.
>>>>
>>>> It is for sure not the best solution, but at least it compiles, and
>>>> it seems
>>>> to be proliferating.
>>>
>>> Yeah, and exactly that's the problem. As the discussion showed "{ { 0
>>> } }" is buggy because it tells the compiler to only initialize the
>>> first member of the structure, but not all of it.
>>>
>>> That is incorrect and rather dangerous cause it can lead to
>>> unforeseen results and should probably trigger a warning.
>>>
>>>>
>>>> $ git grep "{ *{ *0 *} *}" | wc
>>>>      144    1180   11802
>>>> $ git grep "{ *{ *0 *} *}" drivers/gpu/drm/amd/ | wc
>>>>       50     459    5239
>>>>
>>>>> We should either use only "{ }" or even better make nails with
>>>>> heads and
>>>>> use memset().
>>>>
>>>> I'd rather leave it up to the compiler to decide what is most
>>>> efficient.
>>>
>>> And I would rather prefer to have a working driver :)
>>>
>>
>> So { } isn't correct either ?
> 
> Yes, initializing structures with { } is known to be problematic as well.
> 
> It doesn't necessary initialize all bytes when you have padding causing
> random failures when structures are memcmp().
> 
>>
>> One thing I found missing in the discussion was the reference to the C
>> standard.
>> The C99 standard states in section 6.7.8 (Initialization) clause 19:
>> "... all
>> subobjects that are not initialized explicitly shall be initialized
>> implicitly
>> the same as objects that have static storage duration". Clause 21
>> makes further
>> reference to partial initialization, suggesting the same. Various online
>> resources, including the gcc documentation, all state the same. I
>> don't find
>> any reference to a partial initialization which would leave members of
>> a structure
>> undefined. It would be interesting for me to understand how and why
>> this does
>> not apply here.
>>
>> In this context, it is interesting that the other 48 instances of the
>> { { 0 } } initialization in the same driver don't raise similar concerns,
>> nor seemed to have caused any operational problems.
> 
> Feel free to provide patches to replace those with memset().
> 

Not me. As I see it, the problem, if it exists, would be a violation of the
C standard. I don't believe hacking around bad C compilers. I would rather
blacklist such compilers.

>>
>> Anyway, I fixed up the code in our tree (with { }), so I'll leave it
>> up to you folks to decide what if anything to do about it.
> 
> Well considering the known problems with {} initialization I'm certainly
> rejecting all patches which turns memset() into {}.
> 

Please point me to specific instances of this problem.

Thanks,
Guenter

^ permalink raw reply	[flat|nested] 18+ messages in thread

* RE: [PATCH] amdgpu/gmc : fix compile warning
  2018-10-08 14:10             ` Guenter Roeck
@ 2018-10-08 15:57               ` Deucher, Alexander
  2018-10-08 17:22                 ` Koenig, Christian
  2018-10-08 17:41                 ` Guenter Roeck
  0 siblings, 2 replies; 18+ messages in thread
From: Deucher, Alexander @ 2018-10-08 15:57 UTC (permalink / raw)
  To: Guenter Roeck, Koenig, Christian, Peng Hao
  Cc: airlied, linux-kernel, dri-devel, amd-gfx

> -----Original Message-----
> From: Guenter Roeck <groeck7@gmail.com> On Behalf Of Guenter Roeck
> Sent: Monday, October 8, 2018 10:11 AM
> To: Koenig, Christian <Christian.Koenig@amd.com>; Peng Hao
> <peng.hao2@zte.com.cn>
> Cc: airlied@linux.ie; linux-kernel@vger.kernel.org; dri-
> devel@lists.freedesktop.org; amd-gfx@lists.freedesktop.org; Deucher,
> Alexander <Alexander.Deucher@amd.com>
> Subject: Re: [PATCH] amdgpu/gmc : fix compile warning
> 
> On 10/08/2018 06:47 AM, Koenig, Christian wrote:
> > Am 08.10.2018 um 15:33 schrieb Guenter Roeck:
> >> On 10/08/2018 01:00 AM, Christian König wrote:
> >>> Am 05.10.2018 um 10:38 schrieb Guenter Roeck:
> >>>> On 10/05/2018 01:14 AM, Koenig, Christian wrote:
> >>>>> Am 04.10.2018 um 20:52 schrieb Guenter Roeck:
> >>>>>> Hi,
> >>>>>>
> >>>>>> On Fri, Sep 14, 2018 at 06:05:52PM +0800, Peng Hao wrote:
> >>>>>>> drivers/gpu/drm/amd/amdgpu/gmc_v8_0.c:
> >>>>>>>        In function ‘gmc_v8_0_process_interrupt’:
> >>>>>>> drivers/gpu/drm/amd/amdgpu/gmc_v8_0.c:1447:10:
> >>>>>>>        warning: missing braces around initializer
> >>>>>>> [-Wmissing-braces]
> >>>>>>>
> >>>>>>> Signed-off-by: Peng Hao <peng.hao2@zte.com.cn>
> >>>>>> Was there any feedback on this patch ? The problem does affect
> >>>>>> us, and we'll need a fix.
> >>>>>
> >>>>> Well as discussed using "{ { 0 } }" is as wrong as using "{ 0 }".
> >>>>>
> >>>>
> >>>> Ah, sorry, I must have missed the discussion.
> >>>>
> >>>> It is for sure not the best solution, but at least it compiles, and
> >>>> it seems to be proliferating.
> >>>
> >>> Yeah, and exactly that's the problem. As the discussion showed "{ {
> >>> 0 } }" is buggy because it tells the compiler to only initialize the
> >>> first member of the structure, but not all of it.
> >>>
> >>> That is incorrect and rather dangerous cause it can lead to
> >>> unforeseen results and should probably trigger a warning.
> >>>
> >>>>
> >>>> $ git grep "{ *{ *0 *} *}" | wc
> >>>>      144    1180   11802
> >>>> $ git grep "{ *{ *0 *} *}" drivers/gpu/drm/amd/ | wc
> >>>>       50     459    5239
> >>>>
> >>>>> We should either use only "{ }" or even better make nails with
> >>>>> heads and use memset().
> >>>>
> >>>> I'd rather leave it up to the compiler to decide what is most
> >>>> efficient.
> >>>
> >>> And I would rather prefer to have a working driver :)
> >>>
> >>
> >> So { } isn't correct either ?
> >
> > Yes, initializing structures with { } is known to be problematic as well.
> >
> > It doesn't necessary initialize all bytes when you have padding
> > causing random failures when structures are memcmp().
> >
> >>
> >> One thing I found missing in the discussion was the reference to the
> >> C standard.
> >> The C99 standard states in section 6.7.8 (Initialization) clause 19:
> >> "... all
> >> subobjects that are not initialized explicitly shall be initialized
> >> implicitly the same as objects that have static storage duration".
> >> Clause 21 makes further reference to partial initialization,
> >> suggesting the same. Various online resources, including the gcc
> >> documentation, all state the same. I don't find any reference to a
> >> partial initialization which would leave members of a structure
> >> undefined. It would be interesting for me to understand how and why
> >> this does not apply here.
> >>
> >> In this context, it is interesting that the other 48 instances of the
> >> { { 0 } } initialization in the same driver don't raise similar
> >> concerns, nor seemed to have caused any operational problems.
> >
> > Feel free to provide patches to replace those with memset().
> >
> 
> Not me. As I see it, the problem, if it exists, would be a violation of the C
> standard. I don't believe hacking around bad C compilers. I would rather
> blacklist such compilers.
> 
> >>
> >> Anyway, I fixed up the code in our tree (with { }), so I'll leave it
> >> up to you folks to decide what if anything to do about it.
> >
> > Well considering the known problems with {} initialization I'm
> > certainly rejecting all patches which turns memset() into {}.
> >
> 
> Please point me to specific instances of this problem.

I think there are a number of places in DC (drivers/gpu/drm/amd/display) where we applied the original proposed solution before realizing that it would only initialize the first element.  It would be nice to get them fixed up.

Alex


^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH] amdgpu/gmc : fix compile warning
  2018-10-08 15:57               ` Deucher, Alexander
@ 2018-10-08 17:22                 ` Koenig, Christian
  2018-10-08 17:46                   ` Guenter Roeck
  2018-10-08 17:41                 ` Guenter Roeck
  1 sibling, 1 reply; 18+ messages in thread
From: Koenig, Christian @ 2018-10-08 17:22 UTC (permalink / raw)
  To: Deucher, Alexander, Guenter Roeck, Peng Hao
  Cc: airlied, linux-kernel, dri-devel, amd-gfx

Am 08.10.2018 um 17:57 schrieb Deucher, Alexander:
>>>> One thing I found missing in the discussion was the reference to the
>>>> C standard.
>>>> The C99 standard states in section 6.7.8 (Initialization) clause 19:
>>>> "... all
>>>> subobjects that are not initialized explicitly shall be initialized
>>>> implicitly the same as objects that have static storage duration".
>>>> Clause 21 makes further reference to partial initialization,
>>>> suggesting the same. Various online resources, including the gcc
>>>> documentation, all state the same. I don't find any reference to a
>>>> partial initialization which would leave members of a structure
>>>> undefined. It would be interesting for me to understand how and why
>>>> this does not apply here.
>>>>
>>>> In this context, it is interesting that the other 48 instances of the
>>>> { { 0 } } initialization in the same driver don't raise similar
>>>> concerns, nor seemed to have caused any operational problems.
>>> Feel free to provide patches to replace those with memset().
>>>
>> Not me. As I see it, the problem, if it exists, would be a violation of the C
>> standard. I don't believe hacking around bad C compilers. I would rather
>> blacklist such compilers.

Well then you would need to blacklist basically all gcc variants of the 
last decade or so.

Initializing only known members of structures is a perfectly valid 
optimization and well known issue when you then compare the structure 
with memcpy() or use the bytes for hashing or something similar.

>>>> Anyway, I fixed up the code in our tree (with { }), so I'll leave it
>>>> up to you folks to decide what if anything to do about it.
>>> Well considering the known problems with {} initialization I'm
>>> certainly rejecting all patches which turns memset() into {}.
>>>
>> Please point me to specific instances of this problem.

See here for a good example of how people try to avoid issues: 
https://cgit.freedesktop.org/mesa/mesa/commit/?id=9422999e4041d4e984acbd2f44813d5928d20f18

> I think there are a number of places in DC (drivers/gpu/drm/amd/display) where we applied the original proposed solution before realizing that it would only initialize the first element.  It would be nice to get them fixed up.

Yes, agree.

Christian.

>
> Alex
>


^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH] amdgpu/gmc : fix compile warning
  2018-10-08 15:57               ` Deucher, Alexander
  2018-10-08 17:22                 ` Koenig, Christian
@ 2018-10-08 17:41                 ` Guenter Roeck
  2018-10-08 18:24                   ` Deucher, Alexander
  1 sibling, 1 reply; 18+ messages in thread
From: Guenter Roeck @ 2018-10-08 17:41 UTC (permalink / raw)
  To: Deucher, Alexander
  Cc: Koenig, Christian, Peng Hao, airlied, linux-kernel, dri-devel, amd-gfx

On Mon, Oct 08, 2018 at 03:57:07PM +0000, Deucher, Alexander wrote:
> > -----Original Message-----
> > From: Guenter Roeck <groeck7@gmail.com> On Behalf Of Guenter Roeck
> > Sent: Monday, October 8, 2018 10:11 AM
> > To: Koenig, Christian <Christian.Koenig@amd.com>; Peng Hao
> > <peng.hao2@zte.com.cn>
> > Cc: airlied@linux.ie; linux-kernel@vger.kernel.org; dri-
> > devel@lists.freedesktop.org; amd-gfx@lists.freedesktop.org; Deucher,
> > Alexander <Alexander.Deucher@amd.com>
> > Subject: Re: [PATCH] amdgpu/gmc : fix compile warning
> > 
> > On 10/08/2018 06:47 AM, Koenig, Christian wrote:
> > > Am 08.10.2018 um 15:33 schrieb Guenter Roeck:
> > >> On 10/08/2018 01:00 AM, Christian König wrote:
> > >>> Am 05.10.2018 um 10:38 schrieb Guenter Roeck:
> > >>>> On 10/05/2018 01:14 AM, Koenig, Christian wrote:
> > >>>>> Am 04.10.2018 um 20:52 schrieb Guenter Roeck:
> > >>>>>> Hi,
> > >>>>>>
> > >>>>>> On Fri, Sep 14, 2018 at 06:05:52PM +0800, Peng Hao wrote:
> > >>>>>>> drivers/gpu/drm/amd/amdgpu/gmc_v8_0.c:
> > >>>>>>>        In function ‘gmc_v8_0_process_interrupt’:
> > >>>>>>> drivers/gpu/drm/amd/amdgpu/gmc_v8_0.c:1447:10:
> > >>>>>>>        warning: missing braces around initializer
> > >>>>>>> [-Wmissing-braces]
> > >>>>>>>
> > >>>>>>> Signed-off-by: Peng Hao <peng.hao2@zte.com.cn>
> > >>>>>> Was there any feedback on this patch ? The problem does affect
> > >>>>>> us, and we'll need a fix.
> > >>>>>
> > >>>>> Well as discussed using "{ { 0 } }" is as wrong as using "{ 0 }".
> > >>>>>
> > >>>>
> > >>>> Ah, sorry, I must have missed the discussion.
> > >>>>
> > >>>> It is for sure not the best solution, but at least it compiles, and
> > >>>> it seems to be proliferating.
> > >>>
> > >>> Yeah, and exactly that's the problem. As the discussion showed "{ {
> > >>> 0 } }" is buggy because it tells the compiler to only initialize the
> > >>> first member of the structure, but not all of it.
> > >>>
> > >>> That is incorrect and rather dangerous cause it can lead to
> > >>> unforeseen results and should probably trigger a warning.
> > >>>
> > >>>>
> > >>>> $ git grep "{ *{ *0 *} *}" | wc
> > >>>>      144    1180   11802
> > >>>> $ git grep "{ *{ *0 *} *}" drivers/gpu/drm/amd/ | wc
> > >>>>       50     459    5239
> > >>>>
> > >>>>> We should either use only "{ }" or even better make nails with
> > >>>>> heads and use memset().
> > >>>>
> > >>>> I'd rather leave it up to the compiler to decide what is most
> > >>>> efficient.
> > >>>
> > >>> And I would rather prefer to have a working driver :)
> > >>>
> > >>
> > >> So { } isn't correct either ?
> > >
> > > Yes, initializing structures with { } is known to be problematic as well.
> > >
> > > It doesn't necessary initialize all bytes when you have padding
> > > causing random failures when structures are memcmp().
> > >
> > >>
> > >> One thing I found missing in the discussion was the reference to the
> > >> C standard.
> > >> The C99 standard states in section 6.7.8 (Initialization) clause 19:
> > >> "... all
> > >> subobjects that are not initialized explicitly shall be initialized
> > >> implicitly the same as objects that have static storage duration".
> > >> Clause 21 makes further reference to partial initialization,
> > >> suggesting the same. Various online resources, including the gcc
> > >> documentation, all state the same. I don't find any reference to a
> > >> partial initialization which would leave members of a structure
> > >> undefined. It would be interesting for me to understand how and why
> > >> this does not apply here.
> > >>
> > >> In this context, it is interesting that the other 48 instances of the
> > >> { { 0 } } initialization in the same driver don't raise similar
> > >> concerns, nor seemed to have caused any operational problems.
> > >
> > > Feel free to provide patches to replace those with memset().
> > >
> > 
> > Not me. As I see it, the problem, if it exists, would be a violation of the C
> > standard. I don't believe hacking around bad C compilers. I would rather
> > blacklist such compilers.
> > 
> > >>
> > >> Anyway, I fixed up the code in our tree (with { }), so I'll leave it
> > >> up to you folks to decide what if anything to do about it.
> > >
> > > Well considering the known problems with {} initialization I'm
> > > certainly rejecting all patches which turns memset() into {}.
> > >
> > 
> > Please point me to specific instances of this problem.
> 
> I think there are a number of places in DC (drivers/gpu/drm/amd/display) where we applied the original proposed solution before realizing that it would only initialize the first element.  It would be nice to get them fixed up.
> 

I think this is factually incorrect. What you might want to try to say
is that padding may not be initialized when using anything but memset().
But that is a different problem.

Guenter

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH] amdgpu/gmc : fix compile warning
  2018-10-08 17:22                 ` Koenig, Christian
@ 2018-10-08 17:46                   ` Guenter Roeck
  2018-10-08 18:13                     ` Koenig, Christian
  0 siblings, 1 reply; 18+ messages in thread
From: Guenter Roeck @ 2018-10-08 17:46 UTC (permalink / raw)
  To: Koenig, Christian
  Cc: Deucher, Alexander, Peng Hao, airlied, linux-kernel, dri-devel, amd-gfx

On Mon, Oct 08, 2018 at 05:22:24PM +0000, Koenig, Christian wrote:
> Am 08.10.2018 um 17:57 schrieb Deucher, Alexander:
> >>>> One thing I found missing in the discussion was the reference to the
> >>>> C standard.
> >>>> The C99 standard states in section 6.7.8 (Initialization) clause 19:
> >>>> "... all
> >>>> subobjects that are not initialized explicitly shall be initialized
> >>>> implicitly the same as objects that have static storage duration".
> >>>> Clause 21 makes further reference to partial initialization,
> >>>> suggesting the same. Various online resources, including the gcc
> >>>> documentation, all state the same. I don't find any reference to a
> >>>> partial initialization which would leave members of a structure
> >>>> undefined. It would be interesting for me to understand how and why
> >>>> this does not apply here.
> >>>>
> >>>> In this context, it is interesting that the other 48 instances of the
> >>>> { { 0 } } initialization in the same driver don't raise similar
> >>>> concerns, nor seemed to have caused any operational problems.
> >>> Feel free to provide patches to replace those with memset().
> >>>
> >> Not me. As I see it, the problem, if it exists, would be a violation of the C
> >> standard. I don't believe hacking around bad C compilers. I would rather
> >> blacklist such compilers.
> 
> Well then you would need to blacklist basically all gcc variants of the 
> last decade or so.
> 
> Initializing only known members of structures is a perfectly valid 
> optimization and well known issue when you then compare the structure 
> with memcpy() or use the bytes for hashing or something similar.
> 

Isn't that about padding ? That is a completely different issue.

Guenter

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH] amdgpu/gmc : fix compile warning
  2018-10-08 17:46                   ` Guenter Roeck
@ 2018-10-08 18:13                     ` Koenig, Christian
  2018-10-19  8:53                       ` Daniel Vetter
  0 siblings, 1 reply; 18+ messages in thread
From: Koenig, Christian @ 2018-10-08 18:13 UTC (permalink / raw)
  To: Guenter Roeck
  Cc: Deucher, Alexander, Peng Hao, airlied, linux-kernel, dri-devel, amd-gfx

Am 08.10.2018 um 19:46 schrieb Guenter Roeck:
> On Mon, Oct 08, 2018 at 05:22:24PM +0000, Koenig, Christian wrote:
>> Am 08.10.2018 um 17:57 schrieb Deucher, Alexander:
>>>>>> One thing I found missing in the discussion was the reference to the
>>>>>> C standard.
>>>>>> The C99 standard states in section 6.7.8 (Initialization) clause 19:
>>>>>> "... all
>>>>>> subobjects that are not initialized explicitly shall be initialized
>>>>>> implicitly the same as objects that have static storage duration".
>>>>>> Clause 21 makes further reference to partial initialization,
>>>>>> suggesting the same. Various online resources, including the gcc
>>>>>> documentation, all state the same. I don't find any reference to a
>>>>>> partial initialization which would leave members of a structure
>>>>>> undefined. It would be interesting for me to understand how and why
>>>>>> this does not apply here.
>>>>>>
>>>>>> In this context, it is interesting that the other 48 instances of the
>>>>>> { { 0 } } initialization in the same driver don't raise similar
>>>>>> concerns, nor seemed to have caused any operational problems.
>>>>> Feel free to provide patches to replace those with memset().
>>>>>
>>>> Not me. As I see it, the problem, if it exists, would be a violation of the C
>>>> standard. I don't believe hacking around bad C compilers. I would rather
>>>> blacklist such compilers.
>> Well then you would need to blacklist basically all gcc variants of the
>> last decade or so.
>>
>> Initializing only known members of structures is a perfectly valid
>> optimization and well known issue when you then compare the structure
>> with memcpy() or use the bytes for hashing or something similar.
>>
> Isn't that about padding ? That is a completely different issue.

Correct, yes. But that is the reason why I recommend using memset() for 
zero initialization.

See we don't know the inner layout of the structure, could be another 
structure or an union.

If it's a structure everything is fine because if you initialize one 
structure member all other get their default type (whatever that means), 
but if it's an union.....

Not sure if compilers still react allergic to that, but its the status 
I've learned the hard way when the C99 standard came out and it still 
seems like people are working around that so I recommend everybody to 
stick with memset().

Christian.

>
> Guenter


^ permalink raw reply	[flat|nested] 18+ messages in thread

* RE: [PATCH] amdgpu/gmc : fix compile warning
  2018-10-08 17:41                 ` Guenter Roeck
@ 2018-10-08 18:24                   ` Deucher, Alexander
  0 siblings, 0 replies; 18+ messages in thread
From: Deucher, Alexander @ 2018-10-08 18:24 UTC (permalink / raw)
  To: Guenter Roeck
  Cc: Koenig, Christian, Peng Hao, airlied, linux-kernel, dri-devel, amd-gfx

> -----Original Message-----
> From: Guenter Roeck <groeck7@gmail.com> On Behalf Of Guenter Roeck
> Sent: Monday, October 8, 2018 1:41 PM
> To: Deucher, Alexander <Alexander.Deucher@amd.com>
> Cc: Koenig, Christian <Christian.Koenig@amd.com>; Peng Hao
> <peng.hao2@zte.com.cn>; airlied@linux.ie; linux-kernel@vger.kernel.org;
> dri-devel@lists.freedesktop.org; amd-gfx@lists.freedesktop.org
> Subject: Re: [PATCH] amdgpu/gmc : fix compile warning
> 
> On Mon, Oct 08, 2018 at 03:57:07PM +0000, Deucher, Alexander wrote:
> > > -----Original Message-----
> > > From: Guenter Roeck <groeck7@gmail.com> On Behalf Of Guenter
> Roeck
> > > Sent: Monday, October 8, 2018 10:11 AM
> > > To: Koenig, Christian <Christian.Koenig@amd.com>; Peng Hao
> > > <peng.hao2@zte.com.cn>
> > > Cc: airlied@linux.ie; linux-kernel@vger.kernel.org; dri-
> > > devel@lists.freedesktop.org; amd-gfx@lists.freedesktop.org; Deucher,
> > > Alexander <Alexander.Deucher@amd.com>
> > > Subject: Re: [PATCH] amdgpu/gmc : fix compile warning
> > >
> > > On 10/08/2018 06:47 AM, Koenig, Christian wrote:
> > > > Am 08.10.2018 um 15:33 schrieb Guenter Roeck:
> > > >> On 10/08/2018 01:00 AM, Christian König wrote:
> > > >>> Am 05.10.2018 um 10:38 schrieb Guenter Roeck:
> > > >>>> On 10/05/2018 01:14 AM, Koenig, Christian wrote:
> > > >>>>> Am 04.10.2018 um 20:52 schrieb Guenter Roeck:
> > > >>>>>> Hi,
> > > >>>>>>
> > > >>>>>> On Fri, Sep 14, 2018 at 06:05:52PM +0800, Peng Hao wrote:
> > > >>>>>>> drivers/gpu/drm/amd/amdgpu/gmc_v8_0.c:
> > > >>>>>>>        In function ‘gmc_v8_0_process_interrupt’:
> > > >>>>>>> drivers/gpu/drm/amd/amdgpu/gmc_v8_0.c:1447:10:
> > > >>>>>>>        warning: missing braces around initializer
> > > >>>>>>> [-Wmissing-braces]
> > > >>>>>>>
> > > >>>>>>> Signed-off-by: Peng Hao <peng.hao2@zte.com.cn>
> > > >>>>>> Was there any feedback on this patch ? The problem does
> > > >>>>>> affect us, and we'll need a fix.
> > > >>>>>
> > > >>>>> Well as discussed using "{ { 0 } }" is as wrong as using "{ 0 }".
> > > >>>>>
> > > >>>>
> > > >>>> Ah, sorry, I must have missed the discussion.
> > > >>>>
> > > >>>> It is for sure not the best solution, but at least it compiles,
> > > >>>> and it seems to be proliferating.
> > > >>>
> > > >>> Yeah, and exactly that's the problem. As the discussion showed
> > > >>> "{ {
> > > >>> 0 } }" is buggy because it tells the compiler to only initialize
> > > >>> the first member of the structure, but not all of it.
> > > >>>
> > > >>> That is incorrect and rather dangerous cause it can lead to
> > > >>> unforeseen results and should probably trigger a warning.
> > > >>>
> > > >>>>
> > > >>>> $ git grep "{ *{ *0 *} *}" | wc
> > > >>>>      144    1180   11802
> > > >>>> $ git grep "{ *{ *0 *} *}" drivers/gpu/drm/amd/ | wc
> > > >>>>       50     459    5239
> > > >>>>
> > > >>>>> We should either use only "{ }" or even better make nails with
> > > >>>>> heads and use memset().
> > > >>>>
> > > >>>> I'd rather leave it up to the compiler to decide what is most
> > > >>>> efficient.
> > > >>>
> > > >>> And I would rather prefer to have a working driver :)
> > > >>>
> > > >>
> > > >> So { } isn't correct either ?
> > > >
> > > > Yes, initializing structures with { } is known to be problematic as well.
> > > >
> > > > It doesn't necessary initialize all bytes when you have padding
> > > > causing random failures when structures are memcmp().
> > > >
> > > >>
> > > >> One thing I found missing in the discussion was the reference to
> > > >> the C standard.
> > > >> The C99 standard states in section 6.7.8 (Initialization) clause 19:
> > > >> "... all
> > > >> subobjects that are not initialized explicitly shall be
> > > >> initialized implicitly the same as objects that have static storage
> duration".
> > > >> Clause 21 makes further reference to partial initialization,
> > > >> suggesting the same. Various online resources, including the gcc
> > > >> documentation, all state the same. I don't find any reference to
> > > >> a partial initialization which would leave members of a structure
> > > >> undefined. It would be interesting for me to understand how and
> > > >> why this does not apply here.
> > > >>
> > > >> In this context, it is interesting that the other 48 instances of
> > > >> the { { 0 } } initialization in the same driver don't raise
> > > >> similar concerns, nor seemed to have caused any operational
> problems.
> > > >
> > > > Feel free to provide patches to replace those with memset().
> > > >
> > >
> > > Not me. As I see it, the problem, if it exists, would be a violation
> > > of the C standard. I don't believe hacking around bad C compilers. I
> > > would rather blacklist such compilers.
> > >
> > > >>
> > > >> Anyway, I fixed up the code in our tree (with { }), so I'll leave
> > > >> it up to you folks to decide what if anything to do about it.
> > > >
> > > > Well considering the known problems with {} initialization I'm
> > > > certainly rejecting all patches which turns memset() into {}.
> > > >
> > >
> > > Please point me to specific instances of this problem.
> >
> > I think there are a number of places in DC (drivers/gpu/drm/amd/display)
> where we applied the original proposed solution before realizing that it
> would only initialize the first element.  It would be nice to get them fixed up.
> >
> 
> I think this is factually incorrect. What you might want to try to say is that
> padding may not be initialized when using anything but memset().
> But that is a different problem.
>

I just meant that there are a number of places were warning fix patches got applied that did the same thing this patch attempted to do ( replace { 0 } with { { 0 } }) which may have introduced subtle issues.

Alex


^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH] amdgpu/gmc : fix compile warning
  2018-10-08 18:13                     ` Koenig, Christian
@ 2018-10-19  8:53                       ` Daniel Vetter
  2018-10-19  8:56                         ` Daniel Vetter
  2018-10-19 13:08                         ` Guenter Roeck
  0 siblings, 2 replies; 18+ messages in thread
From: Daniel Vetter @ 2018-10-19  8:53 UTC (permalink / raw)
  To: Koenig, Christian
  Cc: Guenter Roeck, Peng Hao, airlied, linux-kernel, amd-gfx,
	dri-devel, Deucher, Alexander, Martin Peres

On Mon, Oct 08, 2018 at 06:13:56PM +0000, Koenig, Christian wrote:
> Am 08.10.2018 um 19:46 schrieb Guenter Roeck:
> > On Mon, Oct 08, 2018 at 05:22:24PM +0000, Koenig, Christian wrote:
> >> Am 08.10.2018 um 17:57 schrieb Deucher, Alexander:
> >>>>>> One thing I found missing in the discussion was the reference to the
> >>>>>> C standard.
> >>>>>> The C99 standard states in section 6.7.8 (Initialization) clause 19:
> >>>>>> "... all
> >>>>>> subobjects that are not initialized explicitly shall be initialized
> >>>>>> implicitly the same as objects that have static storage duration".
> >>>>>> Clause 21 makes further reference to partial initialization,
> >>>>>> suggesting the same. Various online resources, including the gcc
> >>>>>> documentation, all state the same. I don't find any reference to a
> >>>>>> partial initialization which would leave members of a structure
> >>>>>> undefined. It would be interesting for me to understand how and why
> >>>>>> this does not apply here.
> >>>>>>
> >>>>>> In this context, it is interesting that the other 48 instances of the
> >>>>>> { { 0 } } initialization in the same driver don't raise similar
> >>>>>> concerns, nor seemed to have caused any operational problems.
> >>>>> Feel free to provide patches to replace those with memset().
> >>>>>
> >>>> Not me. As I see it, the problem, if it exists, would be a violation of the C
> >>>> standard. I don't believe hacking around bad C compilers. I would rather
> >>>> blacklist such compilers.
> >> Well then you would need to blacklist basically all gcc variants of the
> >> last decade or so.
> >>
> >> Initializing only known members of structures is a perfectly valid
> >> optimization and well known issue when you then compare the structure
> >> with memcpy() or use the bytes for hashing or something similar.
> >>
> > Isn't that about padding ? That is a completely different issue.
> 
> Correct, yes. But that is the reason why I recommend using memset() for 
> zero initialization.
> 
> See we don't know the inner layout of the structure, could be another 
> structure or an union.
> 
> If it's a structure everything is fine because if you initialize one 
> structure member all other get their default type (whatever that means), 
> but if it's an union.....
> 
> Not sure if compilers still react allergic to that, but its the status 
> I've learned the hard way when the C99 standard came out and it still 
> seems like people are working around that so I recommend everybody to 
> stick with memset().

Went boom:

https://bugs.freedesktop.org/show_bug.cgi?id=108490

Can we revert?

Also, can we properly igt this so that intel-gfx-ci could test this before
it's all fireworks?

Thanks, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH] amdgpu/gmc : fix compile warning
  2018-10-19  8:53                       ` Daniel Vetter
@ 2018-10-19  8:56                         ` Daniel Vetter
  2018-10-19 13:08                         ` Guenter Roeck
  1 sibling, 0 replies; 18+ messages in thread
From: Daniel Vetter @ 2018-10-19  8:56 UTC (permalink / raw)
  To: Christian König, Guenter Roeck, Peng Hao, Dave Airlie,
	Linux Kernel Mailing List, amd-gfx list, dri-devel, Alex Deucher,
	Peres, Martin

On Fri, Oct 19, 2018 at 10:53 AM Daniel Vetter <daniel@ffwll.ch> wrote:
>
> On Mon, Oct 08, 2018 at 06:13:56PM +0000, Koenig, Christian wrote:
> > Am 08.10.2018 um 19:46 schrieb Guenter Roeck:
> > > On Mon, Oct 08, 2018 at 05:22:24PM +0000, Koenig, Christian wrote:
> > >> Am 08.10.2018 um 17:57 schrieb Deucher, Alexander:
> > >>>>>> One thing I found missing in the discussion was the reference to the
> > >>>>>> C standard.
> > >>>>>> The C99 standard states in section 6.7.8 (Initialization) clause 19:
> > >>>>>> "... all
> > >>>>>> subobjects that are not initialized explicitly shall be initialized
> > >>>>>> implicitly the same as objects that have static storage duration".
> > >>>>>> Clause 21 makes further reference to partial initialization,
> > >>>>>> suggesting the same. Various online resources, including the gcc
> > >>>>>> documentation, all state the same. I don't find any reference to a
> > >>>>>> partial initialization which would leave members of a structure
> > >>>>>> undefined. It would be interesting for me to understand how and why
> > >>>>>> this does not apply here.
> > >>>>>>
> > >>>>>> In this context, it is interesting that the other 48 instances of the
> > >>>>>> { { 0 } } initialization in the same driver don't raise similar
> > >>>>>> concerns, nor seemed to have caused any operational problems.
> > >>>>> Feel free to provide patches to replace those with memset().
> > >>>>>
> > >>>> Not me. As I see it, the problem, if it exists, would be a violation of the C
> > >>>> standard. I don't believe hacking around bad C compilers. I would rather
> > >>>> blacklist such compilers.
> > >> Well then you would need to blacklist basically all gcc variants of the
> > >> last decade or so.
> > >>
> > >> Initializing only known members of structures is a perfectly valid
> > >> optimization and well known issue when you then compare the structure
> > >> with memcpy() or use the bytes for hashing or something similar.
> > >>
> > > Isn't that about padding ? That is a completely different issue.
> >
> > Correct, yes. But that is the reason why I recommend using memset() for
> > zero initialization.
> >
> > See we don't know the inner layout of the structure, could be another
> > structure or an union.
> >
> > If it's a structure everything is fine because if you initialize one
> > structure member all other get their default type (whatever that means),
> > but if it's an union.....
> >
> > Not sure if compilers still react allergic to that, but its the status
> > I've learned the hard way when the C99 standard came out and it still
> > seems like people are working around that so I recommend everybody to
> > stick with memset().
>
> Went boom:
>
> https://bugs.freedesktop.org/show_bug.cgi?id=108490
>
> Can we revert?
>
> Also, can we properly igt this so that intel-gfx-ci could test this before
> it's all fireworks?

Please disregard this reply, that was the wrong thread.

Sorry, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH] amdgpu/gmc : fix compile warning
  2018-10-19  8:53                       ` Daniel Vetter
  2018-10-19  8:56                         ` Daniel Vetter
@ 2018-10-19 13:08                         ` Guenter Roeck
  2018-10-19 15:30                           ` Alex Deucher
  1 sibling, 1 reply; 18+ messages in thread
From: Guenter Roeck @ 2018-10-19 13:08 UTC (permalink / raw)
  To: Koenig, Christian, Peng Hao, airlied, linux-kernel, amd-gfx,
	dri-devel, Deucher, Alexander, Martin Peres

On 10/19/2018 01:53 AM, Daniel Vetter wrote:
> On Mon, Oct 08, 2018 at 06:13:56PM +0000, Koenig, Christian wrote:
>> Am 08.10.2018 um 19:46 schrieb Guenter Roeck:
>>> On Mon, Oct 08, 2018 at 05:22:24PM +0000, Koenig, Christian wrote:
>>>> Am 08.10.2018 um 17:57 schrieb Deucher, Alexander:
>>>>>>>> One thing I found missing in the discussion was the reference to the
>>>>>>>> C standard.
>>>>>>>> The C99 standard states in section 6.7.8 (Initialization) clause 19:
>>>>>>>> "... all
>>>>>>>> subobjects that are not initialized explicitly shall be initialized
>>>>>>>> implicitly the same as objects that have static storage duration".
>>>>>>>> Clause 21 makes further reference to partial initialization,
>>>>>>>> suggesting the same. Various online resources, including the gcc
>>>>>>>> documentation, all state the same. I don't find any reference to a
>>>>>>>> partial initialization which would leave members of a structure
>>>>>>>> undefined. It would be interesting for me to understand how and why
>>>>>>>> this does not apply here.
>>>>>>>>
>>>>>>>> In this context, it is interesting that the other 48 instances of the
>>>>>>>> { { 0 } } initialization in the same driver don't raise similar
>>>>>>>> concerns, nor seemed to have caused any operational problems.
>>>>>>> Feel free to provide patches to replace those with memset().
>>>>>>>
>>>>>> Not me. As I see it, the problem, if it exists, would be a violation of the C
>>>>>> standard. I don't believe hacking around bad C compilers. I would rather
>>>>>> blacklist such compilers.
>>>> Well then you would need to blacklist basically all gcc variants of the
>>>> last decade or so.
>>>>
>>>> Initializing only known members of structures is a perfectly valid
>>>> optimization and well known issue when you then compare the structure
>>>> with memcpy() or use the bytes for hashing or something similar.
>>>>
>>> Isn't that about padding ? That is a completely different issue.
>>
>> Correct, yes. But that is the reason why I recommend using memset() for
>> zero initialization.
>>
>> See we don't know the inner layout of the structure, could be another
>> structure or an union.
>>
>> If it's a structure everything is fine because if you initialize one
>> structure member all other get their default type (whatever that means),
>> but if it's an union.....
>>
>> Not sure if compilers still react allergic to that, but its the status
>> I've learned the hard way when the C99 standard came out and it still
>> seems like people are working around that so I recommend everybody to
>> stick with memset().
> 
> Went boom:
> 
> https://bugs.freedesktop.org/show_bug.cgi?id=108490
> 

What went boom ? This patch wasn't accepted, and I don't immediately see
the correlation of the suggested revert with the rejected patch.

Guenter

> Can we revert?
> 
> Also, can we properly igt this so that intel-gfx-ci could test this before
> it's all fireworks?
> 
> Thanks, Daniel
> 


^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH] amdgpu/gmc : fix compile warning
  2018-10-19 13:08                         ` Guenter Roeck
@ 2018-10-19 15:30                           ` Alex Deucher
  0 siblings, 0 replies; 18+ messages in thread
From: Alex Deucher @ 2018-10-19 15:30 UTC (permalink / raw)
  To: Guenter Roeck
  Cc: Christian Koenig, peng.hao2, Dave Airlie, LKML, amd-gfx list,
	Maling list - DRI developers, Deucher, Alexander, Martin Peres

On Fri, Oct 19, 2018 at 9:31 AM Guenter Roeck <linux@roeck-us.net> wrote:
>
> On 10/19/2018 01:53 AM, Daniel Vetter wrote:
> > On Mon, Oct 08, 2018 at 06:13:56PM +0000, Koenig, Christian wrote:
> >> Am 08.10.2018 um 19:46 schrieb Guenter Roeck:
> >>> On Mon, Oct 08, 2018 at 05:22:24PM +0000, Koenig, Christian wrote:
> >>>> Am 08.10.2018 um 17:57 schrieb Deucher, Alexander:
> >>>>>>>> One thing I found missing in the discussion was the reference to the
> >>>>>>>> C standard.
> >>>>>>>> The C99 standard states in section 6.7.8 (Initialization) clause 19:
> >>>>>>>> "... all
> >>>>>>>> subobjects that are not initialized explicitly shall be initialized
> >>>>>>>> implicitly the same as objects that have static storage duration".
> >>>>>>>> Clause 21 makes further reference to partial initialization,
> >>>>>>>> suggesting the same. Various online resources, including the gcc
> >>>>>>>> documentation, all state the same. I don't find any reference to a
> >>>>>>>> partial initialization which would leave members of a structure
> >>>>>>>> undefined. It would be interesting for me to understand how and why
> >>>>>>>> this does not apply here.
> >>>>>>>>
> >>>>>>>> In this context, it is interesting that the other 48 instances of the
> >>>>>>>> { { 0 } } initialization in the same driver don't raise similar
> >>>>>>>> concerns, nor seemed to have caused any operational problems.
> >>>>>>> Feel free to provide patches to replace those with memset().
> >>>>>>>
> >>>>>> Not me. As I see it, the problem, if it exists, would be a violation of the C
> >>>>>> standard. I don't believe hacking around bad C compilers. I would rather
> >>>>>> blacklist such compilers.
> >>>> Well then you would need to blacklist basically all gcc variants of the
> >>>> last decade or so.
> >>>>
> >>>> Initializing only known members of structures is a perfectly valid
> >>>> optimization and well known issue when you then compare the structure
> >>>> with memcpy() or use the bytes for hashing or something similar.
> >>>>
> >>> Isn't that about padding ? That is a completely different issue.
> >>
> >> Correct, yes. But that is the reason why I recommend using memset() for
> >> zero initialization.
> >>
> >> See we don't know the inner layout of the structure, could be another
> >> structure or an union.
> >>
> >> If it's a structure everything is fine because if you initialize one
> >> structure member all other get their default type (whatever that means),
> >> but if it's an union.....
> >>
> >> Not sure if compilers still react allergic to that, but its the status
> >> I've learned the hard way when the C99 standard came out and it still
> >> seems like people are working around that so I recommend everybody to
> >> stick with memset().
> >
> > Went boom:
> >
> > https://bugs.freedesktop.org/show_bug.cgi?id=108490
> >
>
> What went boom ? This patch wasn't accepted, and I don't immediately see
> the correlation of the suggested revert with the rejected patch.

Daniel accidentally replied to the wrong thread.  Please ignore.

Alex

>
> Guenter
>
> > Can we revert?
> >
> > Also, can we properly igt this so that intel-gfx-ci could test this before
> > it's all fireworks?
> >
> > Thanks, Daniel
> >
>
> _______________________________________________
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx

^ permalink raw reply	[flat|nested] 18+ messages in thread

end of thread, other threads:[~2018-10-19 15:31 UTC | newest]

Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-09-14 10:05 [PATCH] amdgpu/gmc : fix compile warning Peng Hao
2018-10-04 18:52 ` Guenter Roeck
2018-10-05  8:14   ` Koenig, Christian
2018-10-05  8:38     ` Guenter Roeck
2018-10-08  8:00       ` Christian König
2018-10-08 13:33         ` Guenter Roeck
2018-10-08 13:47           ` Koenig, Christian
2018-10-08 14:10             ` Guenter Roeck
2018-10-08 15:57               ` Deucher, Alexander
2018-10-08 17:22                 ` Koenig, Christian
2018-10-08 17:46                   ` Guenter Roeck
2018-10-08 18:13                     ` Koenig, Christian
2018-10-19  8:53                       ` Daniel Vetter
2018-10-19  8:56                         ` Daniel Vetter
2018-10-19 13:08                         ` Guenter Roeck
2018-10-19 15:30                           ` Alex Deucher
2018-10-08 17:41                 ` Guenter Roeck
2018-10-08 18:24                   ` Deucher, Alexander

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).