[PATCH v6 0/11] MR317: vkd3d-shader: Begin validating the generated IR.
The validation code is meant both as a check that the frontend is behaving properly and as a sort of the documentation to establish what is allowed and what is not in the IR. ~~Currently an assertion is thrown if validation fails. I realize this is a rather strong proposal, but it's of course up for debate. In theory asserting here is the right thing, as it is expected that the frontend is generating correct IR code. However vkd3d is already used in production for many programs, and it could very well be that some of those are working properly even if the generated IR is somewhat out of specs; allowing the assertion might cause regressions as soon as enough checks are implemented in the validator. Please let me know your opinions.~~ **Solved in favor of a softer failure, and only when validation is enabled** -- v6: vkd3d-shader/ir: Validate source parameters. vkd3d-shader/ir: Validate destination parameters. vkd3d-shader/ir: Validate register types. vkd3d-shader/ir: Validate instruction handlers. vkd3d-shader/ir: Introduce a boilerplate to validate the generated IR. vkd3d-shader: Embed the parsing location in vkd3d_shader_instruction. vkd3d-shader/dxil: Destroy the SM6 parser on parsing errors. vkd3d-shader/tpf: Destroy the SM4 parser on parsing errors. vkd3d-shader/d3dbc: Destroy the SM1 parser on parsing errors. vkd3d-shader/d3dbc: Ignore DCL source parameters. vkd3d-shader/ir: Simplify the control flow in shader_instruction_normalise_io_params(). https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/317
From: Giovanni Mascellani <gmascellani(a)codeweavers.com> --- libs/vkd3d-shader/ir.c | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/libs/vkd3d-shader/ir.c b/libs/vkd3d-shader/ir.c index 6d7c8965..87b8f80a 100644 --- a/libs/vkd3d-shader/ir.c +++ b/libs/vkd3d-shader/ir.c @@ -1014,7 +1014,6 @@ static void shader_instruction_normalise_io_params(struct vkd3d_shader_instructi struct io_normaliser *normaliser) { struct vkd3d_shader_register *reg; - bool keep = true; unsigned int i; switch (ins->handler_idx) @@ -1033,15 +1032,16 @@ static void shader_instruction_normalise_io_params(struct vkd3d_shader_instructi /* fall through */ case VKD3DSIH_DCL_INPUT_PS: case VKD3DSIH_DCL_OUTPUT: - keep = shader_dst_param_io_normalise(&ins->declaration.dst, true, normaliser); + if (!shader_dst_param_io_normalise(&ins->declaration.dst, true, normaliser)) + vkd3d_shader_instruction_make_nop(ins); break; case VKD3DSIH_DCL_INPUT_SGV: case VKD3DSIH_DCL_INPUT_SIV: case VKD3DSIH_DCL_INPUT_PS_SGV: case VKD3DSIH_DCL_INPUT_PS_SIV: case VKD3DSIH_DCL_OUTPUT_SIV: - keep = shader_dst_param_io_normalise(&ins->declaration.register_semantic.reg, true, - normaliser); + if (!shader_dst_param_io_normalise(&ins->declaration.register_semantic.reg, true, normaliser)) + vkd3d_shader_instruction_make_nop(ins); break; case VKD3DSIH_HS_CONTROL_POINT_PHASE: case VKD3DSIH_HS_FORK_PHASE: @@ -1060,9 +1060,6 @@ static void shader_instruction_normalise_io_params(struct vkd3d_shader_instructi shader_src_param_io_normalise((struct vkd3d_shader_src_param *)&ins->src[i], normaliser); break; } - - if (!keep) - shader_instruction_init(ins, VKD3DSIH_NOP); } static enum vkd3d_result instruction_array_normalise_io_registers(struct vkd3d_shader_instruction_array *instructions, -- GitLab https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/317
From: Giovanni Mascellani <gmascellani(a)codeweavers.com> They are neither used nor initialised, which triggers failures in the upcoming VSIR validator. --- libs/vkd3d-shader/d3dbc.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/libs/vkd3d-shader/d3dbc.c b/libs/vkd3d-shader/d3dbc.c index 2b02d51f..fcc0e3ec 100644 --- a/libs/vkd3d-shader/d3dbc.c +++ b/libs/vkd3d-shader/d3dbc.c @@ -1128,6 +1128,9 @@ static void shader_sm1_read_instruction(struct vkd3d_shader_sm1_parser *sm1, str if (ins->handler_idx == VKD3DSIH_DCL) { shader_sm1_read_semantic(sm1, &p, &ins->declaration.semantic); + /* DCL has two source parameters, but they seem to be + * unused. We ignore them. */ + ins->src_count = 0; } else if (ins->handler_idx == VKD3DSIH_DEF) { -- GitLab https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/317
From: Giovanni Mascellani <gmascellani(a)codeweavers.com> --- libs/vkd3d-shader/d3dbc.c | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/libs/vkd3d-shader/d3dbc.c b/libs/vkd3d-shader/d3dbc.c index fcc0e3ec..27f6cfb7 100644 --- a/libs/vkd3d-shader/d3dbc.c +++ b/libs/vkd3d-shader/d3dbc.c @@ -1336,12 +1336,19 @@ int vkd3d_shader_sm1_parser_create(const struct vkd3d_shader_compile_info *compi ++instructions->count; } - *parser = &sm1->p; - for (i = 0; i < ARRAY_SIZE(sm1->p.shader_desc.flat_constant_count); ++i) sm1->p.shader_desc.flat_constant_count[i].external = get_external_constant_count(sm1, i); - return sm1->p.failed ? VKD3D_ERROR_INVALID_SHADER : VKD3D_OK; + if (sm1->p.failed) + { + WARN("Failed to parse shader.\n"); + shader_sm1_destroy(&sm1->p); + return VKD3D_ERROR_INVALID_SHADER; + } + + *parser = &sm1->p; + + return VKD3D_OK; } bool hlsl_sm1_register_from_semantic(struct hlsl_ctx *ctx, const struct hlsl_semantic *semantic, -- GitLab https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/317
From: Giovanni Mascellani <gmascellani(a)codeweavers.com> --- libs/vkd3d-shader/tpf.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/libs/vkd3d-shader/tpf.c b/libs/vkd3d-shader/tpf.c index 7949be15..6d8c4507 100644 --- a/libs/vkd3d-shader/tpf.c +++ b/libs/vkd3d-shader/tpf.c @@ -2642,9 +2642,16 @@ int vkd3d_shader_sm4_parser_create(const struct vkd3d_shader_compile_info *compi if (sm4->p.shader_version.type == VKD3D_SHADER_TYPE_HULL && !sm4->has_control_point_phase && !sm4->p.failed) shader_sm4_validate_default_phase_index_ranges(sm4); + if (sm4->p.failed) + { + WARN("Failed to parse shader.\n"); + shader_sm4_destroy(&sm4->p); + return VKD3D_ERROR_INVALID_SHADER; + } + *parser = &sm4->p; - return sm4->p.failed ? VKD3D_ERROR_INVALID_SHADER : VKD3D_OK; + return VKD3D_OK; } static void write_sm4_block(const struct tpf_writer *tpf, const struct hlsl_block *block); -- GitLab https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/317
From: Giovanni Mascellani <gmascellani(a)codeweavers.com> --- libs/vkd3d-shader/dxil.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/libs/vkd3d-shader/dxil.c b/libs/vkd3d-shader/dxil.c index 666d8b08..c99acdee 100644 --- a/libs/vkd3d-shader/dxil.c +++ b/libs/vkd3d-shader/dxil.c @@ -2953,6 +2953,9 @@ int vkd3d_shader_sm6_parser_create(const struct vkd3d_shader_compile_info *compi compile_info->source_name, message_context); vkd3d_free(byte_code); + if (sm6->p.failed && ret >= 0) + ret = VKD3D_ERROR_INVALID_SHADER; + if (ret < 0) { WARN("Failed to initialise shader parser.\n"); -- GitLab https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/317
From: Giovanni Mascellani <gmascellani(a)codeweavers.com> So that it can be used for printing meaningful error locations by downstream processors. --- libs/vkd3d-shader/d3dbc.c | 2 +- libs/vkd3d-shader/dxil.c | 4 ++-- libs/vkd3d-shader/ir.c | 19 ++++++++++++------- libs/vkd3d-shader/tpf.c | 2 +- libs/vkd3d-shader/vkd3d_shader_private.h | 16 +++++++++------- 5 files changed, 25 insertions(+), 18 deletions(-) diff --git a/libs/vkd3d-shader/d3dbc.c b/libs/vkd3d-shader/d3dbc.c index 27f6cfb7..1b993c63 100644 --- a/libs/vkd3d-shader/d3dbc.c +++ b/libs/vkd3d-shader/d3dbc.c @@ -1090,7 +1090,7 @@ static void shader_sm1_read_instruction(struct vkd3d_shader_sm1_parser *sm1, str goto fail; } - ins->handler_idx = opcode_info->vkd3d_opcode; + vsir_instruction_init(ins, &sm1->p.location, opcode_info->vkd3d_opcode); ins->flags = (opcode_token & VKD3D_SM1_INSTRUCTION_FLAGS_MASK) >> VKD3D_SM1_INSTRUCTION_FLAGS_SHIFT; ins->coissue = opcode_token & VKD3D_SM1_COISSUE; ins->raw = false; diff --git a/libs/vkd3d-shader/dxil.c b/libs/vkd3d-shader/dxil.c index c99acdee..128f7b16 100644 --- a/libs/vkd3d-shader/dxil.c +++ b/libs/vkd3d-shader/dxil.c @@ -1999,7 +1999,7 @@ static struct vkd3d_shader_instruction *sm6_parser_add_instruction(struct sm6_pa { struct vkd3d_shader_instruction *ins = sm6_parser_require_space(sm6, 1); assert(ins); - shader_instruction_init(ins, handler_idx); + vsir_instruction_init(ins, &sm6->p.location, handler_idx); ++sm6->p.instructions.count; return ins; } @@ -2190,7 +2190,7 @@ static void sm6_parser_emit_dx_store_output(struct sm6_parser *sm6, struct sm6_b return; } - shader_instruction_init(ins, VKD3DSIH_MOV); + vsir_instruction_init(ins, &sm6->p.location, VKD3DSIH_MOV); if (!(dst_param = instruction_dst_params_alloc(ins, 1, sm6))) return; diff --git a/libs/vkd3d-shader/ir.c b/libs/vkd3d-shader/ir.c index 87b8f80a..b361420a 100644 --- a/libs/vkd3d-shader/ir.c +++ b/libs/vkd3d-shader/ir.c @@ -161,6 +161,7 @@ struct hull_flattener unsigned int instance_count; unsigned int phase_body_idx; enum vkd3d_shader_opcode phase; + struct vkd3d_shader_location last_ret_location; }; static bool flattener_is_in_fork_or_join_phase(const struct hull_flattener *flattener) @@ -233,6 +234,7 @@ static void flattener_eliminate_phase_related_dcls(struct hull_flattener *normal if (ins->handler_idx == VKD3DSIH_RET) { + normaliser->last_ret_location = ins->location; vkd3d_shader_instruction_make_nop(ins); if (locations->count >= ARRAY_SIZE(locations->locations)) { @@ -313,9 +315,11 @@ void shader_register_init(struct vkd3d_shader_register *reg, enum vkd3d_shader_r reg->immconst_type = VKD3D_IMMCONST_SCALAR; } -void shader_instruction_init(struct vkd3d_shader_instruction *ins, enum vkd3d_shader_opcode handler_idx) +void vsir_instruction_init(struct vkd3d_shader_instruction *ins, const struct vkd3d_shader_location *location, + enum vkd3d_shader_opcode handler_idx) { memset(ins, 0, sizeof(*ins)); + ins->location = *location; ins->handler_idx = handler_idx; } @@ -343,7 +347,7 @@ static enum vkd3d_result instruction_array_flatten_hull_shader_phases(struct vkd if (!shader_instruction_array_reserve(&flattener.instructions, flattener.instructions.count + 1)) return VKD3D_ERROR_OUT_OF_MEMORY; - shader_instruction_init(&instructions->elements[instructions->count++], VKD3DSIH_RET); + vsir_instruction_init(&instructions->elements[instructions->count++], &flattener.last_ret_location, VKD3DSIH_RET); } *src_instructions = flattener.instructions; @@ -404,7 +408,8 @@ static void shader_dst_param_io_init(struct vkd3d_shader_dst_param *param, const } static enum vkd3d_result control_point_normaliser_emit_hs_input(struct control_point_normaliser *normaliser, - const struct shader_signature *s, unsigned int input_control_point_count, unsigned int dst) + const struct shader_signature *s, unsigned int input_control_point_count, unsigned int dst, + const struct vkd3d_shader_location *location) { struct vkd3d_shader_instruction *ins; struct vkd3d_shader_dst_param *param; @@ -422,7 +427,7 @@ static enum vkd3d_result control_point_normaliser_emit_hs_input(struct control_p normaliser->instructions.count += count; ins = &normaliser->instructions.elements[dst]; - shader_instruction_init(ins, VKD3DSIH_HS_CONTROL_POINT_PHASE); + vsir_instruction_init(ins, location, VKD3DSIH_HS_CONTROL_POINT_PHASE); ins->flags = 1; ++ins; @@ -434,13 +439,13 @@ static enum vkd3d_result control_point_normaliser_emit_hs_input(struct control_p if (e->sysval_semantic != VKD3D_SHADER_SV_NONE) { - shader_instruction_init(ins, VKD3DSIH_DCL_INPUT_SIV); + vsir_instruction_init(ins, location, VKD3DSIH_DCL_INPUT_SIV); param = &ins->declaration.register_semantic.reg; ins->declaration.register_semantic.sysval_semantic = vkd3d_siv_from_sysval(e->sysval_semantic); } else { - shader_instruction_init(ins, VKD3DSIH_DCL_INPUT); + vsir_instruction_init(ins, location, VKD3DSIH_DCL_INPUT); param = &ins->declaration.dst; } @@ -511,7 +516,7 @@ static enum vkd3d_result instruction_array_normalise_hull_shader_control_point_i case VKD3DSIH_HS_FORK_PHASE: case VKD3DSIH_HS_JOIN_PHASE: ret = control_point_normaliser_emit_hs_input(&normaliser, input_signature, - input_control_point_count, i); + input_control_point_count, i, &ins->location); *src_instructions = normaliser.instructions; return ret; default: diff --git a/libs/vkd3d-shader/tpf.c b/libs/vkd3d-shader/tpf.c index 6d8c4507..228de2d1 100644 --- a/libs/vkd3d-shader/tpf.c +++ b/libs/vkd3d-shader/tpf.c @@ -2327,7 +2327,7 @@ static void shader_sm4_read_instruction(struct vkd3d_shader_sm4_parser *sm4, str return; } - ins->handler_idx = opcode_info->handler_idx; + vsir_instruction_init(ins, &sm4->p.location, opcode_info->handler_idx); if (ins->handler_idx == VKD3DSIH_HS_CONTROL_POINT_PHASE || ins->handler_idx == VKD3DSIH_HS_FORK_PHASE || ins->handler_idx == VKD3DSIH_HS_JOIN_PHASE) sm4->phase = ins->handler_idx; diff --git a/libs/vkd3d-shader/vkd3d_shader_private.h b/libs/vkd3d-shader/vkd3d_shader_private.h index eab1c730..70055614 100644 --- a/libs/vkd3d-shader/vkd3d_shader_private.h +++ b/libs/vkd3d-shader/vkd3d_shader_private.h @@ -968,8 +968,15 @@ struct vkd3d_shader_primitive_type unsigned int patch_vertex_count; }; +struct vkd3d_shader_location +{ + const char *source_name; + unsigned int line, column; +}; + struct vkd3d_shader_instruction { + struct vkd3d_shader_location location; enum vkd3d_shader_opcode handler_idx; DWORD flags; unsigned int dst_count; @@ -1008,7 +1015,8 @@ struct vkd3d_shader_instruction } declaration; }; -void shader_instruction_init(struct vkd3d_shader_instruction *ins, enum vkd3d_shader_opcode handler_idx); +void vsir_instruction_init(struct vkd3d_shader_instruction *ins, const struct vkd3d_shader_location *location, + enum vkd3d_shader_opcode handler_idx); static inline bool vkd3d_shader_instruction_has_texel_offset(const struct vkd3d_shader_instruction *ins) { @@ -1035,12 +1043,6 @@ static inline bool register_is_constant(const struct vkd3d_shader_register *reg) return (reg->type == VKD3DSPR_IMMCONST || reg->type == VKD3DSPR_IMMCONST64); } -struct vkd3d_shader_location -{ - const char *source_name; - unsigned int line, column; -}; - struct vkd3d_shader_param_node { struct vkd3d_shader_param_node *next; -- GitLab https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/317
From: Giovanni Mascellani <gmascellani(a)codeweavers.com> For the moment the validator is trivial, it never fails. Checks will be added incrementally. --- gitlab/build.yml | 4 ++++ libs/vkd3d-shader/d3dbc.c | 3 +++ libs/vkd3d-shader/dxil.c | 3 +++ libs/vkd3d-shader/ir.c | 17 +++++++++++++++++ libs/vkd3d-shader/tpf.c | 3 +++ libs/vkd3d-shader/vkd3d_shader_main.c | 20 ++++++++++++++++++++ libs/vkd3d-shader/vkd3d_shader_private.h | 9 +++++++++ 7 files changed, 59 insertions(+) diff --git a/gitlab/build.yml b/gitlab/build.yml index 780d645b..209e8af7 100644 --- a/gitlab/build.yml +++ b/gitlab/build.yml @@ -31,12 +31,14 @@ build-radv-64: - amd-gpu variables: VK_LOADER_DRIVERS_SELECT: 'radeon_*' + VKD3D_SHADER_CONFIG: 'force_validation' build-llvmpipe-64: extends: .build allow_failure: true variables: VK_LOADER_DRIVERS_SELECT: 'lvp_*' + VKD3D_SHADER_CONFIG: 'force_validation' build-radv-32: extends: .build @@ -45,6 +47,7 @@ build-radv-32: variables: VK_LOADER_DRIVERS_SELECT: 'radeon_*' CC: 'gcc -m32' + VKD3D_SHADER_CONFIG: 'force_validation' build-llvmpipe-32: extends: .build @@ -52,3 +55,4 @@ build-llvmpipe-32: variables: VK_LOADER_DRIVERS_SELECT: 'lvp_*' CC: 'gcc -m32' + VKD3D_SHADER_CONFIG: 'force_validation' diff --git a/libs/vkd3d-shader/d3dbc.c b/libs/vkd3d-shader/d3dbc.c index 1b993c63..f6c3b651 100644 --- a/libs/vkd3d-shader/d3dbc.c +++ b/libs/vkd3d-shader/d3dbc.c @@ -1339,6 +1339,9 @@ int vkd3d_shader_sm1_parser_create(const struct vkd3d_shader_compile_info *compi for (i = 0; i < ARRAY_SIZE(sm1->p.shader_desc.flat_constant_count); ++i) sm1->p.shader_desc.flat_constant_count[i].external = get_external_constant_count(sm1, i); + if (!sm1->p.failed) + vsir_validate(&sm1->p); + if (sm1->p.failed) { WARN("Failed to parse shader.\n"); diff --git a/libs/vkd3d-shader/dxil.c b/libs/vkd3d-shader/dxil.c index 128f7b16..3b54f800 100644 --- a/libs/vkd3d-shader/dxil.c +++ b/libs/vkd3d-shader/dxil.c @@ -2953,6 +2953,9 @@ int vkd3d_shader_sm6_parser_create(const struct vkd3d_shader_compile_info *compi compile_info->source_name, message_context); vkd3d_free(byte_code); + if (!sm6->p.failed && ret >= 0) + vsir_validate(&sm6->p); + if (sm6->p.failed && ret >= 0) ret = VKD3D_ERROR_INVALID_SHADER; diff --git a/libs/vkd3d-shader/ir.c b/libs/vkd3d-shader/ir.c index b361420a..9bfb0f01 100644 --- a/libs/vkd3d-shader/ir.c +++ b/libs/vkd3d-shader/ir.c @@ -1292,5 +1292,22 @@ enum vkd3d_result vkd3d_shader_normalise(struct vkd3d_shader_parser *parser, if (result >= 0 && TRACE_ON()) vkd3d_shader_trace(instructions, &parser->shader_version); + if (result >= 0 && !parser->failed) + vsir_validate(parser); + + if (result >= 0 && parser->failed) + result = VKD3D_ERROR_INVALID_SHADER; + return result; } + +struct validation_context +{ + struct vkd3d_shader_parser *parser; +}; + +void vsir_validate(struct vkd3d_shader_parser *parser) +{ + if (!(parser->config_flags & VKD3D_SHADER_CONFIG_FLAG_FORCE_VALIDATION)) + return; +} diff --git a/libs/vkd3d-shader/tpf.c b/libs/vkd3d-shader/tpf.c index 228de2d1..859b3e61 100644 --- a/libs/vkd3d-shader/tpf.c +++ b/libs/vkd3d-shader/tpf.c @@ -2642,6 +2642,9 @@ int vkd3d_shader_sm4_parser_create(const struct vkd3d_shader_compile_info *compi if (sm4->p.shader_version.type == VKD3D_SHADER_TYPE_HULL && !sm4->has_control_point_phase && !sm4->p.failed) shader_sm4_validate_default_phase_index_ranges(sm4); + if (!sm4->p.failed) + vsir_validate(&sm4->p); + if (sm4->p.failed) { WARN("Failed to parse shader.\n"); diff --git a/libs/vkd3d-shader/vkd3d_shader_main.c b/libs/vkd3d-shader/vkd3d_shader_main.c index 0c3422f6..7a509fb2 100644 --- a/libs/vkd3d-shader/vkd3d_shader_main.c +++ b/libs/vkd3d-shader/vkd3d_shader_main.c @@ -454,6 +454,25 @@ static void init_scan_signature_info(const struct vkd3d_shader_compile_info *inf } } +static const struct vkd3d_debug_option vkd3d_shader_config_options[] = +{ + {"force_validation", VKD3D_SHADER_CONFIG_FLAG_FORCE_VALIDATION}, /* force validation of internal shader representations */ +}; + +static uint64_t vkd3d_shader_init_config_flags(void) +{ + uint64_t config_flags; + const char *config; + + config = getenv("VKD3D_SHADER_CONFIG"); + config_flags = vkd3d_parse_debug_options(config, vkd3d_shader_config_options, ARRAY_SIZE(vkd3d_shader_config_options)); + + if (config_flags) + TRACE("VKD3D_SHADER_CONFIG='%s'.\n", config); + + return config_flags; +} + bool vkd3d_shader_parser_init(struct vkd3d_shader_parser *parser, struct vkd3d_shader_message_context *message_context, const char *source_name, const struct vkd3d_shader_version *version, const struct vkd3d_shader_parser_ops *ops, @@ -465,6 +484,7 @@ bool vkd3d_shader_parser_init(struct vkd3d_shader_parser *parser, parser->location.column = 0; parser->shader_version = *version; parser->ops = ops; + parser->config_flags = vkd3d_shader_init_config_flags(); return shader_instruction_array_init(&parser->instructions, instruction_reserve); } diff --git a/libs/vkd3d-shader/vkd3d_shader_private.h b/libs/vkd3d-shader/vkd3d_shader_private.h index 70055614..c847d73b 100644 --- a/libs/vkd3d-shader/vkd3d_shader_private.h +++ b/libs/vkd3d-shader/vkd3d_shader_private.h @@ -1095,6 +1095,11 @@ bool shader_instruction_array_clone_instruction(struct vkd3d_shader_instruction_ unsigned int dst, unsigned int src); void shader_instruction_array_destroy(struct vkd3d_shader_instruction_array *instructions); +enum vkd3d_shader_config_flags +{ + VKD3D_SHADER_CONFIG_FLAG_FORCE_VALIDATION = 0x00000001, +}; + struct vkd3d_shader_parser { struct vkd3d_shader_message_context *message_context; @@ -1105,6 +1110,8 @@ struct vkd3d_shader_parser struct vkd3d_shader_version shader_version; const struct vkd3d_shader_parser_ops *ops; struct vkd3d_shader_instruction_array instructions; + + uint64_t config_flags; }; struct vkd3d_shader_parser_ops @@ -1293,6 +1300,8 @@ int preproc_lexer_parse(const struct vkd3d_shader_compile_info *compile_info, int hlsl_compile_shader(const struct vkd3d_shader_code *hlsl, const struct vkd3d_shader_compile_info *compile_info, struct vkd3d_shader_code *out, struct vkd3d_shader_message_context *message_context); +void vsir_validate(struct vkd3d_shader_parser *parser); + static inline enum vkd3d_shader_component_type vkd3d_component_type_from_data_type( enum vkd3d_data_type data_type) { -- GitLab https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/317
From: Giovanni Mascellani <gmascellani(a)codeweavers.com> --- libs/vkd3d-shader/ir.c | 36 ++++++++++++++++++++++++ libs/vkd3d-shader/vkd3d_shader_private.h | 1 + 2 files changed, 37 insertions(+) diff --git a/libs/vkd3d-shader/ir.c b/libs/vkd3d-shader/ir.c index 9bfb0f01..7654689c 100644 --- a/libs/vkd3d-shader/ir.c +++ b/libs/vkd3d-shader/ir.c @@ -1304,10 +1304,46 @@ enum vkd3d_result vkd3d_shader_normalise(struct vkd3d_shader_parser *parser, struct validation_context { struct vkd3d_shader_parser *parser; + size_t instruction_idx; }; +static void VKD3D_PRINTF_FUNC(3, 4) validator_error(struct validation_context *ctx, + enum vkd3d_shader_error error, const char *format, ...) +{ + struct vkd3d_string_buffer buf; + va_list args; + + vkd3d_string_buffer_init(&buf); + + va_start(args, format); + vkd3d_string_buffer_vprintf(&buf, format, args); + va_end(args); + + vkd3d_shader_parser_error(ctx->parser, error, "instruction %zu: %s", ctx->instruction_idx + 1, buf.buffer); + + vkd3d_string_buffer_cleanup(&buf); +} + +static void vsir_validate_instruction(struct validation_context *ctx) +{ + const struct vkd3d_shader_instruction *instruction = &ctx->parser->instructions.elements[ctx->instruction_idx]; + + ctx->parser->location = instruction->location; + + if (instruction->handler_idx >= VKD3DSIH_INVALID) + { + validator_error(ctx, VKD3D_SHADER_ERROR_VSIR_INVALID_HANDLER, "Invalid instruction handler %#x.", + instruction->handler_idx); + } +} + void vsir_validate(struct vkd3d_shader_parser *parser) { + struct validation_context ctx = { .parser = parser }; + if (!(parser->config_flags & VKD3D_SHADER_CONFIG_FLAG_FORCE_VALIDATION)) return; + + for (ctx.instruction_idx = 0; ctx.instruction_idx < parser->instructions.count; ++ctx.instruction_idx) + vsir_validate_instruction(&ctx); } diff --git a/libs/vkd3d-shader/vkd3d_shader_private.h b/libs/vkd3d-shader/vkd3d_shader_private.h index c847d73b..fccec507 100644 --- a/libs/vkd3d-shader/vkd3d_shader_private.h +++ b/libs/vkd3d-shader/vkd3d_shader_private.h @@ -183,6 +183,7 @@ enum vkd3d_shader_error VKD3D_SHADER_WARNING_DXIL_UNHANDLED_INTRINSIC = 8305, VKD3D_SHADER_ERROR_VSIR_NOT_IMPLEMENTED = 9000, + VKD3D_SHADER_ERROR_VSIR_INVALID_HANDLER = 9001, }; enum vkd3d_shader_opcode -- GitLab https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/317
From: Giovanni Mascellani <gmascellani(a)codeweavers.com> --- libs/vkd3d-shader/ir.c | 27 ++++++++++++++++++++++++ libs/vkd3d-shader/vkd3d_shader_private.h | 1 + 2 files changed, 28 insertions(+) diff --git a/libs/vkd3d-shader/ir.c b/libs/vkd3d-shader/ir.c index 7654689c..bd3a721e 100644 --- a/libs/vkd3d-shader/ir.c +++ b/libs/vkd3d-shader/ir.c @@ -1324,12 +1324,39 @@ static void VKD3D_PRINTF_FUNC(3, 4) validator_error(struct validation_context *c vkd3d_string_buffer_cleanup(&buf); } +static void vsir_validate_register(struct validation_context *ctx, + const struct vkd3d_shader_register *reg) +{ + if (reg->type >= VKD3DSPR_COUNT) + validator_error(ctx, VKD3D_SHADER_ERROR_VSIR_INVALID_REGISTER_TYPE, "Invalid register type %#x.", + reg->type); +} + +static void vsir_validate_dst_param(struct validation_context *ctx, + const struct vkd3d_shader_dst_param *dst) +{ + vsir_validate_register(ctx, &dst->reg); +} + +static void vsir_validate_src_param(struct validation_context *ctx, + const struct vkd3d_shader_src_param *src) +{ + vsir_validate_register(ctx, &src->reg); +} + static void vsir_validate_instruction(struct validation_context *ctx) { const struct vkd3d_shader_instruction *instruction = &ctx->parser->instructions.elements[ctx->instruction_idx]; + size_t i; ctx->parser->location = instruction->location; + for (i = 0; i < instruction->dst_count; ++i) + vsir_validate_dst_param(ctx, &instruction->dst[i]); + + for (i = 0; i < instruction->src_count; ++i) + vsir_validate_src_param(ctx, &instruction->src[i]); + if (instruction->handler_idx >= VKD3DSIH_INVALID) { validator_error(ctx, VKD3D_SHADER_ERROR_VSIR_INVALID_HANDLER, "Invalid instruction handler %#x.", diff --git a/libs/vkd3d-shader/vkd3d_shader_private.h b/libs/vkd3d-shader/vkd3d_shader_private.h index fccec507..aa5944db 100644 --- a/libs/vkd3d-shader/vkd3d_shader_private.h +++ b/libs/vkd3d-shader/vkd3d_shader_private.h @@ -184,6 +184,7 @@ enum vkd3d_shader_error VKD3D_SHADER_ERROR_VSIR_NOT_IMPLEMENTED = 9000, VKD3D_SHADER_ERROR_VSIR_INVALID_HANDLER = 9001, + VKD3D_SHADER_ERROR_VSIR_INVALID_REGISTER_TYPE = 9002, }; enum vkd3d_shader_opcode -- GitLab https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/317
From: Giovanni Mascellani <gmascellani(a)codeweavers.com> --- libs/vkd3d-shader/d3d_asm.c | 2 +- libs/vkd3d-shader/ir.c | 24 ++++++++++++++++++++++++ libs/vkd3d-shader/vkd3d_shader_private.h | 4 ++++ 3 files changed, 29 insertions(+), 1 deletion(-) diff --git a/libs/vkd3d-shader/d3d_asm.c b/libs/vkd3d-shader/d3d_asm.c index d72402eb..1dde34c0 100644 --- a/libs/vkd3d-shader/d3d_asm.c +++ b/libs/vkd3d-shader/d3d_asm.c @@ -1374,7 +1374,7 @@ static void shader_dump_ins_modifiers(struct vkd3d_d3d_asm_compiler *compiler, if (mmask & VKD3DSPDM_PARTIALPRECISION) shader_addline(buffer, "_pp"); if (mmask & VKD3DSPDM_MSAMPCENTROID) shader_addline(buffer, "_centroid"); - mmask &= ~(VKD3DSPDM_SATURATE | VKD3DSPDM_PARTIALPRECISION | VKD3DSPDM_MSAMPCENTROID); + mmask &= ~VKD3DSPDM_MASK; if (mmask) FIXME("Unrecognised modifier %#x.\n", mmask); } diff --git a/libs/vkd3d-shader/ir.c b/libs/vkd3d-shader/ir.c index bd3a721e..e36bd381 100644 --- a/libs/vkd3d-shader/ir.c +++ b/libs/vkd3d-shader/ir.c @@ -1336,6 +1336,30 @@ static void vsir_validate_dst_param(struct validation_context *ctx, const struct vkd3d_shader_dst_param *dst) { vsir_validate_register(ctx, &dst->reg); + + if (dst->write_mask & ~VKD3DSP_WRITEMASK_ALL) + validator_error(ctx, VKD3D_SHADER_ERROR_VSIR_INVALID_WRITE_MASK, "Destination has invalid write mask %#x.", + dst->write_mask); + + if (dst->modifiers & ~VKD3DSPDM_MASK) + validator_error(ctx, VKD3D_SHADER_ERROR_VSIR_INVALID_MODIFIERS, "Destination has invalid modifiers %#x.", + dst->modifiers); + + switch (dst->shift) + { + case 0: + case 1: + case 2: + case 3: + case 13: + case 14: + case 15: + break; + + default: + validator_error(ctx, VKD3D_SHADER_ERROR_VSIR_INVALID_SHIFT, "Destination has invalid shift %#x.", + dst->shift); + } } static void vsir_validate_src_param(struct validation_context *ctx, diff --git a/libs/vkd3d-shader/vkd3d_shader_private.h b/libs/vkd3d-shader/vkd3d_shader_private.h index aa5944db..85a1d201 100644 --- a/libs/vkd3d-shader/vkd3d_shader_private.h +++ b/libs/vkd3d-shader/vkd3d_shader_private.h @@ -185,6 +185,9 @@ enum vkd3d_shader_error VKD3D_SHADER_ERROR_VSIR_NOT_IMPLEMENTED = 9000, VKD3D_SHADER_ERROR_VSIR_INVALID_HANDLER = 9001, VKD3D_SHADER_ERROR_VSIR_INVALID_REGISTER_TYPE = 9002, + VKD3D_SHADER_ERROR_VSIR_INVALID_WRITE_MASK = 9003, + VKD3D_SHADER_ERROR_VSIR_INVALID_MODIFIERS = 9004, + VKD3D_SHADER_ERROR_VSIR_INVALID_SHIFT = 9005, }; enum vkd3d_shader_opcode @@ -601,6 +604,7 @@ enum vkd3d_shader_dst_modifier VKD3DSPDM_SATURATE = 1, VKD3DSPDM_PARTIALPRECISION = 2, VKD3DSPDM_MSAMPCENTROID = 4, + VKD3DSPDM_MASK = 7, }; enum vkd3d_shader_interpolation_mode -- GitLab https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/317
From: Giovanni Mascellani <gmascellani(a)codeweavers.com> --- libs/vkd3d-shader/ir.c | 8 ++++++++ libs/vkd3d-shader/vkd3d_shader_private.h | 2 ++ 2 files changed, 10 insertions(+) diff --git a/libs/vkd3d-shader/ir.c b/libs/vkd3d-shader/ir.c index e36bd381..bc8bfcd4 100644 --- a/libs/vkd3d-shader/ir.c +++ b/libs/vkd3d-shader/ir.c @@ -1366,6 +1366,14 @@ static void vsir_validate_src_param(struct validation_context *ctx, const struct vkd3d_shader_src_param *src) { vsir_validate_register(ctx, &src->reg); + + if (src->swizzle & ~0x03030303u) + validator_error(ctx, VKD3D_SHADER_ERROR_VSIR_INVALID_SWIZZLE, "Source has invalid swizzle %#x.", + src->swizzle); + + if (src->modifiers >= VKD3DSPSM_COUNT) + validator_error(ctx, VKD3D_SHADER_ERROR_VSIR_INVALID_MODIFIERS, "Source has invalid modifiers %#x.", + src->modifiers); } static void vsir_validate_instruction(struct validation_context *ctx) diff --git a/libs/vkd3d-shader/vkd3d_shader_private.h b/libs/vkd3d-shader/vkd3d_shader_private.h index 85a1d201..5fd93091 100644 --- a/libs/vkd3d-shader/vkd3d_shader_private.h +++ b/libs/vkd3d-shader/vkd3d_shader_private.h @@ -188,6 +188,7 @@ enum vkd3d_shader_error VKD3D_SHADER_ERROR_VSIR_INVALID_WRITE_MASK = 9003, VKD3D_SHADER_ERROR_VSIR_INVALID_MODIFIERS = 9004, VKD3D_SHADER_ERROR_VSIR_INVALID_SHIFT = 9005, + VKD3D_SHADER_ERROR_VSIR_INVALID_SWIZZLE = 9006, }; enum vkd3d_shader_opcode @@ -590,6 +591,7 @@ enum vkd3d_shader_src_modifier VKD3DSPSM_ABS = 11, VKD3DSPSM_ABSNEG = 12, VKD3DSPSM_NOT = 13, + VKD3DSPSM_COUNT, }; #define VKD3DSP_WRITEMASK_0 0x1u /* .x r */ -- GitLab https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/317
The new revision should address the latest batch of comments. Also, the validator found the first potential bug: in SM1, opcode DCL apparently expects two sources, but they were not being parsed (nor, evidently, used). The outcome was that the corresponding entries in `src` were not initialised, and had random values which triggered the validator. I am not sure what the correct fix for that it. For the moment I just pretend that DCL in VSIR doesn't have any source (I see the validator as an internal thing, so if those sources are eventually found out to mean something, we can change the convention and the validator). -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/317#note_44699
Francisco Casas (@fcasas) commented about libs/vkd3d-shader/ir.c:
/* fall through */ case VKD3DSIH_DCL_INPUT_PS: case VKD3DSIH_DCL_OUTPUT: - keep = shader_dst_param_io_normalise(&ins->declaration.dst, true, normaliser); + if (!shader_dst_param_io_normalise(&ins->declaration.dst, true, normaliser)) + vkd3d_shader_instruction_make_nop(ins); break; case VKD3DSIH_DCL_INPUT_SGV: case VKD3DSIH_DCL_INPUT_SIV: case VKD3DSIH_DCL_INPUT_PS_SGV: case VKD3DSIH_DCL_INPUT_PS_SIV: case VKD3DSIH_DCL_OUTPUT_SIV: - keep = shader_dst_param_io_normalise(&ins->declaration.register_semantic.reg, true, - normaliser); + if (!shader_dst_param_io_normalise(&ins->declaration.register_semantic.reg, true, normaliser)) + vkd3d_shader_instruction_make_nop(ins);
It calls my attention the replacement from shader_instruction_init() to vkd3d_shader_instruction_make_nop(), since the latter doesn't initialize all the instruction fields to zero. Can this change the behavior? -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/317#note_44840
Francisco Casas (@fcasas) commented about libs/vkd3d-shader/ir.c:
reg->immconst_type = VKD3D_IMMCONST_SCALAR; }
-void shader_instruction_init(struct vkd3d_shader_instruction *ins, enum vkd3d_shader_opcode handler_idx) +void vsir_instruction_init(struct vkd3d_shader_instruction *ins, const struct vkd3d_shader_location *location,
Maybe it would be better to put the renaming in a separate patch, it could simplify rebases. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/317#note_44841
Francisco Casas (@fcasas) commented about libs/vkd3d-shader/ir.c:
if (ins->handler_idx == VKD3DSIH_RET) { + normaliser->last_ret_location = ins->location;
I don't understand ir.c very well yet. That said, I am not sure if flattener_eliminate_phase_related_dcls() is the right place to initialize `normaliser.last_ret_location`. Maybe initializing this value deserves a separated function, lest we forget that we are charging flattener_eliminate_phase_related_dcls() with this additional responsibility. Also, it seems to me that it could be a local variable in instruction_array_flatten_hull_shader_phases() instead of a member of the normalizer, unless there is another place where it could be used later. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/317#note_44842
On Sun Sep 10 08:12:33 2023 +0000, Francisco Casas wrote:
It calls my attention the replacement from shader_instruction_init() to vkd3d_shader_instruction_make_nop(), since the latter doesn't initialize all the instruction fields to zero. Can this change the behavior? I wish the documentation for VSIR was clearer on this (i.e., it existed, at least; which is part of why I began my effort to write the validator), but my understanding is that once the handler is NOP, no other field of the VSIR instruction should even be read. But the disassembler might be an exception and, even more in view of the validator, it's better to be precise. I'll change so that the whole instruction is reset to zero, except for the location.
-- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/317#note_44890
On Sun Sep 10 08:14:07 2023 +0000, Francisco Casas wrote:
I don't understand ir.c very well yet. That said, I am not sure if flattener_eliminate_phase_related_dcls() is the right place to initialize `normaliser.last_ret_location`. Maybe initializing this value deserves a separated function, lest we forget that we are charging flattener_eliminate_phase_related_dcls() with this additional responsibility. Also, it seems to me that it could be a local variable in instruction_array_flatten_hull_shader_phases() instead of a member of the normalizer, unless there is another place where it could be used later. I don't see this as very problematic: `flattener_eliminate_phase_related_dcls()` is just a helper for `instruction_array_flatten_hull_shader_phases()`, so both functions are basically in the same "domain of responsibility". Same thing for `struct hull_flattener`: it's just a helper structure to share variables between the main function and its helpers without passing a lot of arguments to each call.
-- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/317#note_44891
participants (3)
-
Francisco Casas (＠fcasas) -
Giovanni Mascellani
-
Giovanni Mascellani (＠giomasce)