[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
<jordan.l.justen at intel.com> wrote:
> On 2017-10-20 14:14:25, Jason Ekstrand wrote:
>> On Thu, Oct 19, 2017 at 4:51 PM, Kenneth Graunke <kenneth at whitecape.org>
>> wrote:
>>
>> > On Wednesday, October 18, 2017 10:31:53 PM PDT Jordan Justen wrote:
>> > > Signed-off-by: Jordan Justen <jordan.l.justen at intel.com>
>> > > ---
>> > >  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 at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev