Re: [Mesa-dev] [PATCH v2 05/32] glsl: Zero per_vertex_accumulator::fields for valgrind & nir_serialize

[Kenneth Graunke]

On Friday, October 20, 2017 6:52:11 PM PDT Jordan Justen wrote:
> I'm now doubting the uninitialized padding explanation. I tried the
> memset in the glsl_struct_field constructor as Ken mentioned, but I
> also tried adding initializers for the fields in the default
> constructor, and it fixed valgrind.
> 
> I didn't find anything mentioning that a default constructor on a c++
> struct requires the fields to be explicitly initialized. I would think
> that each field would be default initialized, the same as for a c++
> class.
> 
> Anyway, here is alternate patch that fixes valgrind. Is this
> preferable to a memset in the constructor? Is either Reviewed-by
> worthy?
> 
> diff --git a/src/compiler/glsl_types.h b/src/compiler/glsl_types.h
> index b5e97e638b..0b4a66ca4d 100644
> --- a/src/compiler/glsl_types.h
> +++ b/src/compiler/glsl_types.h
> @@ -1045,6 +1045,13 @@ struct glsl_struct_field {
> }
>  
> glsl_struct_field()
> +  : type(NULL), name(NULL), location(0), offset(0), xfb_buffer(0),
> +xfb_stride(0), interpolation(0), centroid(0),
> +sample(0), matrix_layout(0), patch(0),
> +precision(0), memory_read_only(0),
> +memory_write_only(0), memory_coherent(0), memory_volatile(0),
> +memory_restrict(0), image_format(0), explicit_xfb_buffer(0),
> +implicit_sized_array(0)
> {
>/* empty */
> }
> 
> -Jordan

This looks good to me, initializing members in the constructor is
very reasonable.  I like this better.

Reviewed-by: Kenneth Graunke 


signature.asc
Description: This is a digitally signed message part.
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH v2 05/32] glsl: Zero per_vertex_accumulator::fields for valgrind & nir_serialize

[Ilia Mirkin]

On Fri, Oct 20, 2017 at 9:52 PM, Jordan Justen
 wrote:
> On 2017-10-20 14:14:25, Jason Ekstrand wrote:
>> On Thu, Oct 19, 2017 at 4:51 PM, Kenneth Graunke 
>> wrote:
>>
>> > On Wednesday, October 18, 2017 10:31:53 PM PDT Jordan Justen wrote:
>> > > Signed-off-by: Jordan Justen 
>> > > ---
>> > >  src/compiler/glsl/builtin_variables.cpp | 1 +
>> > >  1 file changed, 1 insertion(+)
>> > >
>> > > diff --git a/src/compiler/glsl/builtin_variables.cpp
>> > b/src/compiler/glsl/builtin_variables.cpp
>> > > index ea2d897cc8..d3cf12475b 100644
>> > > --- a/src/compiler/glsl/builtin_variables.cpp
>> > > +++ b/src/compiler/glsl/builtin_variables.cpp
>> > > @@ -318,6 +318,7 @@ per_vertex_accumulator::per_vertex_accumulator()
>> > > : fields(),
>> > >   num_fields(0)
>> > >  {
>> > > +   memset(fields, 0, sizeof(fields));
>> > >  }
>> >
>> > This shouldn't fix anything, we're calling the fields() constructor,
>> > which should value-initialize every element of the fields array,
>> > calling the constructor for glsl_struct_type on each element.
>> >
>> > That constructor is supposed to zero initialize items.  Maybe it is
>> > missing some of them?
>> >
>>
>> No, the problem is that, when we do the user data check in the blob code,
>> valgrind complains about the padding bytes in-between field elements.  C++
>> constructors don't initialize that data.  That said, I agree that this is a
>> tad sketchy.  I just wish there were some way to shut up valgrind.
>>
>
> I'm now doubting the uninitialized padding explanation. I tried the
> memset in the glsl_struct_field constructor as Ken mentioned, but I
> also tried adding initializers for the fields in the default
> constructor, and it fixed valgrind.
>
> I didn't find anything mentioning that a default constructor on a c++
> struct requires the fields to be explicitly initialized. I would think
> that each field would be default initialized, the same as for a c++
> class.

struct and class are the same (just different default private/public
membership). However I seem to recall that there are oddities
regarding POD types and default init. Pretty sure you have to add
initializers, or a memset.

>
> Anyway, here is alternate patch that fixes valgrind. Is this
> preferable to a memset in the constructor? Is either Reviewed-by
> worthy?
>
> diff --git a/src/compiler/glsl_types.h b/src/compiler/glsl_types.h
> index b5e97e638b..0b4a66ca4d 100644
> --- a/src/compiler/glsl_types.h
> +++ b/src/compiler/glsl_types.h
> @@ -1045,6 +1045,13 @@ struct glsl_struct_field {
> }
>
> glsl_struct_field()
> +  : type(NULL), name(NULL), location(0), offset(0), xfb_buffer(0),
> +xfb_stride(0), interpolation(0), centroid(0),
> +sample(0), matrix_layout(0), patch(0),
> +precision(0), memory_read_only(0),
> +memory_write_only(0), memory_coherent(0), memory_volatile(0),
> +memory_restrict(0), image_format(0), explicit_xfb_buffer(0),
> +implicit_sized_array(0)
> {
>/* empty */
> }
>
> -Jordan
> ___
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH v2 05/32] glsl: Zero per_vertex_accumulator::fields for valgrind & nir_serialize

[Jordan Justen]

On 2017-10-20 14:14:25, Jason Ekstrand wrote:
> On Thu, Oct 19, 2017 at 4:51 PM, Kenneth Graunke 
> wrote:
> 
> > On Wednesday, October 18, 2017 10:31:53 PM PDT Jordan Justen wrote:
> > > Signed-off-by: Jordan Justen 
> > > ---
> > >  src/compiler/glsl/builtin_variables.cpp | 1 +
> > >  1 file changed, 1 insertion(+)
> > >
> > > diff --git a/src/compiler/glsl/builtin_variables.cpp
> > b/src/compiler/glsl/builtin_variables.cpp
> > > index ea2d897cc8..d3cf12475b 100644
> > > --- a/src/compiler/glsl/builtin_variables.cpp
> > > +++ b/src/compiler/glsl/builtin_variables.cpp
> > > @@ -318,6 +318,7 @@ per_vertex_accumulator::per_vertex_accumulator()
> > > : fields(),
> > >   num_fields(0)
> > >  {
> > > +   memset(fields, 0, sizeof(fields));
> > >  }
> >
> > This shouldn't fix anything, we're calling the fields() constructor,
> > which should value-initialize every element of the fields array,
> > calling the constructor for glsl_struct_type on each element.
> >
> > That constructor is supposed to zero initialize items.  Maybe it is
> > missing some of them?
> >
> 
> No, the problem is that, when we do the user data check in the blob code,
> valgrind complains about the padding bytes in-between field elements.  C++
> constructors don't initialize that data.  That said, I agree that this is a
> tad sketchy.  I just wish there were some way to shut up valgrind.
> 

I'm now doubting the uninitialized padding explanation. I tried the
memset in the glsl_struct_field constructor as Ken mentioned, but I
also tried adding initializers for the fields in the default
constructor, and it fixed valgrind.

I didn't find anything mentioning that a default constructor on a c++
struct requires the fields to be explicitly initialized. I would think
that each field would be default initialized, the same as for a c++
class.

Anyway, here is alternate patch that fixes valgrind. Is this
preferable to a memset in the constructor? Is either Reviewed-by
worthy?

diff --git a/src/compiler/glsl_types.h b/src/compiler/glsl_types.h
index b5e97e638b..0b4a66ca4d 100644
--- a/src/compiler/glsl_types.h
+++ b/src/compiler/glsl_types.h
@@ -1045,6 +1045,13 @@ struct glsl_struct_field {
}
 
glsl_struct_field()
+  : type(NULL), name(NULL), location(0), offset(0), xfb_buffer(0),
+xfb_stride(0), interpolation(0), centroid(0),
+sample(0), matrix_layout(0), patch(0),
+precision(0), memory_read_only(0),
+memory_write_only(0), memory_coherent(0), memory_volatile(0),
+memory_restrict(0), image_format(0), explicit_xfb_buffer(0),
+implicit_sized_array(0)
{
   /* empty */
}

-Jordan
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH v2 05/32] glsl: Zero per_vertex_accumulator::fields for valgrind & nir_serialize

[Jason Ekstrand]

On Fri, Oct 20, 2017 at 4:14 PM, Kenneth Graunke 
wrote:

> On Friday, October 20, 2017 4:11:20 PM PDT Kenneth Graunke wrote:
> > On Friday, October 20, 2017 2:14:25 PM PDT Jason Ekstrand wrote:
> > > On Thu, Oct 19, 2017 at 4:51 PM, Kenneth Graunke <
> kenn...@whitecape.org>
> > > wrote:
> > >
> > > > On Wednesday, October 18, 2017 10:31:53 PM PDT Jordan Justen wrote:
> > > > > Signed-off-by: Jordan Justen 
> > > > > ---
> > > > >  src/compiler/glsl/builtin_variables.cpp | 1 +
> > > > >  1 file changed, 1 insertion(+)
> > > > >
> > > > > diff --git a/src/compiler/glsl/builtin_variables.cpp
> > > > b/src/compiler/glsl/builtin_variables.cpp
> > > > > index ea2d897cc8..d3cf12475b 100644
> > > > > --- a/src/compiler/glsl/builtin_variables.cpp
> > > > > +++ b/src/compiler/glsl/builtin_variables.cpp
> > > > > @@ -318,6 +318,7 @@ per_vertex_accumulator::per_
> vertex_accumulator()
> > > > > : fields(),
> > > > >   num_fields(0)
> > > > >  {
> > > > > +   memset(fields, 0, sizeof(fields));
> > > > >  }
> > > >
> > > > This shouldn't fix anything, we're calling the fields() constructor,
> > > > which should value-initialize every element of the fields array,
> > > > calling the constructor for glsl_struct_type on each element.
> > > >
> > > > That constructor is supposed to zero initialize items.  Maybe it is
> > > > missing some of them?
> > > >
> > >
> > > No, the problem is that, when we do the user data check in the blob
> code,
> > > valgrind complains about the padding bytes in-between field elements.
> C++
> > > constructors don't initialize that data.  That said, I agree that this
> is a
> > > tad sketchy.  I just wish there were some way to shut up valgrind.
> >
> > Okay, that makes sense.  In that case, I would drop the fields()
> > constructor in the initialization list and just do the memset.
> >
> > Also, hope nobody adds a non-zero constructor to glsl_struct_field.
>
> Or, alternatively, just make the glsl_struct_field constructor
> initialize the whole struct with a memset, then leave this code be.
>

+1
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH v2 05/32] glsl: Zero per_vertex_accumulator::fields for valgrind & nir_serialize

[Kenneth Graunke]

On Friday, October 20, 2017 4:11:20 PM PDT Kenneth Graunke wrote:
> On Friday, October 20, 2017 2:14:25 PM PDT Jason Ekstrand wrote:
> > On Thu, Oct 19, 2017 at 4:51 PM, Kenneth Graunke 
> > wrote:
> > 
> > > On Wednesday, October 18, 2017 10:31:53 PM PDT Jordan Justen wrote:
> > > > Signed-off-by: Jordan Justen 
> > > > ---
> > > >  src/compiler/glsl/builtin_variables.cpp | 1 +
> > > >  1 file changed, 1 insertion(+)
> > > >
> > > > diff --git a/src/compiler/glsl/builtin_variables.cpp
> > > b/src/compiler/glsl/builtin_variables.cpp
> > > > index ea2d897cc8..d3cf12475b 100644
> > > > --- a/src/compiler/glsl/builtin_variables.cpp
> > > > +++ b/src/compiler/glsl/builtin_variables.cpp
> > > > @@ -318,6 +318,7 @@ per_vertex_accumulator::per_vertex_accumulator()
> > > > : fields(),
> > > >   num_fields(0)
> > > >  {
> > > > +   memset(fields, 0, sizeof(fields));
> > > >  }
> > >
> > > This shouldn't fix anything, we're calling the fields() constructor,
> > > which should value-initialize every element of the fields array,
> > > calling the constructor for glsl_struct_type on each element.
> > >
> > > That constructor is supposed to zero initialize items.  Maybe it is
> > > missing some of them?
> > >
> > 
> > No, the problem is that, when we do the user data check in the blob code,
> > valgrind complains about the padding bytes in-between field elements.  C++
> > constructors don't initialize that data.  That said, I agree that this is a
> > tad sketchy.  I just wish there were some way to shut up valgrind.
> 
> Okay, that makes sense.  In that case, I would drop the fields()
> constructor in the initialization list and just do the memset.
> 
> Also, hope nobody adds a non-zero constructor to glsl_struct_field.

Or, alternatively, just make the glsl_struct_field constructor
initialize the whole struct with a memset, then leave this code be.


signature.asc
Description: This is a digitally signed message part.
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH v2 05/32] glsl: Zero per_vertex_accumulator::fields for valgrind & nir_serialize

[Kenneth Graunke]

On Friday, October 20, 2017 2:14:25 PM PDT Jason Ekstrand wrote:
> On Thu, Oct 19, 2017 at 4:51 PM, Kenneth Graunke 
> wrote:
> 
> > On Wednesday, October 18, 2017 10:31:53 PM PDT Jordan Justen wrote:
> > > Signed-off-by: Jordan Justen 
> > > ---
> > >  src/compiler/glsl/builtin_variables.cpp | 1 +
> > >  1 file changed, 1 insertion(+)
> > >
> > > diff --git a/src/compiler/glsl/builtin_variables.cpp
> > b/src/compiler/glsl/builtin_variables.cpp
> > > index ea2d897cc8..d3cf12475b 100644
> > > --- a/src/compiler/glsl/builtin_variables.cpp
> > > +++ b/src/compiler/glsl/builtin_variables.cpp
> > > @@ -318,6 +318,7 @@ per_vertex_accumulator::per_vertex_accumulator()
> > > : fields(),
> > >   num_fields(0)
> > >  {
> > > +   memset(fields, 0, sizeof(fields));
> > >  }
> >
> > This shouldn't fix anything, we're calling the fields() constructor,
> > which should value-initialize every element of the fields array,
> > calling the constructor for glsl_struct_type on each element.
> >
> > That constructor is supposed to zero initialize items.  Maybe it is
> > missing some of them?
> >
> 
> No, the problem is that, when we do the user data check in the blob code,
> valgrind complains about the padding bytes in-between field elements.  C++
> constructors don't initialize that data.  That said, I agree that this is a
> tad sketchy.  I just wish there were some way to shut up valgrind.

Okay, that makes sense.  In that case, I would drop the fields()
constructor in the initialization list and just do the memset.

Also, hope nobody adds a non-zero constructor to glsl_struct_field.


signature.asc
Description: This is a digitally signed message part.
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH v2 05/32] glsl: Zero per_vertex_accumulator::fields for valgrind & nir_serialize

[Jason Ekstrand]

On Thu, Oct 19, 2017 at 4:51 PM, Kenneth Graunke 
wrote:

> On Wednesday, October 18, 2017 10:31:53 PM PDT Jordan Justen wrote:
> > Signed-off-by: Jordan Justen 
> > ---
> >  src/compiler/glsl/builtin_variables.cpp | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/src/compiler/glsl/builtin_variables.cpp
> b/src/compiler/glsl/builtin_variables.cpp
> > index ea2d897cc8..d3cf12475b 100644
> > --- a/src/compiler/glsl/builtin_variables.cpp
> > +++ b/src/compiler/glsl/builtin_variables.cpp
> > @@ -318,6 +318,7 @@ per_vertex_accumulator::per_vertex_accumulator()
> > : fields(),
> >   num_fields(0)
> >  {
> > +   memset(fields, 0, sizeof(fields));
> >  }
>
> This shouldn't fix anything, we're calling the fields() constructor,
> which should value-initialize every element of the fields array,
> calling the constructor for glsl_struct_type on each element.
>
> That constructor is supposed to zero initialize items.  Maybe it is
> missing some of them?
>

No, the problem is that, when we do the user data check in the blob code,
valgrind complains about the padding bytes in-between field elements.  C++
constructors don't initialize that data.  That said, I agree that this is a
tad sketchy.  I just wish there were some way to shut up valgrind.
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH v2 05/32] glsl: Zero per_vertex_accumulator::fields for valgrind & nir_serialize

[Kenneth Graunke]

On Wednesday, October 18, 2017 10:31:53 PM PDT Jordan Justen wrote:
> Signed-off-by: Jordan Justen 
> ---
>  src/compiler/glsl/builtin_variables.cpp | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/src/compiler/glsl/builtin_variables.cpp 
> b/src/compiler/glsl/builtin_variables.cpp
> index ea2d897cc8..d3cf12475b 100644
> --- a/src/compiler/glsl/builtin_variables.cpp
> +++ b/src/compiler/glsl/builtin_variables.cpp
> @@ -318,6 +318,7 @@ per_vertex_accumulator::per_vertex_accumulator()
> : fields(),
>   num_fields(0)
>  {
> +   memset(fields, 0, sizeof(fields));
>  }

This shouldn't fix anything, we're calling the fields() constructor,
which should value-initialize every element of the fields array,
calling the constructor for glsl_struct_type on each element.

That constructor is supposed to zero initialize items.  Maybe it is
missing some of them?

FWIW, I trying to figure out the behavior of this by reading the
rules here:

https://stackoverflow.com/questions/1628311/array-initialisation


signature.asc
Description: This is a digitally signed message part.
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev