| Message ID | 20180912002559.1853-1-natechancellor@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/amdgpu: Add braces to initialize task_info subojects | expand |
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 >
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
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
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
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.) > > 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 > <div dir="ltr"><div class="gmail_quote"><div dir="ltr">On Wed, Sep 12, 2018 at 10:38 AM Nick Desaulniers <<a href="mailto:ndesaulniers@google.com">ndesaulniers@google.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">On Tue, Sep 11, 2018 at 5:26 PM Nathan Chancellor<br> <<a href="mailto:natechancellor@gmail.com" target="_blank">natechancellor@gmail.com</a>> wrote:<br> ><br> > Clang warns if there are missing braces around a subobject<br> > initializer.<br> ><br> > drivers/gpu/drm/amd/amdgpu/gmc_v8_0.c:1447:41: warning: suggest braces<br> > around initialization of subobject [-Wmissing-braces]<br> > struct amdgpu_task_info task_info = { 0 };<br> > ^<br> > {}<br> > 1 warning generated.<br> ><br> > drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c:262:41: warning: suggest braces<br> > around initialization of subobject [-Wmissing-braces]<br> > struct amdgpu_task_info task_info = { 0 };<br> > ^<br> > {}<br> > 1 warning generated.<br> ><br> > Reported-by: Nick Desaulniers <<a href="mailto:ndesaulniers@google.com" target="_blank">ndesaulniers@google.com</a>><br> > Signed-off-by: Nathan Chancellor <<a href="mailto:natechancellor@gmail.com" target="_blank">natechancellor@gmail.com</a>><br> > ---<br> > drivers/gpu/drm/amd/amdgpu/gmc_v8_0.c | 2 +-<br> > drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c | 2 +-<br> > 2 files changed, 2 insertions(+), 2 deletions(-)<br> ><br> > diff --git a/drivers/gpu/drm/amd/amdgpu/gmc_v8_0.c b/drivers/gpu/drm/amd/amdgpu/gmc_v8_0.c<br> > index 9333109b210d..968cc1b8cdff 100644<br> > --- a/drivers/gpu/drm/amd/amdgpu/gmc_v8_0.c<br> > +++ b/drivers/gpu/drm/amd/amdgpu/gmc_v8_0.c<br> > @@ -1444,7 +1444,7 @@ static int gmc_v8_0_process_interrupt(struct amdgpu_device *adev,<br> > gmc_v8_0_set_fault_enable_default(adev, false);<br> ><br> > if (printk_ratelimit()) {<br> > - struct amdgpu_task_info task_info = { 0 };<br> > + struct amdgpu_task_info task_info = { { 0 } };<br> <br> Hi Nathan,<br> Thanks for this patch. I discussed this syntax with our language<br> lawyers. Turns out, this is not quite correct, as you're now saying<br> "initialize the first subobject to zero, but not the rest of the<br> object." -Wmissing-field-initializers would highlight this, but it's<br> not part of -Wall. It would be more correct to zero initialize the<br> full struct, including all of its subobjects with `= {};`.<br></blockquote><div><br></div><div>Sorry, I think I've caused some confusion here.</div><div><br></div><div>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.<br></div><div><br></div><div>(Long story short: the change here seems fine, but should be unnecessary as of Clang 6.)</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> > amdgpu_vm_get_task_info(adev, entry->pasid, &task_info);<br> ><br> > diff --git a/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c b/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c<br> > index 72f8018fa2a8..a781a5027212 100644<br> > --- a/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c<br> > +++ b/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c<br> > @@ -259,7 +259,7 @@ static int gmc_v9_0_process_interrupt(struct amdgpu_device *adev,<br> > }<br> ><br> > if (printk_ratelimit()) {<br> > - struct amdgpu_task_info task_info = { 0 };<br> > + struct amdgpu_task_info task_info = { { 0 } };<br> ><br> > amdgpu_vm_get_task_info(adev, entry->pasid, &task_info);<br> ><br> > --<br> > 2.18.0<br> ><br> <br> <br> -- <br> Thanks,<br> ~Nick Desaulniers<br> </blockquote></div></div>
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 > >
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.
On Wed, Sep 12, 2018 at 4:13 PM Nick Desaulniers <ndesaulniers@google.com> wrote: > 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()? > 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 <div dir="ltr"><div dir="ltr"><div dir="ltr"><div class="gmail_quote"><div dir="ltr">On Wed, Sep 12, 2018 at 4:13 PM Nick Desaulniers <<a href="mailto:ndesaulniers@google.com">ndesaulniers@google.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">On Wed, Sep 12, 2018 at 1:24 PM Richard Smith <<a href="mailto:richardsmith@google.com" target="_blank">richardsmith@google.com</a>> wrote:<br> ><br> > On Wed, Sep 12, 2018 at 10:38 AM Nick Desaulniers <<a href="mailto:ndesaulniers@google.com" target="_blank">ndesaulniers@google.com</a>> wrote:<br> >><br> >> On Tue, Sep 11, 2018 at 5:26 PM Nathan Chancellor<br> >> <<a href="mailto:natechancellor@gmail.com" target="_blank">natechancellor@gmail.com</a>> wrote:<br> >> ><br> >> > Clang warns if there are missing braces around a subobject<br> >> > initializer.<br> >> ><br> >> > drivers/gpu/drm/amd/amdgpu/gmc_v8_0.c:1447:41: warning: suggest braces<br> >> > around initialization of subobject [-Wmissing-braces]<br> >> > struct amdgpu_task_info task_info = { 0 };<br> >> > ^<br> >> > {}<br> >> > 1 warning generated.<br> >> ><br> >> > drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c:262:41: warning: suggest braces<br> >> > around initialization of subobject [-Wmissing-braces]<br> >> > struct amdgpu_task_info task_info = { 0 };<br> >> > ^<br> >> > {}<br> >> > 1 warning generated.<br> >> ><br> >> > Reported-by: Nick Desaulniers <<a href="mailto:ndesaulniers@google.com" target="_blank">ndesaulniers@google.com</a>><br> >> > Signed-off-by: Nathan Chancellor <<a href="mailto:natechancellor@gmail.com" target="_blank">natechancellor@gmail.com</a>><br> >> > ---<br> >> > drivers/gpu/drm/amd/amdgpu/gmc_v8_0.c | 2 +-<br> >> > drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c | 2 +-<br> >> > 2 files changed, 2 insertions(+), 2 deletions(-)<br> >> ><br> >> > diff --git a/drivers/gpu/drm/amd/amdgpu/gmc_v8_0.c b/drivers/gpu/drm/amd/amdgpu/gmc_v8_0.c<br> >> > index 9333109b210d..968cc1b8cdff 100644<br> >> > --- a/drivers/gpu/drm/amd/amdgpu/gmc_v8_0.c<br> >> > +++ b/drivers/gpu/drm/amd/amdgpu/gmc_v8_0.c<br> >> > @@ -1444,7 +1444,7 @@ static int gmc_v8_0_process_interrupt(struct amdgpu_device *adev,<br> >> > gmc_v8_0_set_fault_enable_default(adev, false);<br> >> ><br> >> > if (printk_ratelimit()) {<br> >> > - struct amdgpu_task_info task_info = { 0 };<br> >> > + struct amdgpu_task_info task_info = { { 0 } };<br> >><br> >> Hi Nathan,<br> >> Thanks for this patch. I discussed this syntax with our language<br> >> lawyers. Turns out, this is not quite correct, as you're now saying<br> >> "initialize the first subobject to zero, but not the rest of the<br> >> object." -Wmissing-field-initializers would highlight this, but it's<br> >> not part of -Wall. It would be more correct to zero initialize the<br> >> full struct, including all of its subobjects with `= {};`.<br> ><br> ><br> > Sorry, I think I've caused some confusion here.<br> ><br> > 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.<br> <br> That doesn't seem to be the case:<br> <a href="https://godbolt.org/z/TZzfo6" rel="noreferrer" target="_blank">https://godbolt.org/z/TZzfo6</a> shouldn't Clang warn in the case of bar()?<br></blockquote><div><br></div><div>This is a GNU extension. Use -pedantic-errors to turn off extensions, then Clang and GCC reject bar(): <a href="https://godbolt.org/z/pIzI6M">https://godbolt.org/z/pIzI6M</a></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> > 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.<br> ><br> > (Long story short: the change here seems fine, but should be unnecessary as of Clang 6.)<br> <br> The warning was identified from clang-8 ToT synced yesterday.<br></blockquote><div><br></div><div>Thanks for the testcase. This is a Clang bug. Apparently the warning suppression only works when the first member is of type 'int'! <a href="https://godbolt.org/z/sxnZvv">https://godbolt.org/z/sxnZvv</a></div></div></div></div></div>
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);
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(-)