Re: [Mesa-dev] [PATCH v2 05/32] glsl: Zero per_vertex_accumulator::fields for valgrind & nir_serialize
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
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
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
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
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
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
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
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
[Mesa-dev] [PATCH v2 05/32] glsl: Zero per_vertex_accumulator::fields for valgrind & nir_serialize
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)); } -- 2.15.0.rc0 ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev