On Wed, Feb 19, 2014 at 4:35 AM, Ian Romanick <i...@freedesktop.org> wrote:
> On 02/09/2014 01:50 AM, Juha-Pekka Heikkilä wrote:
>> The place which cause Klocwork to highlight this is at
>> src/mesa/main/ff_fragment_shader.cpp around line 897 where it says:
>>
>> ...
>> float const_data[4] = {
>> float(1 << rgb_shift),
>> float(1 << rgb_shift),
>> float(1 << rgb_shift),
>> float(1 << alpha_shift)
>> };
>> shift = new(p->mem_ctx) ir_constant(glsl_type::vec4_type,
>> (ir_constant_data *)const_data);
>> ...
>>
>> I don't know if this is the only place for such usage but it looks
>> reasonable to me.
>>
>> If I fix this place instead of what my patch does Klocwork would tell
>> where is next similar usage for this ir_constant constructor, that is
>> if such exist.
>
> The ir_constant constructors are used in a lot of places. I'd rather
> not take the extra overhead to work around callers lying about the
> parameter type. If we have to keep fixing these one at a time, I'm okay
> with that. The error case is (most likely) that we read some garbage
> data from the stack. We should fix those, but it's not a cataclysmic
> error.
>
> Doing a quick check...
>
> egrep -r 'new[^ ]* ir_constant[(][^,)]*,[^)]*[)]' src/
>
> shows
>
> src/mesa/drivers/dri/i965/brw_fs_fp.cpp: ir->coordinate =
> new(mem_ctx) ir_constant(coordinate_type, &junk_data);
> src/glsl/builtin_functions.cpp: return new(mem_ctx) ir_constant(f,
> vector_elements);
> src/glsl/builtin_functions.cpp: return new(mem_ctx) ir_constant(i,
> vector_elements);
> src/glsl/builtin_functions.cpp: return new(mem_ctx) ir_constant(u,
> vector_elements);
> src/glsl/builtin_functions.cpp: return new(mem_ctx) ir_constant(type,
> &data);
> src/glsl/ir_clone.cpp: return new(mem_ctx) ir_constant(this->type,
> &this->value);
> src/glsl/opt_constant_propagation.cpp: *rvalue = new(ralloc_parent(deref))
> ir_constant(type, &data);
> src/glsl/ir_constant_expression.cpp: return new(ctx)
> ir_constant(this->type, &data);
> src/glsl/ir_constant_expression.cpp: return new(ctx)
> ir_constant(this->type, &data);
> src/glsl/ir_constant_expression.cpp: return new(ctx)
> ir_constant(column_type, &data);
> src/glsl/ir_constant_expression.cpp: return new(ctx) ir_constant(array,
> component);
> src/glsl/opt_algebraic.cpp: return new(mem_ctx) ir_constant(ir->type,
> &data);
> src/glsl/ast_to_hir.cpp: var->constant_value = new(var)
> ir_constant(glsl_type::ivec3_type, &data);
> src/glsl/ast_to_hir.cpp: new(var) ir_constant(glsl_type::ivec3_type,
> &data);
> src/glsl/lower_instructions.cpp: ir_constant *sign_mask = new(ir)
> ir_constant(0x80000000u, vec_elem);
> src/glsl/lower_instructions.cpp: ir_constant *exp_shift = new(ir)
> ir_constant(23u, vec_elem);
> src/glsl/lower_instructions.cpp: ir_constant *exp_width = new(ir)
> ir_constant(8u, vec_elem);
> src/glsl/lower_instructions.cpp: new(ir)
> ir_constant(0x1, vec_elem))));
> src/glsl/tests/uniform_initializer_utils.cpp: val = new(mem_ctx)
> ir_constant(type, &data);
> src/glsl/tests/uniform_initializer_utils.cpp: val = new(mem_ctx)
> ir_constant(array_type, &values_for_array);
> src/glsl/ast_function.cpp: return new(ctx) ir_constant(constant,
> component);
> src/glsl/ast_function.cpp: return new(ctx) ir_constant(constructor_type,
> &actual_parameters);
> src/glsl/ast_function.cpp: return new(ctx) ir_constant(constructor_type,
> &actual_parameters);
> src/glsl/ast_function.cpp: return new(mem_ctx)
> ir_constant(constructor_type, parameters);
> src/glsl/ast_function.cpp: ir_rvalue *rhs = new(ctx)
> ir_constant(rhs_type, &data);
> src/glsl/ast_function.cpp: new(ctx)
> ir_constant(rhs_var->type, &zero),
> src/glsl/ast_function.cpp: ir_rvalue *const rhs = new(ctx)
> ir_constant(col_type, &ident);
> src/glsl/ast_function.cpp: return new(ctx)
> ir_constant(constructor_type, &actual_parameters);
> src/glsl/ir_reader.cpp: return new(mem_ctx) ir_constant(type, &elements);
> src/glsl/ir_reader.cpp: return new(mem_ctx) ir_constant(type, &data);
> src/glsl/builtin_variables.cpp: var->constant_value = new(var)
> ir_constant(glsl_type::ivec3_type, &data);
> src/glsl/builtin_variables.cpp: new(var)
> ir_constant(glsl_type::ivec3_type, &data);
>
> Of those, I think we only need to audit:
>
> src/mesa/drivers/dri/i965/brw_fs_fp.cpp: ir->coordinate =
> new(mem_ctx) ir_constant(coordinate_type, &junk_data);
> src/glsl/builtin_functions.cpp: return new(mem_ctx) ir_constant(type,
> &data);
> src/glsl/opt_constant_propagation.cpp: *rvalue = new(ralloc_parent(deref))
> ir_constant(type, &data);
> src/glsl/ir_constant_expression.cpp: return new(ctx)
> ir_constant(this->type, &data);
> src/glsl/ir_constant_expression.cpp: return new(ctx)
> ir_constant(this->type, &data);
> src/glsl/ir_constant_expression.cpp: return new(ctx)
> ir_constant(column_type, &data);
> src/glsl/ir_constant_expression.cpp: return new(ctx) ir_constant(array,
> component);
> src/glsl/opt_algebraic.cpp: return new(mem_ctx) ir_constant(ir->type,
> &data);
> src/glsl/ast_to_hir.cpp: var->constant_value = new(var)
> ir_constant(glsl_type::ivec3_type, &data);
> src/glsl/ast_to_hir.cpp: new(var) ir_constant(glsl_type::ivec3_type,
> &data);
> src/glsl/tests/uniform_initializer_utils.cpp: val = new(mem_ctx)
> ir_constant(type, &data);
> src/glsl/tests/uniform_initializer_utils.cpp: val = new(mem_ctx)
> ir_constant(array_type, &values_for_array);
> src/glsl/ast_function.cpp: ir_rvalue *rhs = new(ctx)
> ir_constant(rhs_type, &data);
> src/glsl/ast_function.cpp: new(ctx)
> ir_constant(rhs_var->type, &zero),
> src/glsl/ast_function.cpp: ir_rvalue *const rhs = new(ctx)
> ir_constant(col_type, &ident);
> src/glsl/ir_reader.cpp: return new(mem_ctx) ir_constant(type, &elements);
> src/glsl/ir_reader.cpp: return new(mem_ctx) ir_constant(type, &data);
> src/glsl/builtin_variables.cpp: var->constant_value = new(var)
> ir_constant(glsl_type::ivec3_type, &data);
> src/glsl/builtin_variables.cpp: new(var)
> ir_constant(glsl_type::ivec3_type, &data);
>
> I skimmed a few of these, and the ones I looked at were fine... none of
> them have an explicit cast, so the compiler would complain if they
> weren't fine. :)
>
> This search obviously isn't perfect... it misses the case we're
> discussing. I'm sure there's some nice cscope-like tool for C++ that
> would find all the callers of that particular constructor.
I have also been greping around this and going after the places where
this is used. So far I found only the one place which Klocwork also
highlight to be a bit questionable. I think you're correct I go fixing
just place at src/mesa/main/ff_fragment_shader.cpp which cause
Klocwork to highlight this. I just did not get to make the patch yet
but it will follow soon. :)
/Juha-Pekka
>
>> /Juha-Pekka
>>
>>
>> On Sat, Feb 8, 2014 at 2:30 AM, Ian Romanick <i...@freedesktop.org> wrote:
>>> On 02/07/2014 04:44 AM, Juha-Pekka Heikkila wrote:
>>>> ir_constant::ir_constant(const struct glsl_type,
>>>> const ir_constant_data *) was copying too much memory.
>>>
>>> The code looks correct as-is to me. This copies one ir_constant_data
>>> union to another... they're declared as the same type, and they have the
>>> same size. What is the actual error? Is there some code somewhere
>>> that's casting a different type to ir_constant_data* to pass into this
>>> constructor?
>>>
>>>> Signed-off-by: Juha-Pekka Heikkila <juhapekka.heikk...@gmail.com>
>>>> ---
>>>> src/glsl/ir.cpp | 2 +-
>>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/src/glsl/ir.cpp b/src/glsl/ir.cpp
>>>> index 1a36bd6..abc5568 100644
>>>> --- a/src/glsl/ir.cpp
>>>> +++ b/src/glsl/ir.cpp
>>>> @@ -622,7 +622,7 @@ ir_constant::ir_constant(const struct glsl_type *type,
>>>>
>>>> this->ir_type = ir_type_constant;
>>>> this->type = type;
>>>> - memcpy(& this->value, data, sizeof(this->value));
>>>> + memcpy(& this->value, data, type->std140_size(false));
>>>> }
>>>>
>>>> ir_constant::ir_constant(float f, unsigned vector_elements)
>
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
