[PATCH 0/4] MR250: vkd3d-shader/hlsl: Declare vars individually when parsing declarations.
In a declaration with multiple variables, the variables must be created before the initializer of the next variable is parsed. This is required for initializers such as: ``` float a = 1, b = a, c = b + 1; ``` The solution is to parse the type information in the same rule as the first variable ('a' in this case) so that it can be declared before the parser reaches the initializer for the following variables. To initialize the following variables, the type information is passed along from the first variable's struct parse_variable_def to the next and so on. --- Solves https://bugs.winehq.org/show_bug.cgi?id=54484, pointed out by @nsivov. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/250
From: Francisco Casas <fcasas(a)codeweavers.com> --- Makefile.am | 1 + tests/hlsl-initializer-multi.shader_test | 43 ++++++++++++++++++++++++ 2 files changed, 44 insertions(+) create mode 100644 tests/hlsl-initializer-multi.shader_test diff --git a/Makefile.am b/Makefile.am index 92c6d0792..114a595a1 100644 --- a/Makefile.am +++ b/Makefile.am @@ -97,6 +97,7 @@ vkd3d_shader_tests = \ tests/hlsl-initializer-invalid-arg-count.shader_test \ tests/hlsl-initializer-local-array.shader_test \ tests/hlsl-initializer-matrix.shader_test \ + tests/hlsl-initializer-multi.shader_test \ tests/hlsl-initializer-nested.shader_test \ tests/hlsl-initializer-numeric.shader_test \ tests/hlsl-initializer-objects.shader_test \ diff --git a/tests/hlsl-initializer-multi.shader_test b/tests/hlsl-initializer-multi.shader_test new file mode 100644 index 000000000..6cb73503d --- /dev/null +++ b/tests/hlsl-initializer-multi.shader_test @@ -0,0 +1,43 @@ +[pixel shader todo] +float4 main() : sv_target +{ + float a = 2.0, b = a + 1.0, c = b; + + return float4(a, b, c, 0); +} + +[test] +todo draw quad +probe all rgba (2, 3, 3, 0) + + +[pixel shader todo] +float4 main() : sv_target +{ + struct apple { + float a; + int b; + } apple1 = {7.2, 8.1}, apple2 = apple1; + + return float4(apple1.a, apple1.b, apple2.a, apple2.b); +} + +[test] +todo draw quad +probe all rgba (7.2, 8.0, 7.2, 8.0) + + +[pixel shader todo] +float4 main() : sv_target +{ + struct apple { + float a; + int b; + } apple1 = {5.2, 9.1}, apples[2] = {apple1, apple1}; + + return float4(apples[0].a, apples[0].b, apples[1].a, apples[1].b); +} + +[test] +todo draw quad +probe all rgba (5.2, 9.0, 5.2, 9.0) -- GitLab https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/250
From: Francisco Casas <fcasas(a)codeweavers.com> Basically, declare_vars() is separated in two functions: 1. declare_var(), which now handles one variable at the time and doesn't free any memory. 2. initialize_vars(), which takes care of preparing the initialization instructions of several variables and frees their struct parse_variable_def, using exclusively free_parse_variable_def(). This allows to declare variables individually before the initializer of the next variable in the same declaration is parsed, which is used in the following patches. Also, simplifies memory management. --- libs/vkd3d-shader/hlsl.y | 353 +++++++++++++++++++++------------------ 1 file changed, 191 insertions(+), 162 deletions(-) diff --git a/libs/vkd3d-shader/hlsl.y b/libs/vkd3d-shader/hlsl.y index 0e07fe578..8831a0ad9 100644 --- a/libs/vkd3d-shader/hlsl.y +++ b/libs/vkd3d-shader/hlsl.y @@ -1924,31 +1924,23 @@ static bool type_has_numeric_components(struct hlsl_type *type) return false; } -static struct list *declare_vars(struct hlsl_ctx *ctx, struct hlsl_type *basic_type, - unsigned int modifiers, const struct vkd3d_shader_location *modifiers_loc, struct list *var_list) +static void declare_var(struct hlsl_ctx *ctx, struct hlsl_type *basic_type, + unsigned int modifiers, const struct vkd3d_shader_location *modifiers_loc, struct parse_variable_def *v) { - struct parse_variable_def *v, *v_next; struct hlsl_ir_function_decl *func; + bool unbounded_res_array = false; unsigned int invalid_modifiers; - struct list *statements_list; struct hlsl_ir_var *var; struct hlsl_type *type; bool local = true; + char *var_name; + unsigned int i; + + assert(basic_type); if (basic_type->class == HLSL_CLASS_MATRIX) assert(basic_type->modifiers & HLSL_MODIFIERS_MAJORITY_MASK); - if (!(statements_list = make_empty_list(ctx))) - { - LIST_FOR_EACH_ENTRY_SAFE(v, v_next, var_list, struct parse_variable_def, entry) - free_parse_variable_def(v); - vkd3d_free(var_list); - return NULL; - } - - if (!var_list) - return statements_list; - invalid_modifiers = modifiers & (HLSL_STORAGE_IN | HLSL_STORAGE_OUT); if (invalid_modifiers) { @@ -1960,175 +1952,190 @@ static struct list *declare_vars(struct hlsl_ctx *ctx, struct hlsl_type *basic_t hlsl_release_string_buffer(ctx, string); } - LIST_FOR_EACH_ENTRY_SAFE(v, v_next, var_list, struct parse_variable_def, entry) - { - bool unbounded_res_array = false; - unsigned int i; + type = basic_type; - type = basic_type; + if (shader_is_sm_5_1(ctx) && type->class == HLSL_CLASS_OBJECT) + { + for (i = 0; i < v->arrays.count; ++i) + unbounded_res_array |= (v->arrays.sizes[i] == HLSL_ARRAY_ELEMENTS_COUNT_IMPLICIT); + } - if (shader_is_sm_5_1(ctx) && type->class == HLSL_CLASS_OBJECT) + if (unbounded_res_array) + { + if (v->arrays.count == 1) { - for (i = 0; i < v->arrays.count; ++i) - unbounded_res_array |= (v->arrays.sizes[i] == HLSL_ARRAY_ELEMENTS_COUNT_IMPLICIT); + hlsl_fixme(ctx, &v->loc, "Unbounded resource arrays."); + return; } - - if (unbounded_res_array) + else { - if (v->arrays.count == 1) - { - hlsl_fixme(ctx, &v->loc, "Unbounded resource arrays."); - free_parse_variable_def(v); - continue; - } - else - { - hlsl_error(ctx, &v->loc, VKD3D_SHADER_ERROR_HLSL_INVALID_TYPE, - "Unbounded resource arrays cannot be multi-dimensional."); - } + hlsl_error(ctx, &v->loc, VKD3D_SHADER_ERROR_HLSL_INVALID_TYPE, + "Unbounded resource arrays cannot be multi-dimensional."); } - else + } + else + { + for (i = 0; i < v->arrays.count; ++i) { - for (i = 0; i < v->arrays.count; ++i) + if (v->arrays.sizes[i] == HLSL_ARRAY_ELEMENTS_COUNT_IMPLICIT) { - if (v->arrays.sizes[i] == HLSL_ARRAY_ELEMENTS_COUNT_IMPLICIT) - { - unsigned int size = initializer_size(&v->initializer); - unsigned int elem_components = hlsl_type_component_count(type); - - if (i < v->arrays.count - 1) - { - hlsl_error(ctx, &v->loc, VKD3D_SHADER_ERROR_HLSL_INVALID_TYPE, - "Only innermost array size can be implicit."); - free_parse_initializer(&v->initializer); - v->initializer.args_count = 0; - } - else if (elem_components == 0) - { - hlsl_error(ctx, &v->loc, VKD3D_SHADER_ERROR_HLSL_INVALID_TYPE, - "Cannot declare an implicit size array of a size 0 type."); - free_parse_initializer(&v->initializer); - v->initializer.args_count = 0; - } - else if (size == 0) - { - hlsl_error(ctx, &v->loc, VKD3D_SHADER_ERROR_HLSL_INVALID_TYPE, - "Implicit size arrays need to be initialized."); - free_parse_initializer(&v->initializer); - v->initializer.args_count = 0; + unsigned int size = initializer_size(&v->initializer); + unsigned int elem_components = hlsl_type_component_count(type); - } - else if (size % elem_components != 0) - { - hlsl_error(ctx, &v->loc, VKD3D_SHADER_ERROR_HLSL_WRONG_PARAMETER_COUNT, - "Cannot initialize implicit size array with %u components, expected a multiple of %u.", - size, elem_components); - free_parse_initializer(&v->initializer); - v->initializer.args_count = 0; - } - else - { - v->arrays.sizes[i] = size / elem_components; - } + if (i < v->arrays.count - 1) + { + hlsl_error(ctx, &v->loc, VKD3D_SHADER_ERROR_HLSL_INVALID_TYPE, + "Only innermost array size can be implicit."); + v->initializer.args_count = 0; + } + else if (elem_components == 0) + { + hlsl_error(ctx, &v->loc, VKD3D_SHADER_ERROR_HLSL_INVALID_TYPE, + "Cannot declare an implicit size array of a size 0 type."); + v->initializer.args_count = 0; + } + else if (size == 0) + { + hlsl_error(ctx, &v->loc, VKD3D_SHADER_ERROR_HLSL_INVALID_TYPE, + "Implicit size arrays need to be initialized."); + v->initializer.args_count = 0; + } + else if (size % elem_components != 0) + { + hlsl_error(ctx, &v->loc, VKD3D_SHADER_ERROR_HLSL_WRONG_PARAMETER_COUNT, + "Cannot initialize implicit size array with %u components, expected a multiple of %u.", + size, elem_components); + v->initializer.args_count = 0; + } + else + { + v->arrays.sizes[i] = size / elem_components; } - type = hlsl_new_array_type(ctx, type, v->arrays.sizes[i]); } + type = hlsl_new_array_type(ctx, type, v->arrays.sizes[i]); } - vkd3d_free(v->arrays.sizes); + } + + if (!(var_name = vkd3d_strdup(v->name))) + return; + + if (!(var = hlsl_new_var(ctx, var_name, type, &v->loc, &v->semantic, modifiers, &v->reg_reservation))) + { + vkd3d_free(var_name); + return; + } + + var->buffer = ctx->cur_buffer; + + if (var->buffer == ctx->globals_buffer) + { + if (var->reg_reservation.offset_type) + hlsl_error(ctx, &var->loc, VKD3D_SHADER_ERROR_HLSL_INVALID_RESERVATION, + "packoffset() is only allowed inside constant buffer declarations."); + } + + if (ctx->cur_scope == ctx->globals) + { + local = false; + + if ((modifiers & HLSL_STORAGE_UNIFORM) && (modifiers & HLSL_STORAGE_STATIC)) + hlsl_error(ctx, &var->loc, VKD3D_SHADER_ERROR_HLSL_INVALID_MODIFIER, + "Variable '%s' is declared as both \"uniform\" and \"static\".", var->name); + + /* Mark it as uniform. We need to do this here since synthetic + * variables also get put in the global scope, but shouldn't be + * considered uniforms, and we have no way of telling otherwise. */ + if (!(modifiers & HLSL_STORAGE_STATIC)) + var->storage_modifiers |= HLSL_STORAGE_UNIFORM; - if (!(var = hlsl_new_var(ctx, v->name, type, &v->loc, &v->semantic, modifiers, &v->reg_reservation))) + if (ctx->profile->major_version < 5 && (var->storage_modifiers & HLSL_STORAGE_UNIFORM) && + type_has_object_components(var->data_type, true)) { - free_parse_variable_def(v); - continue; + hlsl_error(ctx, &var->loc, VKD3D_SHADER_ERROR_HLSL_INVALID_TYPE, + "Target profile doesn't support objects as struct members in uniform variables.\n"); } - var->buffer = ctx->cur_buffer; - - if (var->buffer == ctx->globals_buffer) + if ((func = hlsl_get_func_decl(ctx, var->name))) { - if (var->reg_reservation.offset_type) - hlsl_error(ctx, &var->loc, VKD3D_SHADER_ERROR_HLSL_INVALID_RESERVATION, - "packoffset() is only allowed inside constant buffer declarations."); + hlsl_error(ctx, &var->loc, VKD3D_SHADER_ERROR_HLSL_REDEFINED, + "'%s' is already defined as a function.", var->name); + hlsl_note(ctx, &func->loc, VKD3D_SHADER_LOG_ERROR, + "'%s' was previously defined here.", var->name); } + } + else + { + static const unsigned int invalid = HLSL_STORAGE_EXTERN | HLSL_STORAGE_SHARED + | HLSL_STORAGE_GROUPSHARED | HLSL_STORAGE_UNIFORM; - if (ctx->cur_scope == ctx->globals) + if (modifiers & invalid) { - local = false; + struct vkd3d_string_buffer *string; - if ((modifiers & HLSL_STORAGE_UNIFORM) && (modifiers & HLSL_STORAGE_STATIC)) + if ((string = hlsl_modifiers_to_string(ctx, modifiers & invalid))) hlsl_error(ctx, &var->loc, VKD3D_SHADER_ERROR_HLSL_INVALID_MODIFIER, - "Variable '%s' is declared as both \"uniform\" and \"static\".", var->name); - - /* Mark it as uniform. We need to do this here since synthetic - * variables also get put in the global scope, but shouldn't be - * considered uniforms, and we have no way of telling otherwise. */ - if (!(modifiers & HLSL_STORAGE_STATIC)) - var->storage_modifiers |= HLSL_STORAGE_UNIFORM; + "Modifiers '%s' are not allowed on local variables.", string->buffer); + hlsl_release_string_buffer(ctx, string); + } + if (var->semantic.name) + hlsl_error(ctx, &var->loc, VKD3D_SHADER_ERROR_HLSL_INVALID_SEMANTIC, + "Semantics are not allowed on local variables."); + } - if (ctx->profile->major_version < 5 && (var->storage_modifiers & HLSL_STORAGE_UNIFORM) && - type_has_object_components(var->data_type, true)) - { - hlsl_error(ctx, &var->loc, VKD3D_SHADER_ERROR_HLSL_INVALID_TYPE, - "Target profile doesn't support objects as struct members in uniform variables.\n"); - } + if ((var->storage_modifiers & HLSL_STORAGE_STATIC) && type_has_numeric_components(var->data_type) + && type_has_object_components(var->data_type, false)) + { + hlsl_error(ctx, &var->loc, VKD3D_SHADER_ERROR_HLSL_INVALID_TYPE, + "Static variables cannot have both numeric and resource components."); + } - if ((func = hlsl_get_func_decl(ctx, var->name))) - { - hlsl_error(ctx, &var->loc, VKD3D_SHADER_ERROR_HLSL_REDEFINED, - "'%s' is already defined as a function.", var->name); - hlsl_note(ctx, &func->loc, VKD3D_SHADER_LOG_ERROR, - "'%s' was previously defined here.", var->name); - } - } - else - { - static const unsigned int invalid = HLSL_STORAGE_EXTERN | HLSL_STORAGE_SHARED - | HLSL_STORAGE_GROUPSHARED | HLSL_STORAGE_UNIFORM; + if ((type->modifiers & HLSL_MODIFIER_CONST) && !v->initializer.args_count + && !(modifiers & (HLSL_STORAGE_STATIC | HLSL_STORAGE_UNIFORM))) + { + hlsl_error(ctx, &v->loc, VKD3D_SHADER_ERROR_HLSL_MISSING_INITIALIZER, + "Const variable \"%s\" is missing an initializer.", var->name); + hlsl_free_var(var); + return; + } - if (modifiers & invalid) - { - struct vkd3d_string_buffer *string; + if (!hlsl_add_var(ctx, var, local)) + { + struct hlsl_ir_var *old = hlsl_get_var(ctx->cur_scope, var->name); - if ((string = hlsl_modifiers_to_string(ctx, modifiers & invalid))) - hlsl_error(ctx, &var->loc, VKD3D_SHADER_ERROR_HLSL_INVALID_MODIFIER, - "Modifiers '%s' are not allowed on local variables.", string->buffer); - hlsl_release_string_buffer(ctx, string); - } - if (var->semantic.name) - hlsl_error(ctx, &var->loc, VKD3D_SHADER_ERROR_HLSL_INVALID_SEMANTIC, - "Semantics are not allowed on local variables."); - } + hlsl_error(ctx, &var->loc, VKD3D_SHADER_ERROR_HLSL_REDEFINED, + "Variable \"%s\" was already declared in this scope.", var->name); + hlsl_note(ctx, &old->loc, VKD3D_SHADER_LOG_ERROR, "\"%s\" was previously declared here.", old->name); + hlsl_free_var(var); + return; + } +} - if ((var->storage_modifiers & HLSL_STORAGE_STATIC) && type_has_numeric_components(var->data_type) - && type_has_object_components(var->data_type, false)) - { - hlsl_error(ctx, &var->loc, VKD3D_SHADER_ERROR_HLSL_INVALID_TYPE, - "Static variables cannot have both numeric and resource components."); - } +static struct list *initialize_vars(struct hlsl_ctx *ctx, struct list *var_list) +{ + struct parse_variable_def *v, *v_next; + struct list *statements_list; + struct hlsl_ir_var *var; + struct hlsl_type *type; - if ((type->modifiers & HLSL_MODIFIER_CONST) && !v->initializer.args_count - && !(modifiers & (HLSL_STORAGE_STATIC | HLSL_STORAGE_UNIFORM))) + if (!(statements_list = make_empty_list(ctx))) + { + LIST_FOR_EACH_ENTRY_SAFE(v, v_next, var_list, struct parse_variable_def, entry) { - hlsl_error(ctx, &v->loc, VKD3D_SHADER_ERROR_HLSL_MISSING_INITIALIZER, - "Const variable \"%s\" is missing an initializer.", var->name); - hlsl_free_var(var); - free_parse_initializer(&v->initializer); - vkd3d_free(v); - continue; + free_parse_variable_def(v); } + vkd3d_free(var_list); + return NULL; + } - if (!hlsl_add_var(ctx, var, local)) + LIST_FOR_EACH_ENTRY_SAFE(v, v_next, var_list, struct parse_variable_def, entry) + { + /* If this fails, the variable failed to be declared. */ + if (!(var = hlsl_get_var(ctx->cur_scope, v->name))) { - struct hlsl_ir_var *old = hlsl_get_var(ctx->cur_scope, var->name); - - hlsl_error(ctx, &var->loc, VKD3D_SHADER_ERROR_HLSL_REDEFINED, - "Variable \"%s\" was already declared in this scope.", var->name); - hlsl_note(ctx, &old->loc, VKD3D_SHADER_LOG_ERROR, "\"%s\" was previously declared here.", old->name); - hlsl_free_var(var); - free_parse_initializer(&v->initializer); - vkd3d_free(v); + free_parse_variable_def(v); continue; } + type = var->data_type; if (v->initializer.args_count) { @@ -2143,8 +2150,7 @@ static struct list *declare_vars(struct hlsl_ctx *ctx, struct hlsl_type *basic_t hlsl_error(ctx, &v->loc, VKD3D_SHADER_ERROR_HLSL_WRONG_PARAMETER_COUNT, "Expected %u components in initializer, but got %u.", hlsl_type_component_count(type), size); - free_parse_initializer(&v->initializer); - vkd3d_free(v); + free_parse_variable_def(v); continue; } @@ -2163,12 +2169,10 @@ static struct list *declare_vars(struct hlsl_ctx *ctx, struct hlsl_type *basic_t add_assignment(ctx, v->initializer.instrs, &load->node, ASSIGN_OP_ASSIGN, v->initializer.args[0]); } - if (modifiers & HLSL_STORAGE_STATIC) + if (var->storage_modifiers & HLSL_STORAGE_STATIC) list_move_tail(&ctx->static_initializers.instrs, v->initializer.instrs); else list_move_tail(statements_list, v->initializer.instrs); - vkd3d_free(v->initializer.args); - vkd3d_free(v->initializer.instrs); } else if (var->storage_modifiers & HLSL_STORAGE_STATIC) { @@ -2178,32 +2182,33 @@ static struct list *declare_vars(struct hlsl_ctx *ctx, struct hlsl_type *basic_t if (type_has_object_components(var->data_type, false)) { - vkd3d_free(v); + free_parse_variable_def(v); continue; } if (!(zero = hlsl_new_uint_constant(ctx, 0, &var->loc))) { - vkd3d_free(v); + free_parse_variable_def(v); continue; } hlsl_block_add_instr(&ctx->static_initializers, zero); if (!(cast = add_cast(ctx, &ctx->static_initializers.instrs, zero, var->data_type, &var->loc))) { - vkd3d_free(v); + free_parse_variable_def(v); continue; } if (!(store = hlsl_new_simple_store(ctx, var, cast))) { - vkd3d_free(v); + free_parse_variable_def(v); continue; } hlsl_block_add_instr(&ctx->static_initializers, store); } - vkd3d_free(v); + free_parse_variable_def(v); } + vkd3d_free(var_list); return statements_list; } @@ -4564,6 +4569,7 @@ preproc_directive: struct_declaration: var_modifiers struct_spec variables_def_optional ';' { + struct parse_variable_def *v, *v_next; struct hlsl_type *type; unsigned int modifiers = $1; @@ -4579,7 +4585,22 @@ struct_declaration: if (!(type = apply_type_modifiers(ctx, $2, &modifiers, true, &@1))) YYABORT; - $$ = declare_vars(ctx, type, modifiers, &@1, $3); + + if ($3) + { + LIST_FOR_EACH_ENTRY_SAFE(v, v_next, $3, struct parse_variable_def, entry) + { + declare_var(ctx, type, modifiers, &@1, v); + } + + if (!($$ = initialize_vars(ctx, $3))) + YYABORT; + } + else + { + if (!($$ = make_empty_list(ctx))) + YYABORT; + } } struct_spec: @@ -5418,12 +5439,20 @@ type_spec: declaration: var_modifiers type variables_def ';' { + struct parse_variable_def *v, *v_next; struct hlsl_type *type; unsigned int modifiers = $1; if (!(type = apply_type_modifiers(ctx, $2, &modifiers, true, &@1))) YYABORT; - $$ = declare_vars(ctx, type, modifiers, &@1, $3); + + LIST_FOR_EACH_ENTRY_SAFE(v, v_next, $3, struct parse_variable_def, entry) + { + declare_var(ctx, type, modifiers, &@1, v); + } + + if (!($$ = initialize_vars(ctx, $3))) + YYABORT; } variables_def_optional: -- GitLab https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/250
From: Francisco Casas <fcasas(a)codeweavers.com> In a declaration with multiple variables, the variables must be created before the initializer of the next variable is parsed. This is required for initializers such as: float a = 1, b = a, c = b + 1; A requisite for this is that the type information is parsed in the same rule as the first variable (as a variable_def_typed) so it is immediately available to declare the first variable. Then, the next untyped variable declaration is parsed, and the type from the first variable can be used to declare the second, before the third is parsed, and so on. --- libs/vkd3d-shader/hlsl.y | 76 ++++++++++++++++++------ tests/hlsl-initializer-multi.shader_test | 4 +- 2 files changed, 61 insertions(+), 19 deletions(-) diff --git a/libs/vkd3d-shader/hlsl.y b/libs/vkd3d-shader/hlsl.y index 8831a0ad9..c36ae01b8 100644 --- a/libs/vkd3d-shader/hlsl.y +++ b/libs/vkd3d-shader/hlsl.y @@ -73,6 +73,10 @@ struct parse_variable_def struct hlsl_semantic semantic; struct hlsl_reg_reservation reg_reservation; struct parse_initializer initializer; + + struct hlsl_type *basic_type; + unsigned int modifiers; + struct vkd3d_shader_location modifiers_loc; }; struct parse_function @@ -1924,10 +1928,12 @@ static bool type_has_numeric_components(struct hlsl_type *type) return false; } -static void declare_var(struct hlsl_ctx *ctx, struct hlsl_type *basic_type, - unsigned int modifiers, const struct vkd3d_shader_location *modifiers_loc, struct parse_variable_def *v) +static void declare_var(struct hlsl_ctx *ctx, struct parse_variable_def *v) { + const struct vkd3d_shader_location *modifiers_loc = &v->modifiers_loc; + struct hlsl_type *basic_type = v->basic_type; struct hlsl_ir_function_decl *func; + uint32_t modifiers = v->modifiers; bool unbounded_res_array = false; unsigned int invalid_modifiers; struct hlsl_ir_var *var; @@ -4436,6 +4442,7 @@ static void validate_texture_format_type(struct hlsl_ctx *ctx, struct hlsl_type %type <list> unary_expr %type <list> variables_def %type <list> variables_def_optional +%type <list> variables_def_typed %token <name> VAR_IDENTIFIER %token <name> NEW_IDENTIFIER @@ -4498,6 +4505,7 @@ static void validate_texture_format_type(struct hlsl_ctx *ctx, struct hlsl_type %type <variable_def> type_spec %type <variable_def> variable_decl %type <variable_def> variable_def +%type <variable_def> variable_def_typed %% @@ -4590,7 +4598,11 @@ struct_declaration: { LIST_FOR_EACH_ENTRY_SAFE(v, v_next, $3, struct parse_variable_def, entry) { - declare_var(ctx, type, modifiers, &@1, v); + v->basic_type = type; + v->modifiers = modifiers; + v->modifiers_loc = @1; + + declare_var(ctx, v); } if (!($$ = initialize_vars(ctx, $3))) @@ -5437,21 +5449,9 @@ type_spec: } declaration: - var_modifiers type variables_def ';' + variables_def_typed ';' { - struct parse_variable_def *v, *v_next; - struct hlsl_type *type; - unsigned int modifiers = $1; - - if (!(type = apply_type_modifiers(ctx, $2, &modifiers, true, &@1))) - YYABORT; - - LIST_FOR_EACH_ENTRY_SAFE(v, v_next, $3, struct parse_variable_def, entry) - { - declare_var(ctx, type, modifiers, &@1, v); - } - - if (!($$ = initialize_vars(ctx, $3))) + if (!($$ = initialize_vars(ctx, $1))) YYABORT; } @@ -5475,6 +5475,33 @@ variables_def: list_add_tail($$, &$3->entry); } +variables_def_typed: + variable_def_typed + { + if (!($$ = make_empty_list(ctx))) + YYABORT; + list_add_head($$, &$1->entry); + + declare_var(ctx, $1); + } + | variables_def_typed ',' variable_def + { + struct parse_variable_def *head_def; + + assert(!list_empty($1)); + head_def = LIST_ENTRY(list_head($1), struct parse_variable_def, entry); + + assert(head_def->basic_type); + $3->basic_type = head_def->basic_type; + $3->modifiers = head_def->modifiers; + $3->modifiers_loc = head_def->modifiers_loc; + + declare_var(ctx, $3); + + $$ = $1; + list_add_tail($$, &$3->entry); + } + variable_decl: any_identifier arrays colon_attribute { @@ -5516,6 +5543,21 @@ variable_def: ctx->in_state_block = 0; } +variable_def_typed: + var_modifiers type variable_def + { + struct hlsl_type *type; + unsigned int modifiers = $1; + + if (!(type = apply_type_modifiers(ctx, $2, &modifiers, true, &@1))) + YYABORT; + + $$ = $3; + $$->basic_type = type; + $$->modifiers = modifiers; + $$->modifiers_loc = @1; + } + arrays: %empty { diff --git a/tests/hlsl-initializer-multi.shader_test b/tests/hlsl-initializer-multi.shader_test index 6cb73503d..f542960d5 100644 --- a/tests/hlsl-initializer-multi.shader_test +++ b/tests/hlsl-initializer-multi.shader_test @@ -1,4 +1,4 @@ -[pixel shader todo] +[pixel shader] float4 main() : sv_target { float a = 2.0, b = a + 1.0, c = b; @@ -7,7 +7,7 @@ float4 main() : sv_target } [test] -todo draw quad +draw quad probe all rgba (2, 3, 3, 0) -- GitLab https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/250
From: Francisco Casas <fcasas(a)codeweavers.com> A struct declaration with variables is now absorbed into the 'declaration' rule, like any other variable declaration. A struct declaration without variables is now reduced to the 'struct_declaration_without_vars' rule. They both are reduced to a 'declaration_statement' in the end. --- libs/vkd3d-shader/hlsl.y | 71 +++++++++--------------- tests/hlsl-initializer-multi.shader_test | 8 +-- 2 files changed, 29 insertions(+), 50 deletions(-) diff --git a/libs/vkd3d-shader/hlsl.y b/libs/vkd3d-shader/hlsl.y index c36ae01b8..5debeaa70 100644 --- a/libs/vkd3d-shader/hlsl.y +++ b/libs/vkd3d-shader/hlsl.y @@ -4437,11 +4437,10 @@ static void validate_texture_format_type(struct hlsl_ctx *ctx, struct hlsl_type %type <list> shift_expr %type <list> statement %type <list> statement_list -%type <list> struct_declaration +%type <list> struct_declaration_without_vars %type <list> type_specs %type <list> unary_expr %type <list> variables_def -%type <list> variables_def_optional %type <list> variables_def_typed %token <name> VAR_IDENTIFIER @@ -4574,45 +4573,19 @@ preproc_directive: } } -struct_declaration: - var_modifiers struct_spec variables_def_optional ';' +struct_declaration_without_vars: + var_modifiers struct_spec ';' { - struct parse_variable_def *v, *v_next; - struct hlsl_type *type; - unsigned int modifiers = $1; + if (!$2->name) + hlsl_error(ctx, &@2, VKD3D_SHADER_ERROR_HLSL_INVALID_SYNTAX, + "Anonymous struct type must declare a variable."); - if (!$3) - { - if (!$2->name) - hlsl_error(ctx, &@2, VKD3D_SHADER_ERROR_HLSL_INVALID_SYNTAX, - "Anonymous struct type must declare a variable."); - if (modifiers) - hlsl_error(ctx, &@1, VKD3D_SHADER_ERROR_HLSL_INVALID_MODIFIER, - "Modifiers are not allowed on struct type declarations."); - } + if ($1) + hlsl_error(ctx, &@1, VKD3D_SHADER_ERROR_HLSL_INVALID_MODIFIER, + "Modifiers are not allowed on struct type declarations."); - if (!(type = apply_type_modifiers(ctx, $2, &modifiers, true, &@1))) + if (!($$ = make_empty_list(ctx))) YYABORT; - - if ($3) - { - LIST_FOR_EACH_ENTRY_SAFE(v, v_next, $3, struct parse_variable_def, entry) - { - v->basic_type = type; - v->modifiers = modifiers; - v->modifiers_loc = @1; - - declare_var(ctx, v); - } - - if (!($$ = initialize_vars(ctx, $3))) - YYABORT; - } - else - { - if (!($$ = make_empty_list(ctx))) - YYABORT; - } } struct_spec: @@ -5387,7 +5360,7 @@ type: declaration_statement: declaration - | struct_declaration + | struct_declaration_without_vars | typedef { if (!($$ = make_empty_list(ctx))) @@ -5455,13 +5428,6 @@ declaration: YYABORT; } -variables_def_optional: - %empty - { - $$ = NULL; - } - | variables_def - variables_def: variable_def { @@ -5544,7 +5510,20 @@ variable_def: } variable_def_typed: - var_modifiers type variable_def + var_modifiers struct_spec variable_def + { + struct hlsl_type *type; + unsigned int modifiers = $1; + + if (!(type = apply_type_modifiers(ctx, $2, &modifiers, true, &@1))) + YYABORT; + + $$ = $3; + $$->basic_type = type; + $$->modifiers = modifiers; + $$->modifiers_loc = @1; + } + | var_modifiers type variable_def { struct hlsl_type *type; unsigned int modifiers = $1; diff --git a/tests/hlsl-initializer-multi.shader_test b/tests/hlsl-initializer-multi.shader_test index f542960d5..f27bf7e23 100644 --- a/tests/hlsl-initializer-multi.shader_test +++ b/tests/hlsl-initializer-multi.shader_test @@ -11,7 +11,7 @@ draw quad probe all rgba (2, 3, 3, 0) -[pixel shader todo] +[pixel shader] float4 main() : sv_target { struct apple { @@ -23,11 +23,11 @@ float4 main() : sv_target } [test] -todo draw quad +draw quad probe all rgba (7.2, 8.0, 7.2, 8.0) -[pixel shader todo] +[pixel shader] float4 main() : sv_target { struct apple { @@ -39,5 +39,5 @@ float4 main() : sv_target } [test] -todo draw quad +draw quad probe all rgba (5.2, 9.0, 5.2, 9.0) -- GitLab https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/250
Giovanni Mascellani (@giomasce) commented about tests/hlsl-initializer-multi.shader_test:
+[pixel shader todo] +float4 main() : sv_target +{ + float a = 2.0, b = a + 1.0, c = b; + + return float4(a, b, c, 0); +} + Would you mind adding this test too?
```hlsl [pixel shader fail] float4 main() : sv_target { float a = 2.0, b = c + 1.0, c = b; return float4(0); } ``` Also, this commits lacks the "tests:" subject prefix. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/250#note_37112
Giovanni Mascellani (@giomasce) commented about libs/vkd3d-shader/hlsl.y:
+ } + else + { + v->arrays.sizes[i] = size / elem_components; } - type = hlsl_new_array_type(ctx, type, v->arrays.sizes[i]); } + type = hlsl_new_array_type(ctx, type, v->arrays.sizes[i]); } - vkd3d_free(v->arrays.sizes); + } + + if (!(var_name = vkd3d_strdup(v->name))) + return; + + if (!(var = hlsl_new_var(ctx, var_name, type, &v->loc, &v->semantic, modifiers, &v->reg_reservation))) I'm not convinced ownership for `semantic->name` is handled correctly. Here `hlsl_new_var()` acquires ownership of the name, but at the same time it seems that the `struct parse_variable_def` still retains it (given that `free_parse_variable_def()` is called in `initialize_vars()`). Am I missing something?
-- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/250#note_37113
I reviewed the first two commits. I'll leave the other two for another day. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/250#note_37114
On Wed Jun 28 11:20:13 2023 +0000, Giovanni Mascellani wrote:
I'm not convinced ownership for `semantic->name` is handled correctly. Here `hlsl_new_var()` acquires ownership of the name, but at the same time it seems that the `struct parse_variable_def` still retains it (given that `free_parse_variable_def()` is called in `initialize_vars()`). Am I missing something? You are right, I didn't realize because there is no test that triggers an invalid read, we don't test semantics in variable|struct declarations, which makes sense since they don't show up in the output bytecode unlike when they appear in function parameters, function return value, or struct fields.
This test is enough to trigger the failure: ``` float a : SEM; float4 main() : sv_target { return float4(a, a, a, 0); } ``` I think that the proper solution would be making the new variable store a copy of `var->semantic.name`, just as it stores a copy of `var->name`. I am even thinking that hlsl_new_variable() should take care of creating a copy of both strings internally. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/250#note_37170
participants (3)
-
Francisco Casas -
Francisco Casas (＠fcasas)
-
Giovanni Mascellani (＠giomasce)