Bug 69449 - Valgrind error in program_resource_visitor::recursion
Valgrind error in program_resource_visitor::recursion
Status: RESOLVED FIXED
Product: Mesa
Classification: Unclassified
Component: glsl-compiler
git
Other All
: medium normal
Assigned To: Ian Romanick
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2013-09-17 06:16 UTC by Kenneth Graunke
Modified: 2013-10-06 20:30 UTC (History)
1 user (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Kenneth Graunke 2013-09-17 06:16:04 UTC
In the Piglit directory, run:

valgrind ./bin/arb_uniform_buffer_object-layout-std140 -auto -fbo

You'll get:

==19578== Invalid read of size 1
==19578==    at 0xA9E136A: program_resource_visitor::recursion(glsl_type const*, char**, unsigned long, bool, glsl_type const*) (link_uniforms.cpp:144)
==19578==    by 0xA9E1278: program_resource_visitor::recursion(glsl_type const*, char**, unsigned long, bool, glsl_type const*) (link_uniforms.cpp:125)
==19578==    by 0xA9E0E8E: program_resource_visitor::process(glsl_type const*, char const*) (link_uniforms.cpp:63)
==19578==    by 0xA9E3F84: link_uniform_blocks(void*, gl_shader_program*, gl_shader**, unsigned int, gl_uniform_block**) (link_uniform_blocks.cpp:194)
==19578==    by 0xA9DD0E0: link_intrastage_shaders(void*, gl_context*, gl_shader_program*, gl_shader**, unsigned int) (linker.cpp:1171)
==19578==    by 0xA9DEC6A: link_shaders(gl_context*, gl_shader_program*) (linker.cpp:1958)
==19578==    by 0xAA115DB: _mesa_glsl_link_shader (ir_to_mesa.cpp:3128)
==19578==    by 0xA81275A: link_program (shaderapi.c:834)
==19578==    by 0xA8138B0: _mesa_LinkProgram (shaderapi.c:1317)
==19578==    by 0x74575E1: shared_dispatch_stub_509 (glapi_mapi_tmp.h:16564)
==19578==    by 0x4ECE678: stub_glLinkProgram (generated_dispatch.c:17556)
==19578==    by 0x4F0851D: piglit_link_simple_program (piglit-shader.c:258)
==19578==  Address 0xc3dacc8 is 0 bytes after a block of size 88 alloc'd
==19578==    at 0x4C29734: calloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==19578==    by 0xA95DA07: ralloc_size (ralloc.c:109)
==19578==    by 0xA9BEAFD: glsl_type::operator new(unsigned long) (glsl_types.h:108)
==19578==    by 0xA9BDDFA: glsl_type::get_record_instance(glsl_struct_field const*, unsigned int, char const*) (glsl_types.cpp:493)
==19578==    by 0xA990B33: ast_struct_specifier::hir(exec_list*, _mesa_glsl_parse_state*) (ast_to_hir.cpp:4574)
==19578==    by 0xA990579: ast_type_specifier::hir(exec_list*, _mesa_glsl_parse_state*) (ast_to_hir.cpp:4389)
==19578==    by 0xA990691: ast_process_structure_or_interface_block(exec_list*, _mesa_glsl_parse_state*, exec_list*, YYLTYPE&, glsl_struct_field**, bool, bool) (ast_to_hir.cpp:4441)
==19578==    by 0xA990D07: ast_interface_block::hir(exec_list*, _mesa_glsl_parse_state*) (ast_to_hir.cpp:4627)
==19578==    by 0xA985A9E: _mesa_ast_to_hir(exec_list*, _mesa_glsl_parse_state*) (ast_to_hir.cpp:93)
==19578==    by 0xA9BBFDD: _mesa_glsl_compile_shader (glsl_parser_extras.cpp:1476)
==19578==    by 0xA8124C4: compile_shader (shaderapi.c:772)
==19578==    by 0xA812E14: _mesa_CompileShader (shaderapi.c:1072)

I haven't looked into what's going wrong here.
Comment 1 Eero Tamminen 2013-09-30 15:03:59 UTC
I looked just at source, didn't valgrind piglit, but the invalid read access seems to be done here:

void
program_resource_visitor::recursion(const glsl_type *t, char **name,
                                    size_t name_length, bool row_major,
                                    const glsl_type *record_type)
... 1st call ...
      for (unsigned i = 0; i < t->length; i++) {
...
         recursion(t->fields.structure[i].type, name, new_length,
                   t->fields.structure[i].row_major, record_type);
... 2nd call ...
      for (unsigned i = 0; i < t->length; i++) {
...
         recursion(t->fields.array, name, new_length,
                   t->fields.structure[i].row_major, record_type);


Access is to memory right after the allocation done here:
  const glsl_type *t =
  glsl_type::get_record_instance(fields, decl_count, this->name);
  -> t = new glsl_type(fields, num_fields, name);
    -> this->fields.structure = ralloc_array(this->mem_ctx,
                                         glsl_struct_field, length);
      -> return ralloc_size(ctx, size * count);
        -> type = ralloc_size(glsl_type::mem_ctx, size);
          -> void *block = calloc(1, size + sizeof(ralloc_header));


The allocated type and what's accessed is:
-------------------
struct glsl_type {
        GLenum                     gl_type;              /*     0     4 */
        enum glsl_base_type        base_type;            /*     4     4 */
        unsigned int               sampler_dimensionality:3; /*     8:29  4 */
        unsigned int               sampler_shadow:1;     /*     8:28  4 */
        unsigned int               sampler_array:1;      /*     8:27  4 */
        unsigned int               sampler_type:2;       /*     8:25  4 */
        unsigned int               interface_packing:2;  /*     8:23  4 */
        unsigned int               vector_elements:3;    /*     8:20  4 */
        unsigned int               matrix_columns:3;     /*     8:17  4 */

        /* XXX 17 bits hole, try to pack */
        /* XXX 4 bytes hole, try to pack */

        const char  *              name;                 /*    16     8 */
        unsigned int               length;               /*    24     4 */

        /* XXX 4 bytes hole, try to pack */

        union {
                const class glsl_type  * array;          /*           8 */
                const class glsl_type  * parameters;     /*           8 */
                class glsl_struct_field * structure;     /*           8 */
        } fields;
...
        /* size: 40, cachelines: 1, members: 168 */
        /* sum members: 32, holes: 2, sum holes: 8 */
        /* bit holes: 1, sum bit holes: 17 bits */
        /* last cacheline: 40 bytes */
};
struct glsl_struct_field {
        const class glsl_type  *   type;                 /*     0     8 */
        const char  *              name;                 /*     8     8 */
        bool                       row_major;            /*    16     1 */

        /* size: 24, cachelines: 1, members: 3 */
        /* padding: 7 */
        /* last cacheline: 24 bytes */
};
-----------------------
(Pahole output for 64-bit Mesa build on F19)

And the ralloc_header is:
-------------------------
struct ralloc_header
{
   /* A canary value used to determine whether a pointer is ralloc'd. */
   unsigned canary;

   struct ralloc_header *parent;

   /* The first child (head of a linked list) */
   struct ralloc_header *child;

   /* Linked list of siblings */
   struct ralloc_header *prev;
   struct ralloc_header *next;

   void (*destructor)(void *);
};
---------------------

I.e. assumably 4+4+5*8 = 48 bytes (4 bytes for padding).

48 + 40 byte glsl_type = 88 i.e. it *seems* like only single glsl_type item is allocated and access is to be for start of non-existing next one.


How in this code:
---------------------
      for (unsigned i = 0; i < t->length; i++) {
...
         recursion(t->fields.array, name, new_length,
                   t->fields.structure[i].row_major, record_type);
---------------------

t->length corresponds to number of items in t->fields.structure[], and how that information is kept in sync?


> ==19578== Invalid read of size 1
> ==19578==    at 0xA9E136A: program_resource_visitor::recursion(glsl_type const*, char**, unsigned long, bool, glsl_type const*) (link_uniforms.cpp:144)
...
> ==19578==  Address 0xc3dacc8 is 0 bytes after a block of size 88 alloc'd

I don't know why access is to offset 0 and only for 1 byte, as:
-  at glsl_type offset 0 is 'gl_type' member which is 4 bytes,
- 'structure' offset is 24 and size 8, and
- 1 byte 'row_major' offset is 16 and from glsl_struct_field struct start.

But if we instead make an assumption that either:
1. somehow Valgrind got the allocation size wrong, or
2. Mesa allocated glsl_type and then at some later point assigned
   its address to glsl_struct_field 'structure' member

trying to access 'row_major' in 3rd glsl_struct_field would be:
3*24+16 = 88 bytes.

I would assume 2) to give double free error from Valgrind (or Glibc) when Mesa tries to free structure record and glsl_type memory though.


Asking Valgrind to invoke Gdb and looking at assembly code at that point could help.



Btw. regarding the row_major struct item, as far as I can see, although it's passed as arg, it's not actually needed by the recursion() or different visit_field() members...
Comment 2 Francisco Jerez 2013-10-06 20:30:09 UTC
This should be fixed in master now [1].

Thanks.

[1] http://cgit.freedesktop.org/mesa/mesa/commit/?id=b3c04362b44a4eceb38c938ceb387a9c04d06973