From 3725aa7b5dbea96a747ede0182a3c8a52d756948 Mon Sep 17 00:00:00 2001 From: Andrii Simiklit Date: Thu, 14 May 2020 14:33:55 +0300 Subject: [PATCH] glsl_type: don't serialize padding bytes from glsl_struct_field MIME-Version: 1.0 Content-Type: text/plain; charset=utf8 Content-Transfer-Encoding: 8bit This should fix such valgrind warnings: ==37417== Uninitialised byte(s) found during client check request ==37417== at 0x6183471: blob_write_bytes (blob.c:163) ==37417== by 0x629785B: encode_type_to_blob (glsl_types.cpp:2760) ==37417== by 0x61E68D8: write_variable (nir_serialize.c:293) ==37417== by 0x61E6F6A: write_var_list (nir_serialize.c:421) ==37417== by 0x61EBA7A: nir_serialize (nir_serialize.c:2018) ==37417== by 0x5B5E007: serialize_nir_part (brw_program_binary.c:135) ==37417== by 0x5B5E7F3: brw_serialize_program_binary (brw_program_binary.c:299) ==37417== by 0x5FEF5FF: write_program_payload (program_binary.c:177) ==37417== by 0x5FEF7BB: _mesa_get_program_binary_length (program_binary.c:225) ==37417== by 0x5E3D31D: get_programiv (shaderapi.c:912) ==37417== by 0x5E3F730: _mesa_GetProgramiv (shaderapi.c:1827) ==37417== by 0x111DA0: program_binary_save_restore (shader_runner.c:686) ==37417== Address 0x8f59481 is 81 bytes inside a block of size 480 alloc'd ==37417== at 0x483B7F3: malloc (vg_replace_malloc.c:309) ==37417== by 0x618CE67: ralloc_size (ralloc.c:123) ==37417== by 0x618CF35: rzalloc_size (ralloc.c:155) ==37417== by 0x618D245: rzalloc_array_size (ralloc.c:234) ==37417== by 0x629041D: glsl_type::glsl_type(glsl_struct_field const*, unsigned int, glsl_interface_packing, bool, char const*) (glsl_types.cpp:148) ==37417== by 0x6293EC3: glsl_type::get_interface_instance(glsl_struct_field const*, unsigned int, glsl_interface_packing, bool, char const*) (glsl_types.cpp:1271) ==37417== by 0x604C878: (anonymous namespace)::per_vertex_accumulator::construct_interface_instance() const (builtin_variables.cpp:365) ==37417== by 0x6050722: (anonymous namespace)::builtin_variable_generator::generate_varyings() (builtin_variables.cpp:1568) ==37417== by 0x60509CA: _mesa_glsl_initialize_variables(exec_list*, _mesa_glsl_parse_state*) (builtin_variables.cpp:1600) ==37417== by 0x6149AE9: _mesa_ast_to_hir(exec_list*, _mesa_glsl_parse_state*) (ast_to_hir.cpp:131) ==37417== by 0x60706D6: _mesa_glsl_compile_shader (glsl_parser_extras.cpp:2222) ==37417== by 0x5E3DC16: _mesa_compile_shader (shaderapi.c:1211) ==37417== Use of uninitialised value of size 8 ==37417== at 0x529AE13: ??? (in /usr/lib/x86_64-linux-gnu/libz.so.1.2.11) ==37417== by 0x6184075: util_hash_crc32 (crc32.c:127) ==37417== by 0x5FEF401: write_program_binary (program_binary.c:95) ==37417== by 0x5FEF8BC: _mesa_get_program_binary (program_binary.c:252) ==37417== by 0x5E40E22: _mesa_GetProgramBinary (shaderapi.c:2411) ==37417== by 0x4914057: stub_glGetProgramBinary (piglit-dispatch-gen.c:24737) ==37417== by 0x111E4A: program_binary_save_restore (shader_runner.c:704) ==37417== by 0x11F765: piglit_display (shader_runner.c:5112) ==37417== by 0x499082F: run_test (piglit_fbo_framework.c:52) ==37417== by 0x4980E89: piglit_gl_test_run (piglit-framework-gl.c:229) ==37417== by 0x110DA9: main (shader_runner.c:72) v2: - decode_glsl_struct_field_from_blob and encode_glsl_struct_field should be `static` ( Pierre-Eric Pelloux-Prayer ) v3: - we can get rid of `struct packed_struct_field_flags` ( Tapani Pälli ) - we can get rid of `unsigned __pad: 15` bitfield ( Pierre-Eric Pelloux-Prayer ) Reviewed-by: Marek Olšák Reviewed-by: Pierre-Eric Pelloux-Prayer Reviewed-by: Tapani Pälli Signed-off-by: Andrii Simiklit Part-of: --- src/compiler/glsl_types.cpp | 64 +++++++++-------- src/compiler/glsl_types.h | 135 ++++++++++++++++++------------------ 2 files changed, 99 insertions(+), 100 deletions(-) diff --git a/src/compiler/glsl_types.cpp b/src/compiler/glsl_types.cpp index 71a149e4722..a1c1beae872 100644 --- a/src/compiler/glsl_types.cpp +++ b/src/compiler/glsl_types.cpp @@ -2620,16 +2620,6 @@ glsl_type::coordinate_components() const #include "compiler/builtin_type_macros.h" /** @} */ -static void -get_struct_type_field_and_pointer_sizes(size_t *s_field_size, - size_t *s_field_ptrs) -{ - *s_field_size = sizeof(glsl_struct_field); - *s_field_ptrs = - sizeof(((glsl_struct_field *)0)->type) + - sizeof(((glsl_struct_field *)0)->name); -} - union packed_type { uint32_t u32; struct { @@ -2660,6 +2650,32 @@ union packed_type { } strct; }; +static void +encode_glsl_struct_field(blob *blob, const glsl_struct_field *struct_field) +{ + encode_type_to_blob(blob, struct_field->type); + blob_write_string(blob, struct_field->name); + blob_write_uint32(blob, struct_field->location); + blob_write_uint32(blob, struct_field->offset); + blob_write_uint32(blob, struct_field->xfb_buffer); + blob_write_uint32(blob, struct_field->xfb_stride); + blob_write_uint32(blob, struct_field->image_format); + blob_write_uint32(blob, struct_field->flags); +} + +static void +decode_glsl_struct_field_from_blob(blob_reader *blob, glsl_struct_field *struct_field) +{ + struct_field->type = decode_type_from_blob(blob); + struct_field->name = blob_read_string(blob); + struct_field->location = blob_read_uint32(blob); + struct_field->offset = blob_read_uint32(blob); + struct_field->xfb_buffer = blob_read_uint32(blob); + struct_field->xfb_stride = blob_read_uint32(blob); + struct_field->image_format = (pipe_format)blob_read_uint32(blob); + struct_field->flags = blob_read_uint32(blob); +} + void encode_type_to_blob(struct blob *blob, const glsl_type *type) { @@ -2749,18 +2765,8 @@ encode_type_to_blob(struct blob *blob, const glsl_type *type) if (encoded.strct.length == 0xffffff) blob_write_uint32(blob, type->length); - size_t s_field_size, s_field_ptrs; - get_struct_type_field_and_pointer_sizes(&s_field_size, &s_field_ptrs); - - for (unsigned i = 0; i < type->length; i++) { - encode_type_to_blob(blob, type->fields.structure[i].type); - blob_write_string(blob, type->fields.structure[i].name); - - /* Write the struct field skipping the pointers */ - blob_write_bytes(blob, - ((char *)&type->fields.structure[i]) + s_field_ptrs, - s_field_size - s_field_ptrs); - } + for (unsigned i = 0; i < type->length; i++) + encode_glsl_struct_field(blob, &type->fields.structure[i]); return; case GLSL_TYPE_VOID: break; @@ -2842,18 +2848,10 @@ decode_type_from_blob(struct blob_reader *blob) if (num_fields == 0xffffff) num_fields = blob_read_uint32(blob); - size_t s_field_size, s_field_ptrs; - get_struct_type_field_and_pointer_sizes(&s_field_size, &s_field_ptrs); - glsl_struct_field *fields = - (glsl_struct_field *) malloc(s_field_size * num_fields); - for (unsigned i = 0; i < num_fields; i++) { - fields[i].type = decode_type_from_blob(blob); - fields[i].name = blob_read_string(blob); - - blob_copy_bytes(blob, ((uint8_t *) &fields[i]) + s_field_ptrs, - s_field_size - s_field_ptrs); - } + (glsl_struct_field *) malloc(sizeof(glsl_struct_field) * num_fields); + for (unsigned i = 0; i < num_fields; i++) + decode_glsl_struct_field_from_blob(blob, &fields[i]); const glsl_type *t; if (base_type == GLSL_TYPE_INTERFACE) { diff --git a/src/compiler/glsl_types.h b/src/compiler/glsl_types.h index f709bdd702e..c1e0aea2eaa 100644 --- a/src/compiler/glsl_types.h +++ b/src/compiler/glsl_types.h @@ -1272,93 +1272,94 @@ struct glsl_struct_field { * -1 otherwise. */ int xfb_stride; - - /** - * For interface blocks, the interpolation mode (as in - * ir_variable::interpolation). 0 otherwise. - */ - unsigned interpolation:3; - - /** - * For interface blocks, 1 if this variable uses centroid interpolation (as - * in ir_variable::centroid). 0 otherwise. - */ - unsigned centroid:1; - - /** - * For interface blocks, 1 if this variable uses sample interpolation (as - * in ir_variable::sample). 0 otherwise. - */ - unsigned sample:1; - - /** - * Layout of the matrix. Uses glsl_matrix_layout values. - */ - unsigned matrix_layout:2; - - /** - * For interface blocks, 1 if this variable is a per-patch input or output - * (as in ir_variable::patch). 0 otherwise. - */ - unsigned patch:1; - - /** - * Precision qualifier - */ - unsigned precision:2; - - /** - * Memory qualifiers, applicable to buffer variables defined in shader - * storage buffer objects (SSBOs) - */ - unsigned memory_read_only:1; - unsigned memory_write_only:1; - unsigned memory_coherent:1; - unsigned memory_volatile:1; - unsigned memory_restrict:1; - /** * Layout format, applicable to image variables only. */ enum pipe_format image_format; - /** - * Any of the xfb_* qualifiers trigger the shader to be in transform - * feedback mode so we need to keep track of whether the buffer was - * explicitly set or if its just been assigned the default global value. - */ - unsigned explicit_xfb_buffer:1; - - unsigned implicit_sized_array:1; + union { + struct { + /** + * For interface blocks, the interpolation mode (as in + * ir_variable::interpolation). 0 otherwise. + */ + unsigned interpolation:3; + + /** + * For interface blocks, 1 if this variable uses centroid interpolation (as + * in ir_variable::centroid). 0 otherwise. + */ + unsigned centroid:1; + + /** + * For interface blocks, 1 if this variable uses sample interpolation (as + * in ir_variable::sample). 0 otherwise. + */ + unsigned sample:1; + + /** + * Layout of the matrix. Uses glsl_matrix_layout values. + */ + unsigned matrix_layout:2; + + /** + * For interface blocks, 1 if this variable is a per-patch input or output + * (as in ir_variable::patch). 0 otherwise. + */ + unsigned patch:1; + + /** + * Precision qualifier + */ + unsigned precision:2; + + /** + * Memory qualifiers, applicable to buffer variables defined in shader + * storage buffer objects (SSBOs) + */ + unsigned memory_read_only:1; + unsigned memory_write_only:1; + unsigned memory_coherent:1; + unsigned memory_volatile:1; + unsigned memory_restrict:1; + + /** + * Any of the xfb_* qualifiers trigger the shader to be in transform + * feedback mode so we need to keep track of whether the buffer was + * explicitly set or if its just been assigned the default global value. + */ + unsigned explicit_xfb_buffer:1; + + unsigned implicit_sized_array:1; + }; + unsigned flags; + }; #ifdef __cplusplus -#define DEFAULT_CONSTRUCTORS(_type, _precision, _name) \ +#define DEFAULT_CONSTRUCTORS(_type, _name) \ type(_type), name(_name), location(-1), offset(-1), xfb_buffer(0), \ - xfb_stride(0), interpolation(0), centroid(0), \ - sample(0), matrix_layout(GLSL_MATRIX_LAYOUT_INHERITED), patch(0), \ - precision(_precision), memory_read_only(0), \ - memory_write_only(0), memory_coherent(0), memory_volatile(0), \ - memory_restrict(0), image_format(PIPE_FORMAT_NONE), \ - explicit_xfb_buffer(0), \ - implicit_sized_array(0) + xfb_stride(0), image_format(PIPE_FORMAT_NONE), flags(0) \ glsl_struct_field(const struct glsl_type *_type, int _precision, const char *_name) - : DEFAULT_CONSTRUCTORS(_type, _precision, _name) + : DEFAULT_CONSTRUCTORS(_type, _name) { - /* empty */ + matrix_layout = GLSL_MATRIX_LAYOUT_INHERITED; + precision = _precision; } glsl_struct_field(const struct glsl_type *_type, const char *_name) - : DEFAULT_CONSTRUCTORS(_type, GLSL_PRECISION_NONE, _name) + : DEFAULT_CONSTRUCTORS(_type, _name) { - /* empty */ + matrix_layout = GLSL_MATRIX_LAYOUT_INHERITED; + precision = GLSL_PRECISION_NONE; } glsl_struct_field() - : DEFAULT_CONSTRUCTORS(NULL, GLSL_PRECISION_NONE, NULL) + : DEFAULT_CONSTRUCTORS(NULL, NULL) { - /* empty */ + matrix_layout = GLSL_MATRIX_LAYOUT_INHERITED; + precision = GLSL_PRECISION_NONE; } #undef DEFAULT_CONSTRUCTORS #endif -- 2.30.2