On Mon, 19 Dec 2011 13:24:20 -0800
Ian Romanick <i...@freedesktop.org> wrote:
> On 12/19/2011 05:23 AM, Pekka Paalanen wrote:
> > On Fri, 16 Dec 2011 10:46:11 -0800
> > Ian Romanick<i...@freedesktop.org> wrote:
> >
> >> On 12/14/2011 11:26 PM, Pekka Paalanen wrote:
> >>> string_to_uint_map::put() already does a strdup() for the key
> >>> argument, so we leak the memory allocated by strdup() in
> >>> link_uniforms.cpp.
> >>>
> >>> Remove the extra strdup(), fixes a few Valgrind detected leaks.
> >>
> >> Have you run piglit on this? I seem to recall adding the extra
> >> strdup for a reason. The hash table keeps a copy of the key
> >> pointer passed to it, and the underlying object may be deleted
> >> before the hash table is deleted. This can happen if the back-end
> >> optimizes some uniforms away after the linker has generated the
> >> list of "active" uniforms. I'm pretty sure there were one or two
> >> test cases that hit this on i965.
> >
> > Sorry, I didn't. Finally got piglit running, though readPixSanity
> > failed on stencil values. Anyway it's running all.tests now for the
> > upstream Mesa master, and then I'll run it with my patches.
> >
> > Oh, this is Sandybridge, btw.
> >
> >>> --- a/src/glsl/link_uniforms.cpp
> >>> +++ b/src/glsl/link_uniforms.cpp
> >>> @@ -174,8 +174,7 @@ private:
> >>> if (this->map->get(id, name))
> >>> return;
> >>>
> >>> - char *key = strdup(name);
> >>> - this->map->put(this->num_active_uniforms, key);
> >>> + this->map->put(this->num_active_uniforms, name);
> >>>
> >>> /* Each leaf uniform occupies one entry in the list of
> >>> active
> >>> * uniforms.
> >
> > The whole visit_field() function is this:
> >
> > virtual void visit_field(const glsl_type *type, const char
> > *name) {
> > assert(!type->is_record());
> > assert(!(type->is_array()&&
> > type->fields.array->is_record()));
> >
> > /* Count the number of samplers regardless of whether the
> > uniform is
> > * already in the hash table. The hash table prevents
> > adding the same
> > * uniform for multiple shader targets, but in this case we
> > want to
> > * count it for each shader target.
> > */
> > const unsigned values = values_for_type(type);
> > if (type->contains_sampler()) {
> > this->num_shader_samplers +=
> > type->is_array() ? type->array_size() : 1;
> > } else {
> > /* Accumulate the total number of uniform slots used by
> > this shader.
> > * Note that samplers do not count against this limit
> > because they
> > * don't use any storage on current hardware.
> > */
> > this->num_shader_uniform_components += values;
> > }
> >
> > /* If the uniform is already in the map, there's nothing
> > more to do. */
> > unsigned id;
> > if (this->map->get(id, name))
> > return;
> >
> > char *key = strdup(name);
> > this->map->put(this->num_active_uniforms, key);
> >
> > /* Each leaf uniform occupies one entry in the list of active
> > * uniforms.
> > */
> > this->num_active_uniforms++;
> > this->num_values += values;
> > }
> >
> > The variable 'key' is not used anywhere else but passed to
> > string_to_uint_map::put(). That function in its whole is:
> >
> > void put(unsigned value, const char *key)
> > {
> > /* The low-level hash table structure returns NULL if key is
> > not in the
> > * hash table. However, users of this map might want to
> > store zero as a
> > * valid value in the table. Bias the value by +1 so that a
> > * user-specified zero is stored as 1. This enables ::get
> > to tell the
> > * difference between a user-specified zero (returned as 1 by
> > * hash_table_find) and the key not in the table (returned
> > as 0 by
> > * hash_table_find).
> > *
> > * The net effect is that we can't store UINT_MAX in the
> > table. This is
> > * because UINT_MAX+1 = 0.
> > */
> > assert(value != UINT_MAX);
> > hash_table_replace(this->ht,
> > (void *) (intptr_t) (value + 1),
> > strdup(key));
> > }
> >
> > Therefore, as far as I see, 'key' is not used for anything else than
> > passed again through strdup(), which would effectively be:
>
> Ah yes, I forgot that I added that in string_to_uint_map::put. You
> are quite correct.
>
> Okay, the original patch is:
>
> Reviewed-by: Ian Romanick <ian.d.roman...@intel.com>
You were right, it does regress in piglit.
This particular patch causes varray-disabled to fail in quick.tests.
>From 4 runs of rebuilt Mesa, 3 fail in varray-disabled, and the
remaining one fails in EXT_TIMER_QUERY:time-elapsed instead.
Some of the rebuilds were from clean checkout (git clean -dxf), some
simply by adding the patch on top of a previous build that produced the
reference piglit results.
Unfortunately I have to move on for now, so I'm not pushing this patch.
Thanks.
> > hash_table_replace(..., ..., strdup(strdup(name)));
> >
> > I don't see any case where this could not be a leak, or prevent a
> > bug.
> >
> > What do I miss? Is there somewhere another put() that overwrites or
> > overloads this version of put()?
> >
> >
> > Thanks,
> > pq
> >
> > ps. As for the other patch, thanks for the advice on how to fix it
> > properly. I don't think I can do that this year, though.
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
