<div dir="ltr">On 18 September 2013 07:43, Ian Romanick <span dir="ltr"><<a href="mailto:idr@freedesktop.org" target="_blank">idr@freedesktop.org</a>></span> wrote:<br><div class="gmail_extra"><div class="gmail_quote">
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div class="im">On 09/15/2013 02:10 AM, Francisco Jerez wrote:<br>
> The _mesa_glsl_parse_state object relies on the memory allocator<br>
> zeroing out its contents before it's initialized, which seems rather<br>
> risky. One of the following commits will homogenize implementations<br>
> of the new operator in a way that would break this assumption leaving<br>
> some of the member variables of this object uninitialized.<br>
<br>
</div>This was an intentional design decision that we made, after a fair<br>
amount of consideration, in the early days of the compiler. A number of<br>
factors came together:<br>
<br>
* Use of ralloc et al. meant that we could never have stack-allocated<br>
objects.<br>
<br>
* Use of rzalloc meant that objects would always be pre-allocated to zero.<br>
<br>
* Uninitialized class member bugs are hard to track down.<br>
<br>
* It's easy to forget to add 'this->foo = 0;' when you add a field foo<br>
to a class.<br>
<br>
* A dozen lines of 'this->foo = 0;' is hard to properly review when a<br>
new class is added. Are there actually 13 fields that need to be<br>
initialized?<br>
<br>
* Writing a dozen lines of 'this->foo = 0;' is annoying. :)<br>
<br>
In the end, we decided to just rely on the allocator always providing<br>
cleared memory.<br>
<br>
I'm not very excited about adding a bunch of redundant code to the these<br>
constructors... especially since that code probably won't get maintained<br>
(e.g., when the next member gets added to those classes).<br>
<br>
I'd like to hear Ken and Eric weigh in.<br></blockquote><div><br></div><div>I think I have sufficient development experience and familiarity with the code base to weigh in as well. I fall on the opposite side of this particular fence--I prefer to see class members initialized in the constructor. My reasoning is:<br>
<br></div><div><div>* Initializing class members in the constructor costs almost nothing and decreases the risk of subtle and difficult-to-find bugs.<br><br></div><div>* Initializing class members in the constructor is common practice in C++, and therefore less surprising to developers who are new to the Mesa code base.<br>
</div><div><br></div>* If a class initializes all its members in the constructor, we have the freedom to allocate it in whatever way is appropriate (stack, rzalloc, malloc, new operator, or placement new, to name a few examples). If it doesn't, we have no choice but to allocate it using rzalloc.<br>
<br>* There's no compile-time mechanism to ensure that all uses of a class use rzalloc (nor are there any static analysis tools I'm aware of that are capable of ensuring this). It's also a pain to verify using grep. So if a class doesn't initialize all its members in the constructor, we have to be vigilant in our code review to make sure that all present and future users of the class use rzalloc.<br>
<br></div><div>* By contrast, Coverity *is* able to ensure that class members are initialized in the constructor, and I suspect other static analysis tools are too. <br></div><div> <br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<div class=""><div class="h5"><br>
> ---<br>
> src/glsl/glsl_parser_extras.cpp | 16 ++++++++++++++--<br>
> 1 file changed, 14 insertions(+), 2 deletions(-)<br>
><br>
> diff --git a/src/glsl/glsl_parser_extras.cpp b/src/glsl/glsl_parser_extras.cpp<br>
> index cac5a18..772933f 100644<br>
> --- a/src/glsl/glsl_parser_extras.cpp<br>
> +++ b/src/glsl/glsl_parser_extras.cpp<br>
> @@ -55,7 +55,7 @@ static unsigned known_desktop_glsl_versions[] =<br>
><br>
> _mesa_glsl_parse_state::_mesa_glsl_parse_state(struct gl_context *_ctx,<br>
> GLenum target, void *mem_ctx)<br>
> - : ctx(_ctx)<br>
> + : ctx(_ctx), switch_state()<br>
> {<br>
> switch (target) {<br>
> case GL_VERTEX_SHADER: this->target = vertex_shader; break;<br>
> @@ -66,10 +66,14 @@ _mesa_glsl_parse_state::_mesa_glsl_parse_state(struct gl_context *_ctx,<br>
> this->scanner = NULL;<br>
> this->translation_unit.make_empty();<br>
> this->symbols = new(mem_ctx) glsl_symbol_table;<br>
> +<br>
> + this->num_uniform_blocks = 0;<br>
> + this->uniform_block_array_size = 0;<br>
> + this->uniform_blocks = NULL;<br>
> +<br>
> this->info_log = ralloc_strdup(mem_ctx, "");<br>
> this->error = false;<br>
> this->loop_nesting_ast = NULL;<br>
> - this->switch_state.switch_nesting_ast = NULL;<br>
><br>
> this->struct_specifier_depth = 0;<br>
> this->num_builtins_to_link = 0;<br>
> @@ -105,6 +109,13 @@ _mesa_glsl_parse_state::_mesa_glsl_parse_state(struct gl_context *_ctx,<br>
><br>
> this->Const.MaxDrawBuffers = ctx->Const.MaxDrawBuffers;<br>
><br>
> + this->current_function = NULL;<br>
> + this->toplevel_ir = NULL;<br>
> + this->found_return = false;<br>
> + this->all_invariant = false;<br>
> + this->user_structures = NULL;<br>
> + this->num_user_structures = 0;<br>
> +<br>
> /* Populate the list of supported GLSL versions */<br>
> /* FINISHME: Once the OpenGL 3.0 'forward compatible' context or<br>
> * the OpenGL 3.2 Core context is supported, this logic will need<br>
> @@ -163,6 +174,7 @@ _mesa_glsl_parse_state::_mesa_glsl_parse_state(struct gl_context *_ctx,<br>
><br>
> this->gs_input_prim_type_specified = false;<br>
> this->gs_input_prim_type = GL_POINTS;<br>
> + this->gs_input_size = 0;<br>
> this->out_qualifier = new(this) ast_type_qualifier();<br>
> }<br>
><br>
><br>
<br>
</div></div><div class=""><div class="h5">_______________________________________________<br>
mesa-dev mailing list<br>
<a href="mailto:mesa-dev@lists.freedesktop.org">mesa-dev@lists.freedesktop.org</a><br>
<a href="http://lists.freedesktop.org/mailman/listinfo/mesa-dev" target="_blank">http://lists.freedesktop.org/mailman/listinfo/mesa-dev</a><br>
</div></div></blockquote></div><br></div></div>