[2/4] compiler: avoid 'unused variable' and 'may be used uninitialized' warnings

Submitted by andrey simiklit on Sept. 11, 2018, 12:42 p.m.

Details

Message ID 1536669727-22009-3-git-send-email-asimiklit.work@gmail.com
State New
Headers show
Series "mesa: fix against several compilation warnings" ( rev: 1 ) in Mesa

Not browsing as part of any series.

Commit Message

andrey simiklit Sept. 11, 2018, 12:42 p.m.
From: Andrii Simiklit <andrii.simiklit@globallogic.com>

1. nir/nir_lower_vars_to_ssa.c:691:21: warning:
       unused variable ‘var’
       nir_variable *var = path->path[0]->var;

2. nir_types.cpp:558:31: warning:
    ‘elem_align’ may be used uninitialized in this function
           unsigned elem_size, elem_align;
   nir_types.cpp:558:20: warning:
   ‘elem_size’ may be used uninitialized in this function
           unsigned elem_size, elem_align;

Signed-off-by: Andrii Simiklit <andrii.simiklit@globallogic.com>
---
 src/compiler/nir/nir_lower_vars_to_ssa.c | 2 +-
 src/compiler/nir_types.cpp               | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

Patch hide | download patch | download mbox

diff --git a/src/compiler/nir/nir_lower_vars_to_ssa.c b/src/compiler/nir/nir_lower_vars_to_ssa.c
index cd679be..96de261 100644
--- a/src/compiler/nir/nir_lower_vars_to_ssa.c
+++ b/src/compiler/nir/nir_lower_vars_to_ssa.c
@@ -688,7 +688,7 @@  nir_lower_vars_to_ssa_impl(nir_function_impl *impl)
       nir_deref_path *path = &node->path;
 
       assert(path->path[0]->deref_type == nir_deref_type_var);
-      nir_variable *var = path->path[0]->var;
+      MAYBE_UNUSED nir_variable *var = path->path[0]->var;
 
       /* We don't build deref nodes for non-local variables */
       assert(var->data.mode == nir_var_local);
diff --git a/src/compiler/nir_types.cpp b/src/compiler/nir_types.cpp
index d24f094..d39d87b 100644
--- a/src/compiler/nir_types.cpp
+++ b/src/compiler/nir_types.cpp
@@ -542,7 +542,7 @@  glsl_get_natural_size_align_bytes(const struct glsl_type *type,
    }
 
    case GLSL_TYPE_ARRAY: {
-      unsigned elem_size, elem_align;
+      unsigned elem_size = 0, elem_align = 0;
       glsl_get_natural_size_align_bytes(type->fields.array,
                                         &elem_size, &elem_align);
       *align = elem_align;
@@ -554,7 +554,7 @@  glsl_get_natural_size_align_bytes(const struct glsl_type *type,
       *size = 0;
       *align = 0;
       for (unsigned i = 0; i < type->length; i++) {
-         unsigned elem_size, elem_align;
+         unsigned elem_size = 0, elem_align = 0;
          glsl_get_natural_size_align_bytes(type->fields.structure[i].type,
                                            &elem_size, &elem_align);
          *align = MAX2(*align, elem_align);

Comments

On Tuesday, 2018-09-11 15:42:05 +0300, asimiklit.work@gmail.com wrote:
> From: Andrii Simiklit <andrii.simiklit@globallogic.com>
> 
> 1. nir/nir_lower_vars_to_ssa.c:691:21: warning:
>        unused variable ‘var’
>        nir_variable *var = path->path[0]->var;
> 
> 2. nir_types.cpp:558:31: warning:
>     ‘elem_align’ may be used uninitialized in this function
>            unsigned elem_size, elem_align;
>    nir_types.cpp:558:20: warning:
>    ‘elem_size’ may be used uninitialized in this function
>            unsigned elem_size, elem_align;
> 
> Signed-off-by: Andrii Simiklit <andrii.simiklit@globallogic.com>
> ---
>  src/compiler/nir/nir_lower_vars_to_ssa.c | 2 +-
>  src/compiler/nir_types.cpp               | 4 ++--
>  2 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/src/compiler/nir/nir_lower_vars_to_ssa.c b/src/compiler/nir/nir_lower_vars_to_ssa.c
> index cd679be..96de261 100644
> --- a/src/compiler/nir/nir_lower_vars_to_ssa.c
> +++ b/src/compiler/nir/nir_lower_vars_to_ssa.c
> @@ -688,7 +688,7 @@ nir_lower_vars_to_ssa_impl(nir_function_impl *impl)
>        nir_deref_path *path = &node->path;
>  
>        assert(path->path[0]->deref_type == nir_deref_type_var);
> -      nir_variable *var = path->path[0]->var;
> +      MAYBE_UNUSED nir_variable *var = path->path[0]->var;
>  
>        /* We don't build deref nodes for non-local variables */
>        assert(var->data.mode == nir_var_local);

Sure, or you could simply remove the single-use extra local variable and
just fold it into the assert :)

> diff --git a/src/compiler/nir_types.cpp b/src/compiler/nir_types.cpp
> index d24f094..d39d87b 100644
> --- a/src/compiler/nir_types.cpp
> +++ b/src/compiler/nir_types.cpp
> @@ -542,7 +542,7 @@ glsl_get_natural_size_align_bytes(const struct glsl_type *type,
>     }
>  
>     case GLSL_TYPE_ARRAY: {
> -      unsigned elem_size, elem_align;
> +      unsigned elem_size = 0, elem_align = 0;

Not really keen on these ones; it looks like your compiler is ignoring the
unreachable() in glsl_get_natural_size_align_bytes() and mis-computing the
possible code-paths, resulting in it wrongly printing a warning.

I'd prefer to keep these two hunks as is, so that real issue in
glsl_get_natural_size_align_bytes() would get flagged appropriately.

>        glsl_get_natural_size_align_bytes(type->fields.array,
>                                          &elem_size, &elem_align);
>        *align = elem_align;
> @@ -554,7 +554,7 @@ glsl_get_natural_size_align_bytes(const struct glsl_type *type,
>        *size = 0;
>        *align = 0;
>        for (unsigned i = 0; i < type->length; i++) {
> -         unsigned elem_size, elem_align;
> +         unsigned elem_size = 0, elem_align = 0;
>           glsl_get_natural_size_align_bytes(type->fields.structure[i].type,
>                                             &elem_size, &elem_align);
>           *align = MAX2(*align, elem_align);
> -- 
> 2.7.4
> 
> _______________________________________________
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Hello,

Thanks for review.
Please find my comments below:

On Fri, Nov 9, 2018 at 2:52 PM Eric Engestrom <eric.engestrom@intel.com>
wrote:

> On Tuesday, 2018-09-11 15:42:05 +0300, asimiklit.work@gmail.com wrote:
> > From: Andrii Simiklit <andrii.simiklit@globallogic.com>
> >
> > 1. nir/nir_lower_vars_to_ssa.c:691:21: warning:
> >        unused variable ‘var’
> >        nir_variable *var = path->path[0]->var;
> >
> > 2. nir_types.cpp:558:31: warning:
> >     ‘elem_align’ may be used uninitialized in this function
> >            unsigned elem_size, elem_align;
> >    nir_types.cpp:558:20: warning:
> >    ‘elem_size’ may be used uninitialized in this function
> >            unsigned elem_size, elem_align;
> >
> > Signed-off-by: Andrii Simiklit <andrii.simiklit@globallogic.com>
> > ---
> >  src/compiler/nir/nir_lower_vars_to_ssa.c | 2 +-
> >  src/compiler/nir_types.cpp               | 4 ++--
> >  2 files changed, 3 insertions(+), 3 deletions(-)
> >
> > diff --git a/src/compiler/nir/nir_lower_vars_to_ssa.c
> b/src/compiler/nir/nir_lower_vars_to_ssa.c
> > index cd679be..96de261 100644
> > --- a/src/compiler/nir/nir_lower_vars_to_ssa.c
> > +++ b/src/compiler/nir/nir_lower_vars_to_ssa.c
> > @@ -688,7 +688,7 @@ nir_lower_vars_to_ssa_impl(nir_function_impl *impl)
> >        nir_deref_path *path = &node->path;
> >
> >        assert(path->path[0]->deref_type == nir_deref_type_var);
> > -      nir_variable *var = path->path[0]->var;
> > +      MAYBE_UNUSED nir_variable *var = path->path[0]->var;
> >
> >        /* We don't build deref nodes for non-local variables */
> >        assert(var->data.mode == nir_var_local);
>
> Sure, or you could simply remove the single-use extra local variable and
> just fold it into the assert :)
>

I agree,will fix :)


>
> > diff --git a/src/compiler/nir_types.cpp b/src/compiler/nir_types.cpp
> > index d24f094..d39d87b 100644
> > --- a/src/compiler/nir_types.cpp
> > +++ b/src/compiler/nir_types.cpp
> > @@ -542,7 +542,7 @@ glsl_get_natural_size_align_bytes(const struct
> glsl_type *type,
> >     }
> >
> >     case GLSL_TYPE_ARRAY: {
> > -      unsigned elem_size, elem_align;
> > +      unsigned elem_size = 0, elem_align = 0;
>
> Not really keen on these ones; it looks like your compiler is ignoring the
> unreachable() in glsl_get_natural_size_align_bytes() and mis-computing the
> possible code-paths, resulting in it wrongly printing a warning.
>

You are right when I added the 'default:' case to the switch the warning is
disappeared:

case GLSL_TYPE_INTERFACE:
case GLSL_TYPE_FUNCTION:
+default:
   unreachable("type does not have a natural size");

What do you think about such solution? Is it acceptable for you?


> I'd prefer to keep these two hunks as is, so that real issue in
> glsl_get_natural_size_align_bytes() would get flagged appropriately.
>
> >        glsl_get_natural_size_align_bytes(type->fields.array,
> >                                          &elem_size, &elem_align);
> >        *align = elem_align;
> > @@ -554,7 +554,7 @@ glsl_get_natural_size_align_bytes(const struct
> glsl_type *type,
> >        *size = 0;
> >        *align = 0;
> >        for (unsigned i = 0; i < type->length; i++) {
> > -         unsigned elem_size, elem_align;
> > +         unsigned elem_size = 0, elem_align = 0;
> >
>  glsl_get_natural_size_align_bytes(type->fields.structure[i].type,
> >                                             &elem_size, &elem_align);
> >           *align = MAX2(*align, elem_align);
> > --
> > 2.7.4
> >
> > _______________________________________________
> > mesa-dev mailing list
> > mesa-dev@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
On Friday, 2018-11-09 16:15:12 +0200, andrey simiklit wrote:
> Hello,
> 
> Thanks for review.
> Please find my comments below:
> 
> On Fri, Nov 9, 2018 at 2:52 PM Eric Engestrom <eric.engestrom@intel.com>
> wrote:
> 
> > On Tuesday, 2018-09-11 15:42:05 +0300, asimiklit.work@gmail.com wrote:
> > > From: Andrii Simiklit <andrii.simiklit@globallogic.com>
> > >
> > > 1. nir/nir_lower_vars_to_ssa.c:691:21: warning:
> > >        unused variable ‘var’
> > >        nir_variable *var = path->path[0]->var;
> > >
> > > 2. nir_types.cpp:558:31: warning:
> > >     ‘elem_align’ may be used uninitialized in this function
> > >            unsigned elem_size, elem_align;
> > >    nir_types.cpp:558:20: warning:
> > >    ‘elem_size’ may be used uninitialized in this function
> > >            unsigned elem_size, elem_align;
> > >
> > > Signed-off-by: Andrii Simiklit <andrii.simiklit@globallogic.com>
> > > ---
> > >  src/compiler/nir/nir_lower_vars_to_ssa.c | 2 +-
> > >  src/compiler/nir_types.cpp               | 4 ++--
> > >  2 files changed, 3 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/src/compiler/nir/nir_lower_vars_to_ssa.c
> > b/src/compiler/nir/nir_lower_vars_to_ssa.c
> > > index cd679be..96de261 100644
> > > --- a/src/compiler/nir/nir_lower_vars_to_ssa.c
> > > +++ b/src/compiler/nir/nir_lower_vars_to_ssa.c
> > > @@ -688,7 +688,7 @@ nir_lower_vars_to_ssa_impl(nir_function_impl *impl)
> > >        nir_deref_path *path = &node->path;
> > >
> > >        assert(path->path[0]->deref_type == nir_deref_type_var);
> > > -      nir_variable *var = path->path[0]->var;
> > > +      MAYBE_UNUSED nir_variable *var = path->path[0]->var;
> > >
> > >        /* We don't build deref nodes for non-local variables */
> > >        assert(var->data.mode == nir_var_local);
> >
> > Sure, or you could simply remove the single-use extra local variable and
> > just fold it into the assert :)
> >
> 
> I agree,will fix :)
> 
> 
> >
> > > diff --git a/src/compiler/nir_types.cpp b/src/compiler/nir_types.cpp
> > > index d24f094..d39d87b 100644
> > > --- a/src/compiler/nir_types.cpp
> > > +++ b/src/compiler/nir_types.cpp
> > > @@ -542,7 +542,7 @@ glsl_get_natural_size_align_bytes(const struct
> > glsl_type *type,
> > >     }
> > >
> > >     case GLSL_TYPE_ARRAY: {
> > > -      unsigned elem_size, elem_align;
> > > +      unsigned elem_size = 0, elem_align = 0;
> >
> > Not really keen on these ones; it looks like your compiler is ignoring the
> > unreachable() in glsl_get_natural_size_align_bytes() and mis-computing the
> > possible code-paths, resulting in it wrongly printing a warning.
> >
> 
> You are right when I added the 'default:' case to the switch the warning is
> disappeared:
> 
> case GLSL_TYPE_INTERFACE:
> case GLSL_TYPE_FUNCTION:
> +default:
>    unreachable("type does not have a natural size");
> 
> What do you think about such solution? Is it acceptable for you?

Is there any possible value of `enum glsl_base_type` that is not handled
by the switch? If so, they should be added. Otherwise, this is
a compiler bug, not a code bug :)

> 
> 
> > I'd prefer to keep these two hunks as is, so that real issue in
> > glsl_get_natural_size_align_bytes() would get flagged appropriately.
> >
> > >        glsl_get_natural_size_align_bytes(type->fields.array,
> > >                                          &elem_size, &elem_align);
> > >        *align = elem_align;
> > > @@ -554,7 +554,7 @@ glsl_get_natural_size_align_bytes(const struct
> > glsl_type *type,
> > >        *size = 0;
> > >        *align = 0;
> > >        for (unsigned i = 0; i < type->length; i++) {
> > > -         unsigned elem_size, elem_align;
> > > +         unsigned elem_size = 0, elem_align = 0;
> > >
> >  glsl_get_natural_size_align_bytes(type->fields.structure[i].type,
> > >                                             &elem_size, &elem_align);
> > >           *align = MAX2(*align, elem_align);
> > > --
> > > 2.7.4
> > >
> > > _______________________________________________
> > > mesa-dev mailing list
> > > mesa-dev@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/mesa-dev
> >