On 02/22/2017 03:08 PM, Timothy Arceri wrote: > On 23/02/17 07:44, Ian Romanick wrote: >> I thought we already landed a different patch for this. I'll look >> through the archives when I get back in front of a computer... >> > > It looks like the patches are still waiting on a review: > > https://patchwork.freedesktop.org/series/19382/
D'oh... okay, I've fixed that. Sorry. :( >> On February 22, 2017 11:05:26 AM Nicolai Hähnle <[email protected]> >> wrote: >> >>> From: Nicolai Hähnle <[email protected]> >>> >>> When determining whether the array is implicitly sized, we must avoid >>> accessing var->data.mode (around line 5270), because var may have been >>> deleted. >>> >>> Instead of adding yet another ternary condition based on earlier == >>> NULL, >>> refactor get_variable_being_redeclared so that we do not keep dangling >>> pointers around. >>> >>> Found by ASAN in GL45-CTS.gtf21.GL.build.array10_frag. >>> >>> Cc: [email protected] >>> --- >>> src/compiler/glsl/ast_to_hir.cpp | 55 >>> ++++++++++++++++++++++------------------ >>> 1 file changed, 30 insertions(+), 25 deletions(-) >>> >>> diff --git a/src/compiler/glsl/ast_to_hir.cpp >>> b/src/compiler/glsl/ast_to_hir.cpp >>> index b90ad97..57a9db9 100644 >>> --- a/src/compiler/glsl/ast_to_hir.cpp >>> +++ b/src/compiler/glsl/ast_to_hir.cpp >>> @@ -3942,44 +3942,50 @@ apply_type_qualifier_to_variable(const struct >>> ast_type_qualifier *qual, >>> "the shared storage qualifiers can only be >>> used with " >>> "compute shaders"); >>> } >>> >>> apply_image_qualifier_to_variable(qual, var, state, loc); >>> } >>> >>> /** >>> * Get the variable that is being redeclared by this declaration >>> * >>> + * \p pvar is changed to point to an existing variable in the >>> current scope if >>> + * the declaration that it initially points to is a redeclaration. >>> + * >>> + * The declaration originally pointed to by \p pvar may be deleted. >>> + * >>> * Semantic checks to verify the validity of the redeclaration are also >>> * performed. If semantic checks fail, compilation error will be >>> emitted via >>> - * \c _mesa_glsl_error, but a non-\c NULL pointer will still be >>> returned. >>> + * \c _mesa_glsl_error, but a non-\c NULL pointer will still be >>> provided. >>> * >>> * \returns >>> - * A pointer to an existing variable in the current scope if the >>> declaration >>> - * is a redeclaration, \c NULL otherwise. >>> + * Whether the declaration is a redeclaration. >>> */ >>> -static ir_variable * >>> -get_variable_being_redeclared(ir_variable *var, YYLTYPE loc, >>> +static bool >>> +get_variable_being_redeclared(ir_variable **pvar, YYLTYPE loc, >>> struct _mesa_glsl_parse_state *state, >>> bool allow_all_redeclarations) >>> { >>> + ir_variable *var = *pvar; >>> + >>> /* Check if this declaration is actually a re-declaration, either to >>> * resize an array or add qualifiers to an existing variable. >>> * >>> * This is allowed for variables in the current scope, or when at >>> * global scope (for built-ins in the implicit outer scope). >>> */ >>> ir_variable *earlier = state->symbols->get_variable(var->name); >>> if (earlier == NULL || >>> (state->current_function != NULL && >>> !state->symbols->name_declared_this_scope(var->name))) { >>> - return NULL; >>> + return false; >>> } >>> >>> >>> /* From page 24 (page 30 of the PDF) of the GLSL 1.50 spec, >>> * >>> * "It is legal to declare an array without a size and then >>> * later re-declare the same name as an array of the same >>> * type and specify a size." >>> */ >>> if (earlier->type->is_unsized_array() && var->type->is_array() >>> @@ -4082,21 +4088,22 @@ get_variable_being_redeclared(ir_variable >>> *var, YYLTYPE loc, >>> var->name); >>> } else if (earlier->type != var->type) { >>> _mesa_glsl_error(&loc, state, >>> "redeclaration of `%s' has incorrect type", >>> var->name); >>> } >>> } else { >>> _mesa_glsl_error(&loc, state, "`%s' redeclared", var->name); >>> } >>> >>> - return earlier; >>> + *pvar = earlier; >>> + return true; >>> } >>> >>> /** >>> * Generate the IR for an initializer in a variable declaration >>> */ >>> ir_rvalue * >>> process_initializer(ir_variable *var, ast_declaration *decl, >>> ast_fully_specified_type *type, >>> exec_list *initializer_instructions, >>> struct _mesa_glsl_parse_state *state) >>> @@ -5196,56 +5203,54 @@ ast_declarator_list::hir(exec_list >>> *instructions, >>> * list. This list will be added to the instruction stream >>> (below) after >>> * the declaration is added. This is done because in some >>> cases (such as >>> * redeclarations) the declaration may not actually be added >>> to the >>> * instruction stream. >>> */ >>> exec_list initializer_instructions; >>> >>> /* Examine var name here since var may get deleted in the next >>> call */ >>> bool var_is_gl_id = is_gl_identifier(var->name); >>> >>> - ir_variable *earlier = >>> - get_variable_being_redeclared(var, decl->get_location(), >>> state, >>> + bool is_redeclaration = >>> + get_variable_being_redeclared(&var, decl->get_location(), >>> state, >>> false /* >>> allow_all_redeclarations */); >>> - if (earlier != NULL) { >>> + if (is_redeclaration) { >>> if (var_is_gl_id && >>> - earlier->data.how_declared == ir_var_declared_in_block) { >>> + var->data.how_declared == ir_var_declared_in_block) { >>> _mesa_glsl_error(&loc, state, >>> "`%s' has already been redeclared using " >>> - "gl_PerVertex", earlier->name); >>> + "gl_PerVertex", var->name); >>> } >>> - earlier->data.how_declared = ir_var_declared_normally; >>> + var->data.how_declared = ir_var_declared_normally; >>> } >>> >>> if (decl->initializer != NULL) { >>> - result = process_initializer((earlier == NULL) ? var : >>> earlier, >>> - decl, this->type, >>> + result = process_initializer(var, decl, this->type, >>> &initializer_instructions, state); >>> } else { >>> validate_array_dimensions(var_type, state, &loc); >>> } >>> >>> /* From page 23 (page 29 of the PDF) of the GLSL 1.10 spec: >>> * >>> * "It is an error to write to a const variable outside of >>> * its declaration, so they must be initialized when >>> * declared." >>> */ >>> if (this->type->qualifier.flags.q.constant && >>> decl->initializer == NULL) { >>> _mesa_glsl_error(& loc, state, >>> "const declaration of `%s' must be >>> initialized", >>> decl->identifier); >>> } >>> >>> if (state->es_shader) { >>> - const glsl_type *const t = (earlier == NULL) >>> - ? var->type : earlier->type; >>> + const glsl_type *const t = var->type; >>> >>> /* Skip the unsized array check for TCS/TES/GS inputs & TCS >>> outputs. >>> * >>> * The GL_OES_tessellation_shader spec says about inputs: >>> * >>> * "Declaring an array size is optional. If no size is >>> specified, >>> * it will be taken from the implementation-dependent >>> maximum >>> * patch size (gl_MaxPatchVertices)." >>> * >>> * and about TCS outputs: >>> @@ -5286,21 +5291,21 @@ ast_declarator_list::hir(exec_list >>> *instructions, >>> */ >>> _mesa_glsl_error(& loc, state, >>> "unsized array declarations are not >>> allowed in " >>> "GLSL ES"); >>> } >>> >>> /* If the declaration is not a redeclaration, there are a few >>> additional >>> * semantic checks that must be applied. In addition, >>> variable that was >>> * created for the declaration should be added to the IR stream. >>> */ >>> - if (earlier == NULL) { >>> + if (!is_redeclaration) { >>> validate_identifier(decl->identifier, loc, state); >>> >>> /* Add the variable to the symbol table. Note that the >>> initializer's >>> * IR was already processed earlier (though it hasn't been >>> emitted >>> * yet), without the variable in scope. >>> * >>> * This differs from most C-like languages, but it follows >>> the GLSL >>> * specification. From page 28 (page 34 of the PDF) of the >>> GLSL 1.50 >>> * spec: >>> * >>> @@ -7862,34 +7867,34 @@ ast_interface_block::hir(exec_list >>> *instructions, >>> var->data.matrix_layout = fields[i].matrix_layout; >>> } >>> >>> if (var->data.mode == ir_var_shader_storage) >>> apply_memory_qualifiers(var, fields[i]); >>> >>> /* Examine var name here since var may get deleted in the >>> next call */ >>> bool var_is_gl_id = is_gl_identifier(var->name); >>> >>> if (redeclaring_per_vertex) { >>> - ir_variable *earlier = >>> - get_variable_being_redeclared(var, loc, state, >>> + bool is_redeclaration = >>> + get_variable_being_redeclared(&var, loc, state, >>> true /* >>> allow_all_redeclarations */); >>> - if (!var_is_gl_id || earlier == NULL) { >>> + if (!var_is_gl_id || !is_redeclaration) { >>> _mesa_glsl_error(&loc, state, >>> "redeclaration of gl_PerVertex can >>> only " >>> "include built-in variables"); >>> - } else if (earlier->data.how_declared == >>> ir_var_declared_normally) { >>> + } else if (var->data.how_declared == >>> ir_var_declared_normally) { >>> _mesa_glsl_error(&loc, state, >>> "`%s' has already been redeclared", >>> - earlier->name); >>> + var->name); >>> } else { >>> - earlier->data.how_declared = ir_var_declared_in_block; >>> - earlier->reinit_interface_type(block_type); >>> + var->data.how_declared = ir_var_declared_in_block; >>> + var->reinit_interface_type(block_type); >>> } >>> continue; >>> } >>> >>> if (state->symbols->get_variable(var->name) != NULL) >>> _mesa_glsl_error(&loc, state, "`%s' redeclared", >>> var->name); >>> >>> /* Propagate the "binding" keyword into this UBO/SSBO's >>> fields. >>> * The UBO declaration itself doesn't get an ir_variable >>> unless it >>> * has an instance name. This is ugly. >>> -- >>> 2.9.3 >>> >>> _______________________________________________ >>> mesa-stable mailing list >>> [email protected] >>> https://lists.freedesktop.org/mailman/listinfo/mesa-stable >> >> _______________________________________________ >> mesa-dev mailing list >> [email protected] >> https://lists.freedesktop.org/mailman/listinfo/mesa-dev _______________________________________________ mesa-dev mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/mesa-dev
