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.
> /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
