[PATCH] drm/amdgpu: Add braces to initialize task_info subojects

[PATCH] drm/amdgpu: Add braces to initialize task_info subojects

[Nathan Chancellor]

Clang warns if there are missing braces around a subobject
initializer.

drivers/gpu/drm/amd/amdgpu/gmc_v8_0.c:1447:41: warning: suggest braces
around initialization of subobject [-Wmissing-braces]
                struct amdgpu_task_info task_info = { 0 };
                                                      ^
                                                      {}
1 warning generated.

drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c:262:41: warning: suggest braces
around initialization of subobject [-Wmissing-braces]
                struct amdgpu_task_info task_info = { 0 };
                                                      ^
                                                      {}
1 warning generated.

Reported-by: Nick Desaulniers <ndesaulniers@google.com>
Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
---
 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 9333109b210d..968cc1b8cdff 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 72f8018fa2a8..a781a5027212 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);
 
-- 
2.18.0

Re: [PATCH] drm/amdgpu: Add braces to initialize task_info subojects

[Nick Desaulniers]

On Tue, Sep 11, 2018 at 5:26 PM Nathan Chancellor
<natechancellor@gmail.com> wrote:
>
> Clang warns if there are missing braces around a subobject
> initializer.
>
> drivers/gpu/drm/amd/amdgpu/gmc_v8_0.c:1447:41: warning: suggest braces
> around initialization of subobject [-Wmissing-braces]
>                 struct amdgpu_task_info task_info = { 0 };
>                                                       ^
>                                                       {}
> 1 warning generated.
>
> drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c:262:41: warning: suggest braces
> around initialization of subobject [-Wmissing-braces]
>                 struct amdgpu_task_info task_info = { 0 };
>                                                       ^
>                                                       {}
> 1 warning generated.
>
> Reported-by: Nick Desaulniers <ndesaulniers@google.com>
> Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
> ---
>  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 9333109b210d..968cc1b8cdff 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 } };

Hi Nathan,
Thanks for this patch.  I discussed this syntax with our language
lawyers.  Turns out, this is not quite correct, as you're now saying
"initialize the first subobject to zero, but not the rest of the
object."  -Wmissing-field-initializers would highlight this, but it's
not part of -Wall.  It would be more correct to zero initialize the
full struct, including all of its subobjects with `= {};`.

>
>                 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 72f8018fa2a8..a781a5027212 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);
>
> --
> 2.18.0
>


-- 
Thanks,
~Nick Desaulniers

Re: [PATCH] drm/amdgpu: Add braces to initialize task_info subojects

[Nathan Chancellor]

On Wed, Sep 12, 2018 at 10:38:30AM -0700, Nick Desaulniers wrote:
> On Tue, Sep 11, 2018 at 5:26 PM Nathan Chancellor
> <natechancellor@gmail.com> wrote:
> >
> > Clang warns if there are missing braces around a subobject
> > initializer.
> >
> > drivers/gpu/drm/amd/amdgpu/gmc_v8_0.c:1447:41: warning: suggest braces
> > around initialization of subobject [-Wmissing-braces]
> >                 struct amdgpu_task_info task_info = { 0 };
> >                                                       ^
> >                                                       {}
> > 1 warning generated.
> >
> > drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c:262:41: warning: suggest braces
> > around initialization of subobject [-Wmissing-braces]
> >                 struct amdgpu_task_info task_info = { 0 };
> >                                                       ^
> >                                                       {}
> > 1 warning generated.
> >
> > Reported-by: Nick Desaulniers <ndesaulniers@google.com>
> > Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
> > ---
> >  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 9333109b210d..968cc1b8cdff 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 } };
> 
> Hi Nathan,
> Thanks for this patch.  I discussed this syntax with our language
> lawyers.  Turns out, this is not quite correct, as you're now saying
> "initialize the first subobject to zero, but not the rest of the
> object."  -Wmissing-field-initializers would highlight this, but it's
> not part of -Wall.  It would be more correct to zero initialize the
> full struct, including all of its subobjects with `= {};`.
> 

Good point, I was debating on which one was correct. There are several
places in this driver that use the multiple brace + 0 idiom, which is
why I used this form. I will spin up a v2 with your suggestion, thank
you for the review!

Nathan

> >
> >                 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 72f8018fa2a8..a781a5027212 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);
> >
> > --
> > 2.18.0
> >
> 
> 
> -- 
> Thanks,
> ~Nick Desaulniers

Re: [PATCH] drm/amdgpu: Add braces to initialize task_info subojects

[Alex Deucher]

On Wed, Sep 12, 2018 at 2:40 PM Nathan Chancellor
<natechancellor@gmail.com> wrote:
>
> On Wed, Sep 12, 2018 at 10:38:30AM -0700, Nick Desaulniers wrote:
> > On Tue, Sep 11, 2018 at 5:26 PM Nathan Chancellor
> > <natechancellor@gmail.com> wrote:
> > >
> > > Clang warns if there are missing braces around a subobject
> > > initializer.
> > >
> > > drivers/gpu/drm/amd/amdgpu/gmc_v8_0.c:1447:41: warning: suggest braces
> > > around initialization of subobject [-Wmissing-braces]
> > >                 struct amdgpu_task_info task_info = { 0 };
> > >                                                       ^
> > >                                                       {}
> > > 1 warning generated.
> > >
> > > drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c:262:41: warning: suggest braces
> > > around initialization of subobject [-Wmissing-braces]
> > >                 struct amdgpu_task_info task_info = { 0 };
> > >                                                       ^
> > >                                                       {}
> > > 1 warning generated.
> > >
> > > Reported-by: Nick Desaulniers <ndesaulniers@google.com>
> > > Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
> > > ---
> > >  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 9333109b210d..968cc1b8cdff 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 } };
> >
> > Hi Nathan,
> > Thanks for this patch.  I discussed this syntax with our language
> > lawyers.  Turns out, this is not quite correct, as you're now saying
> > "initialize the first subobject to zero, but not the rest of the
> > object."  -Wmissing-field-initializers would highlight this, but it's
> > not part of -Wall.  It would be more correct to zero initialize the
> > full struct, including all of its subobjects with `= {};`.
> >
>
> Good point, I was debating on which one was correct. There are several
> places in this driver that use the multiple brace + 0 idiom, which is
> why I used this form. I will spin up a v2 with your suggestion, thank
> you for the review!

Feel free to fix up the others as well. The others were only changed
due to the same warning you sent the patch for.

Alex

>
> Nathan
>
> > >
> > >                 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 72f8018fa2a8..a781a5027212 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);
> > >
> > > --
> > > 2.18.0
> > >
> >
> >
> > --
> > Thanks,
> > ~Nick Desaulniers
> _______________________________________________
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] drm/amdgpu: Add braces to initialize task_info subojects

[Alex Deucher]

On Wed, Sep 12, 2018 at 2:40 PM Nathan Chancellor
<natechancellor@gmail.com> wrote:
>
> On Wed, Sep 12, 2018 at 10:38:30AM -0700, Nick Desaulniers wrote:
> > On Tue, Sep 11, 2018 at 5:26 PM Nathan Chancellor
> > <natechancellor@gmail.com> wrote:
> > >
> > > Clang warns if there are missing braces around a subobject
> > > initializer.
> > >
> > > drivers/gpu/drm/amd/amdgpu/gmc_v8_0.c:1447:41: warning: suggest braces
> > > around initialization of subobject [-Wmissing-braces]
> > >                 struct amdgpu_task_info task_info = { 0 };
> > >                                                       ^
> > >                                                       {}
> > > 1 warning generated.
> > >
> > > drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c:262:41: warning: suggest braces
> > > around initialization of subobject [-Wmissing-braces]
> > >                 struct amdgpu_task_info task_info = { 0 };
> > >                                                       ^
> > >                                                       {}
> > > 1 warning generated.
> > >
> > > Reported-by: Nick Desaulniers <ndesaulniers@google.com>
> > > Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
> > > ---
> > >  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 9333109b210d..968cc1b8cdff 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 } };
> >
> > Hi Nathan,
> > Thanks for this patch.  I discussed this syntax with our language
> > lawyers.  Turns out, this is not quite correct, as you're now saying
> > "initialize the first subobject to zero, but not the rest of the
> > object."  -Wmissing-field-initializers would highlight this, but it's
> > not part of -Wall.  It would be more correct to zero initialize the
> > full struct, including all of its subobjects with `= {};`.
> >
>
> Good point, I was debating on which one was correct. There are several
> places in this driver that use the multiple brace + 0 idiom, which is
> why I used this form. I will spin up a v2 with your suggestion, thank
> you for the review!

Feel free to fix up the others as well. The others were only changed
due to the same warning you sent the patch for.

Alex

>
> Nathan
>
> > >
> > >                 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 72f8018fa2a8..a781a5027212 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);
> > >
> > > --
> > > 2.18.0
> > >
> >
> >
> > --
> > Thanks,
> > ~Nick Desaulniers
> _______________________________________________
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] drm/amdgpu: Add braces to initialize task_info subojects

[Nathan Chancellor]

On Wed, Sep 12, 2018 at 02:44:34PM -0400, Alex Deucher wrote:
> On Wed, Sep 12, 2018 at 2:40 PM Nathan Chancellor
> <natechancellor@gmail.com> wrote:
> >
> > On Wed, Sep 12, 2018 at 10:38:30AM -0700, Nick Desaulniers wrote:
> > > On Tue, Sep 11, 2018 at 5:26 PM Nathan Chancellor
> > > <natechancellor@gmail.com> wrote:
> > > >
> > > > Clang warns if there are missing braces around a subobject
> > > > initializer.
> > > >
> > > > drivers/gpu/drm/amd/amdgpu/gmc_v8_0.c:1447:41: warning: suggest braces
> > > > around initialization of subobject [-Wmissing-braces]
> > > >                 struct amdgpu_task_info task_info = { 0 };
> > > >                                                       ^
> > > >                                                       {}
> > > > 1 warning generated.
> > > >
> > > > drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c:262:41: warning: suggest braces
> > > > around initialization of subobject [-Wmissing-braces]
> > > >                 struct amdgpu_task_info task_info = { 0 };
> > > >                                                       ^
> > > >                                                       {}
> > > > 1 warning generated.
> > > >
> > > > Reported-by: Nick Desaulniers <ndesaulniers@google.com>
> > > > Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
> > > > ---
> > > >  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 9333109b210d..968cc1b8cdff 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 } };
> > >
> > > Hi Nathan,
> > > Thanks for this patch.  I discussed this syntax with our language
> > > lawyers.  Turns out, this is not quite correct, as you're now saying
> > > "initialize the first subobject to zero, but not the rest of the
> > > object."  -Wmissing-field-initializers would highlight this, but it's
> > > not part of -Wall.  It would be more correct to zero initialize the
> > > full struct, including all of its subobjects with `= {};`.
> > >
> >
> > Good point, I was debating on which one was correct. There are several
> > places in this driver that use the multiple brace + 0 idiom, which is
> > why I used this form. I will spin up a v2 with your suggestion, thank
> > you for the review!
> 
> Feel free to fix up the others as well. The others were only changed
> due to the same warning you sent the patch for.
> 
> Alex
> 

Hi Alex,

Thank you for the information, I will do that in v2.

Thanks,
Nathan

> >
> > Nathan
> >
> > > >
> > > >                 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 72f8018fa2a8..a781a5027212 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);
> > > >
> > > > --
> > > > 2.18.0
> > > >
> > >
> > >
> > > --
> > > Thanks,
> > > ~Nick Desaulniers
> > _______________________________________________
> > amd-gfx mailing list
> > amd-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] drm/amdgpu: Add braces to initialize task_info subojects

[Nathan Chancellor]

On Wed, Sep 12, 2018 at 02:44:34PM -0400, Alex Deucher wrote:
> On Wed, Sep 12, 2018 at 2:40 PM Nathan Chancellor
> <natechancellor@gmail.com> wrote:
> >
> > On Wed, Sep 12, 2018 at 10:38:30AM -0700, Nick Desaulniers wrote:
> > > On Tue, Sep 11, 2018 at 5:26 PM Nathan Chancellor
> > > <natechancellor@gmail.com> wrote:
> > > >
> > > > Clang warns if there are missing braces around a subobject
> > > > initializer.
> > > >
> > > > drivers/gpu/drm/amd/amdgpu/gmc_v8_0.c:1447:41: warning: suggest braces
> > > > around initialization of subobject [-Wmissing-braces]
> > > >                 struct amdgpu_task_info task_info = { 0 };
> > > >                                                       ^
> > > >                                                       {}
> > > > 1 warning generated.
> > > >
> > > > drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c:262:41: warning: suggest braces
> > > > around initialization of subobject [-Wmissing-braces]
> > > >                 struct amdgpu_task_info task_info = { 0 };
> > > >                                                       ^
> > > >                                                       {}
> > > > 1 warning generated.
> > > >
> > > > Reported-by: Nick Desaulniers <ndesaulniers@google.com>
> > > > Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
> > > > ---
> > > >  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 9333109b210d..968cc1b8cdff 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 } };
> > >
> > > Hi Nathan,
> > > Thanks for this patch.  I discussed this syntax with our language
> > > lawyers.  Turns out, this is not quite correct, as you're now saying
> > > "initialize the first subobject to zero, but not the rest of the
> > > object."  -Wmissing-field-initializers would highlight this, but it's
> > > not part of -Wall.  It would be more correct to zero initialize the
> > > full struct, including all of its subobjects with `= {};`.
> > >
> >
> > Good point, I was debating on which one was correct. There are several
> > places in this driver that use the multiple brace + 0 idiom, which is
> > why I used this form. I will spin up a v2 with your suggestion, thank
> > you for the review!
> 
> Feel free to fix up the others as well. The others were only changed
> due to the same warning you sent the patch for.
> 
> Alex
> 

Hi Alex,

Thank you for the information, I will do that in v2.

Thanks,
Nathan

> >
> > Nathan
> >
> > > >
> > > >                 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 72f8018fa2a8..a781a5027212 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);
> > > >
> > > > --
> > > > 2.18.0
> > > >
> > >
> > >
> > > --
> > > Thanks,
> > > ~Nick Desaulniers
> > _______________________________________________
> > amd-gfx mailing list
> > amd-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] drm/amdgpu: Add braces to initialize task_info subojects

[Richard Smith]

[-- Attachment #1.1: Type: text/plain, Size: 3901 bytes --]

On Wed, Sep 12, 2018 at 10:38 AM Nick Desaulniers <ndesaulniers-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
wrote:

> On Tue, Sep 11, 2018 at 5:26 PM Nathan Chancellor
> <natechancellor-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> >
> > Clang warns if there are missing braces around a subobject
> > initializer.
> >
> > drivers/gpu/drm/amd/amdgpu/gmc_v8_0.c:1447:41: warning: suggest braces
> > around initialization of subobject [-Wmissing-braces]
> >                 struct amdgpu_task_info task_info = { 0 };
> >                                                       ^
> >                                                       {}
> > 1 warning generated.
> >
> > drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c:262:41: warning: suggest braces
> > around initialization of subobject [-Wmissing-braces]
> >                 struct amdgpu_task_info task_info = { 0 };
> >                                                       ^
> >                                                       {}
> > 1 warning generated.
> >
> > Reported-by: Nick Desaulniers <ndesaulniers-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
> > Signed-off-by: Nathan Chancellor <natechancellor-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> > ---
> >  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 9333109b210d..968cc1b8cdff 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 } };
>
> Hi Nathan,
> Thanks for this patch.  I discussed this syntax with our language
> lawyers.  Turns out, this is not quite correct, as you're now saying
> "initialize the first subobject to zero, but not the rest of the
> object."  -Wmissing-field-initializers would highlight this, but it's
> not part of -Wall.  It would be more correct to zero initialize the
> full struct, including all of its subobjects with `= {};`.
>

Sorry, I think I've caused some confusion here.

Elements with an omitted initializer get implicitly zero-initialized. In
C++, it's idiomatic to write `= {}` to perform aggregate
zero-initialization, but in C, that's invalid because at least one
initializer is syntactically required within the braces. As a result, `=
{0}` is an idiomatic way to perform zero-initialization of an aggregate in
C. Clang intends to suppress the -Wmissing-braces in that case; if the
warning is still being produced in a recent version of Clang, that's a bug.
However, the warning suppression was added between Clang 5 and Clang 6, so
it's very plausible that the compiler being used here is simply older than
the warning fix.

(Long story short: the change here seems fine, but should be unnecessary as
of Clang 6.)


> >                 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 72f8018fa2a8..a781a5027212 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);
> >
> > --
> > 2.18.0
> >
>
>
> --
> Thanks,
> ~Nick Desaulniers
>

[-- Attachment #1.2: Type: text/html, Size: 5197 bytes --]

[-- Attachment #2: Type: text/plain, Size: 154 bytes --]

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] drm/amdgpu: Add braces to initialize task_info subojects

[Nathan Chancellor]

On Wed, Sep 12, 2018 at 01:24:34PM -0700, Richard Smith wrote:
> On Wed, Sep 12, 2018 at 10:38 AM Nick Desaulniers <ndesaulniers@google.com>
> wrote:
> 
> > On Tue, Sep 11, 2018 at 5:26 PM Nathan Chancellor
> > <natechancellor@gmail.com> wrote:
> > >
> > > Clang warns if there are missing braces around a subobject
> > > initializer.
> > >
> > > drivers/gpu/drm/amd/amdgpu/gmc_v8_0.c:1447:41: warning: suggest braces
> > > around initialization of subobject [-Wmissing-braces]
> > >                 struct amdgpu_task_info task_info = { 0 };
> > >                                                       ^
> > >                                                       {}
> > > 1 warning generated.
> > >
> > > drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c:262:41: warning: suggest braces
> > > around initialization of subobject [-Wmissing-braces]
> > >                 struct amdgpu_task_info task_info = { 0 };
> > >                                                       ^
> > >                                                       {}
> > > 1 warning generated.
> > >
> > > Reported-by: Nick Desaulniers <ndesaulniers@google.com>
> > > Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
> > > ---
> > >  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 9333109b210d..968cc1b8cdff 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 } };
> >
> > Hi Nathan,
> > Thanks for this patch.  I discussed this syntax with our language
> > lawyers.  Turns out, this is not quite correct, as you're now saying
> > "initialize the first subobject to zero, but not the rest of the
> > object."  -Wmissing-field-initializers would highlight this, but it's
> > not part of -Wall.  It would be more correct to zero initialize the
> > full struct, including all of its subobjects with `= {};`.
> >
> 
> Sorry, I think I've caused some confusion here.
> 
> Elements with an omitted initializer get implicitly zero-initialized. In
> C++, it's idiomatic to write `= {}` to perform aggregate
> zero-initialization, but in C, that's invalid because at least one
> initializer is syntactically required within the braces. As a result, `=
> {0}` is an idiomatic way to perform zero-initialization of an aggregate in
> C. Clang intends to suppress the -Wmissing-braces in that case; if the
> warning is still being produced in a recent version of Clang, that's a bug.
> However, the warning suppression was added between Clang 5 and Clang 6, so
> it's very plausible that the compiler being used here is simply older than
> the warning fix.
> 
> (Long story short: the change here seems fine, but should be unnecessary as
> of Clang 6.)
> 

Interesting...

nathan@flashbox ~/kernels/next (master >) $ clang --version | head -n1
clang version 6.0.1 (tags/RELEASE_601/final)

I guess the v2 I sent is unnecessary then. I'll leave it up to the
maintainers to decide which one they want to take.

Thanks!
Nathan

> 
> > >                 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 72f8018fa2a8..a781a5027212 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);
> > >
> > > --
> > > 2.18.0
> > >
> >
> >
> > --
> > Thanks,
> > ~Nick Desaulniers
> >

Re: [PATCH] drm/amdgpu: Add braces to initialize task_info subojects

[Nick Desaulniers]

On Wed, Sep 12, 2018 at 1:24 PM Richard Smith <richardsmith@google.com> wrote:
>
> On Wed, Sep 12, 2018 at 10:38 AM Nick Desaulniers <ndesaulniers@google.com> wrote:
>>
>> On Tue, Sep 11, 2018 at 5:26 PM Nathan Chancellor
>> <natechancellor@gmail.com> wrote:
>> >
>> > Clang warns if there are missing braces around a subobject
>> > initializer.
>> >
>> > drivers/gpu/drm/amd/amdgpu/gmc_v8_0.c:1447:41: warning: suggest braces
>> > around initialization of subobject [-Wmissing-braces]
>> >                 struct amdgpu_task_info task_info = { 0 };
>> >                                                       ^
>> >                                                       {}
>> > 1 warning generated.
>> >
>> > drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c:262:41: warning: suggest braces
>> > around initialization of subobject [-Wmissing-braces]
>> >                 struct amdgpu_task_info task_info = { 0 };
>> >                                                       ^
>> >                                                       {}
>> > 1 warning generated.
>> >
>> > Reported-by: Nick Desaulniers <ndesaulniers@google.com>
>> > Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
>> > ---
>> >  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 9333109b210d..968cc1b8cdff 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 } };
>>
>> Hi Nathan,
>> Thanks for this patch.  I discussed this syntax with our language
>> lawyers.  Turns out, this is not quite correct, as you're now saying
>> "initialize the first subobject to zero, but not the rest of the
>> object."  -Wmissing-field-initializers would highlight this, but it's
>> not part of -Wall.  It would be more correct to zero initialize the
>> full struct, including all of its subobjects with `= {};`.
>
>
> Sorry, I think I've caused some confusion here.
>
> Elements with an omitted initializer get implicitly zero-initialized. In C++, it's idiomatic to write `= {}` to perform aggregate zero-initialization, but in C, that's invalid because at least one initializer is syntactically required within the braces. As a result, `= {0}` is an idiomatic way to perform zero-initialization of an aggregate in C.

That doesn't seem to be the case:
https://godbolt.org/z/TZzfo6 shouldn't Clang warn in the case of bar()?

> Clang intends to suppress the -Wmissing-braces in that case; if the warning is still being produced in a recent version of Clang, that's a bug. However, the warning suppression was added between Clang 5 and Clang 6, so it's very plausible that the compiler being used here is simply older than the warning fix.
>
> (Long story short: the change here seems fine, but should be unnecessary as of Clang 6.)

The warning was identified from clang-8 ToT synced yesterday.

-- 
Thanks,
~Nick Desaulniers

Re: [PATCH] drm/amdgpu: Add braces to initialize task_info subojects

[Richard Smith]

[-- Attachment #1.1: Type: text/plain, Size: 4055 bytes --]

On Wed, Sep 12, 2018 at 4:13 PM Nick Desaulniers <ndesaulniers-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
wrote:

> On Wed, Sep 12, 2018 at 1:24 PM Richard Smith <richardsmith-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
> wrote:
> >
> > On Wed, Sep 12, 2018 at 10:38 AM Nick Desaulniers <
> ndesaulniers-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org> wrote:
> >>
> >> On Tue, Sep 11, 2018 at 5:26 PM Nathan Chancellor
> >> <natechancellor-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> >> >
> >> > Clang warns if there are missing braces around a subobject
> >> > initializer.
> >> >
> >> > drivers/gpu/drm/amd/amdgpu/gmc_v8_0.c:1447:41: warning: suggest braces
> >> > around initialization of subobject [-Wmissing-braces]
> >> >                 struct amdgpu_task_info task_info = { 0 };
> >> >                                                       ^
> >> >                                                       {}
> >> > 1 warning generated.
> >> >
> >> > drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c:262:41: warning: suggest braces
> >> > around initialization of subobject [-Wmissing-braces]
> >> >                 struct amdgpu_task_info task_info = { 0 };
> >> >                                                       ^
> >> >                                                       {}
> >> > 1 warning generated.
> >> >
> >> > Reported-by: Nick Desaulniers <ndesaulniers-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
> >> > Signed-off-by: Nathan Chancellor <natechancellor-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> >> > ---
> >> >  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 9333109b210d..968cc1b8cdff 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 } };
> >>
> >> Hi Nathan,
> >> Thanks for this patch.  I discussed this syntax with our language
> >> lawyers.  Turns out, this is not quite correct, as you're now saying
> >> "initialize the first subobject to zero, but not the rest of the
> >> object."  -Wmissing-field-initializers would highlight this, but it's
> >> not part of -Wall.  It would be more correct to zero initialize the
> >> full struct, including all of its subobjects with `= {};`.
> >
> >
> > Sorry, I think I've caused some confusion here.
> >
> > Elements with an omitted initializer get implicitly zero-initialized. In
> C++, it's idiomatic to write `= {}` to perform aggregate
> zero-initialization, but in C, that's invalid because at least one
> initializer is syntactically required within the braces. As a result, `=
> {0}` is an idiomatic way to perform zero-initialization of an aggregate in
> C.
>
> That doesn't seem to be the case:
> https://godbolt.org/z/TZzfo6 shouldn't Clang warn in the case of bar()?
>

This is a GNU extension. Use -pedantic-errors to turn off extensions, then
Clang and GCC reject bar(): https://godbolt.org/z/pIzI6M


> > Clang intends to suppress the -Wmissing-braces in that case; if the
> warning is still being produced in a recent version of Clang, that's a bug.
> However, the warning suppression was added between Clang 5 and Clang 6, so
> it's very plausible that the compiler being used here is simply older than
> the warning fix.
> >
> > (Long story short: the change here seems fine, but should be unnecessary
> as of Clang 6.)
>
> The warning was identified from clang-8 ToT synced yesterday.
>

Thanks for the testcase. This is a Clang bug. Apparently the warning
suppression only works when the first member is of type 'int'!
https://godbolt.org/z/sxnZvv

[-- Attachment #1.2: Type: text/html, Size: 5983 bytes --]

[-- Attachment #2: Type: text/plain, Size: 154 bytes --]

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx