glsl_type: don't serialize padding bytes from glsl_struct_field
authorAndrii Simiklit <asimiklit.work@gmail.com>
Thu, 14 May 2020 11:33:55 +0000 (14:33 +0300)
committerMarge Bot <eric+marge@anholt.net>
Wed, 20 May 2020 14:15:00 +0000 (14:15 +0000)
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 <pierre-eric.pelloux-prayer@amd.com> )
v3: - we can get rid of `struct packed_struct_field_flags`
    ( Tapani Pälli <tapani.palli@intel.com> )
    - we can get rid of `unsigned __pad: 15` bitfield
    ( Pierre-Eric Pelloux-Prayer <pierre-eric.pelloux-prayer@amd.com> )

Reviewed-by: Marek Olšák <marek.olsak@amd.com>
Reviewed-by: Pierre-Eric Pelloux-Prayer <pierre-eric.pelloux-prayer@amd.com>
Reviewed-by: Tapani Pälli <tapani.palli@intel.com>
Signed-off-by: Andrii Simiklit <asimiklit.work@gmail.com>
Part-of: <https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/5054>

src/compiler/glsl_types.cpp
src/compiler/glsl_types.h

index 71a149e47227ff707c5d429f3af9c3c7ed268f23..a1c1beae87246c26d6e8184cb8400452e350e8c7 100644 (file)
@@ -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) {
index f709bdd702e2c977d7b04bc96771d155e040a8f1..c1e0aea2eaadefae5a0b95bbc80a1112a18901ca 100644 (file)
@@ -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