[PATCH v8 0/13] 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** -- v8: 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: Skip DCL semantic tokens properly. https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/317
From: Giovanni Mascellani <gmascellani(a)codeweavers.com> --- libs/vkd3d-shader/d3dbc.c | 2 +- libs/vkd3d-shader/dxil.c | 4 ++-- libs/vkd3d-shader/ir.c | 12 ++++++------ libs/vkd3d-shader/tpf.c | 2 +- libs/vkd3d-shader/vkd3d_shader_private.h | 2 +- 5 files changed, 11 insertions(+), 11 deletions(-) diff --git a/libs/vkd3d-shader/d3dbc.c b/libs/vkd3d-shader/d3dbc.c index 2b02d51f..661efde7 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, 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 666d8b08..b315d645 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, 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, 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 6d7c8965..92f09c49 100644 --- a/libs/vkd3d-shader/ir.c +++ b/libs/vkd3d-shader/ir.c @@ -313,7 +313,7 @@ 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, enum vkd3d_shader_opcode handler_idx) { memset(ins, 0, sizeof(*ins)); ins->handler_idx = handler_idx; @@ -343,7 +343,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++], VKD3DSIH_RET); } *src_instructions = flattener.instructions; @@ -422,7 +422,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, VKD3DSIH_HS_CONTROL_POINT_PHASE); ins->flags = 1; ++ins; @@ -434,13 +434,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, 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, VKD3DSIH_DCL_INPUT); param = &ins->declaration.dst; } @@ -1062,7 +1062,7 @@ static void shader_instruction_normalise_io_params(struct vkd3d_shader_instructi } if (!keep) - shader_instruction_init(ins, VKD3DSIH_NOP); + vsir_instruction_init(ins, VKD3DSIH_NOP); } static enum vkd3d_result instruction_array_normalise_io_registers(struct vkd3d_shader_instruction_array *instructions, diff --git a/libs/vkd3d-shader/tpf.c b/libs/vkd3d-shader/tpf.c index 7949be15..7557f638 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, 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..fb51516e 100644 --- a/libs/vkd3d-shader/vkd3d_shader_private.h +++ b/libs/vkd3d-shader/vkd3d_shader_private.h @@ -1008,7 +1008,7 @@ 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, enum vkd3d_shader_opcode handler_idx); static inline bool vkd3d_shader_instruction_has_texel_offset(const struct vkd3d_shader_instruction *ins) { -- GitLab https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/317
From: Giovanni Mascellani <gmascellani(a)codeweavers.com> --- libs/vkd3d-shader/ir.c | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/libs/vkd3d-shader/ir.c b/libs/vkd3d-shader/ir.c index 92f09c49..df1b47d2 100644 --- a/libs/vkd3d-shader/ir.c +++ b/libs/vkd3d-shader/ir.c @@ -31,11 +31,7 @@ static bool shader_instruction_is_dcl(const struct vkd3d_shader_instruction *ins static void vkd3d_shader_instruction_make_nop(struct vkd3d_shader_instruction *ins) { - ins->handler_idx = VKD3DSIH_NOP; - ins->dst_count = 0; - ins->src_count = 0; - ins->dst = NULL; - ins->src = NULL; + vsir_instruction_init(ins, VKD3DSIH_NOP); } static void shader_register_eliminate_phase_addressing(struct vkd3d_shader_register *reg, -- GitLab 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 df1b47d2..c84f1c2a 100644 --- a/libs/vkd3d-shader/ir.c +++ b/libs/vkd3d-shader/ir.c @@ -1010,7 +1010,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) @@ -1029,15 +1028,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: @@ -1056,9 +1056,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) - vsir_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> --- libs/vkd3d-shader/d3dbc.c | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/libs/vkd3d-shader/d3dbc.c b/libs/vkd3d-shader/d3dbc.c index 661efde7..9d76eb40 100644 --- a/libs/vkd3d-shader/d3dbc.c +++ b/libs/vkd3d-shader/d3dbc.c @@ -261,7 +261,7 @@ static const struct vkd3d_sm1_opcode_info vs_opcode_table[] = {VKD3D_SM1_OP_M3x3, 1, 2, VKD3DSIH_M3x3}, {VKD3D_SM1_OP_M3x2, 1, 2, VKD3DSIH_M3x2}, /* Declarations */ - {VKD3D_SM1_OP_DCL, 0, 2, VKD3DSIH_DCL}, + {VKD3D_SM1_OP_DCL, 0, 0, VKD3DSIH_DCL}, /* Constant definitions */ {VKD3D_SM1_OP_DEF, 1, 1, VKD3DSIH_DEF}, {VKD3D_SM1_OP_DEFB, 1, 1, VKD3DSIH_DEFB}, @@ -328,7 +328,7 @@ static const struct vkd3d_sm1_opcode_info ps_opcode_table[] = {VKD3D_SM1_OP_M3x3, 1, 2, VKD3DSIH_M3x3}, {VKD3D_SM1_OP_M3x2, 1, 2, VKD3DSIH_M3x2}, /* Declarations */ - {VKD3D_SM1_OP_DCL, 0, 2, VKD3DSIH_DCL}, + {VKD3D_SM1_OP_DCL, 0, 0, VKD3DSIH_DCL}, /* Constant definitions */ {VKD3D_SM1_OP_DEF, 1, 1, VKD3DSIH_DEF}, {VKD3D_SM1_OP_DEFB, 1, 1, VKD3DSIH_DEFB}, @@ -853,6 +853,14 @@ static void shader_sm1_skip_opcode(const struct vkd3d_shader_sm1_parser *sm1, co return; } + /* DCL instructions do not have sources or destinations, but they + * read two tokens to a semantic. See + * shader_sm1_read_semantic(). */ + if (opcode_info->vkd3d_opcode == VKD3DSIH_DCL) + { + *ptr += 2; + } + *ptr += (opcode_info->dst_count + opcode_info->src_count); } -- 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 9d76eb40..057ea1eb 100644 --- a/libs/vkd3d-shader/d3dbc.c +++ b/libs/vkd3d-shader/d3dbc.c @@ -1341,12 +1341,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 7557f638..62b93f49 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 b315d645..510183f9 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 | 23 +++++++++++++++-------- libs/vkd3d-shader/tpf.c | 2 +- libs/vkd3d-shader/vkd3d_shader_private.h | 16 +++++++++------- 5 files changed, 28 insertions(+), 19 deletions(-) diff --git a/libs/vkd3d-shader/d3dbc.c b/libs/vkd3d-shader/d3dbc.c index 057ea1eb..1fd941e0 100644 --- a/libs/vkd3d-shader/d3dbc.c +++ b/libs/vkd3d-shader/d3dbc.c @@ -1098,7 +1098,7 @@ static void shader_sm1_read_instruction(struct vkd3d_shader_sm1_parser *sm1, str goto fail; } - vsir_instruction_init(ins, 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 510183f9..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); - vsir_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; } - vsir_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 c84f1c2a..3222834b 100644 --- a/libs/vkd3d-shader/ir.c +++ b/libs/vkd3d-shader/ir.c @@ -31,7 +31,9 @@ static bool shader_instruction_is_dcl(const struct vkd3d_shader_instruction *ins static void vkd3d_shader_instruction_make_nop(struct vkd3d_shader_instruction *ins) { - vsir_instruction_init(ins, VKD3DSIH_NOP); + struct vkd3d_shader_location location = ins->location; + + vsir_instruction_init(ins, &location, VKD3DSIH_NOP); } static void shader_register_eliminate_phase_addressing(struct vkd3d_shader_register *reg, @@ -157,6 +159,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) @@ -229,6 +232,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)) { @@ -309,9 +313,11 @@ void shader_register_init(struct vkd3d_shader_register *reg, enum vkd3d_shader_r reg->immconst_type = VKD3D_IMMCONST_SCALAR; } -void vsir_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; } @@ -339,7 +345,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; - vsir_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; @@ -400,7 +406,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; @@ -418,7 +425,7 @@ static enum vkd3d_result control_point_normaliser_emit_hs_input(struct control_p normaliser->instructions.count += count; ins = &normaliser->instructions.elements[dst]; - vsir_instruction_init(ins, VKD3DSIH_HS_CONTROL_POINT_PHASE); + vsir_instruction_init(ins, location, VKD3DSIH_HS_CONTROL_POINT_PHASE); ins->flags = 1; ++ins; @@ -430,13 +437,13 @@ static enum vkd3d_result control_point_normaliser_emit_hs_input(struct control_p if (e->sysval_semantic != VKD3D_SHADER_SV_NONE) { - vsir_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 { - vsir_instruction_init(ins, VKD3DSIH_DCL_INPUT); + vsir_instruction_init(ins, location, VKD3DSIH_DCL_INPUT); param = &ins->declaration.dst; } @@ -507,7 +514,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 62b93f49..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; } - vsir_instruction_init(ins, 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 fb51516e..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 vsir_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 1fd941e0..f377076e 100644 --- a/libs/vkd3d-shader/d3dbc.c +++ b/libs/vkd3d-shader/d3dbc.c @@ -1344,6 +1344,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 3222834b..ac2b8d12 100644 --- a/libs/vkd3d-shader/ir.c +++ b/libs/vkd3d-shader/ir.c @@ -1290,5 +1290,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 ac2b8d12..ab188481 100644 --- a/libs/vkd3d-shader/ir.c +++ b/libs/vkd3d-shader/ir.c @@ -1302,10 +1302,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 ab188481..e50148e7 100644 --- a/libs/vkd3d-shader/ir.c +++ b/libs/vkd3d-shader/ir.c @@ -1322,12 +1322,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 e50148e7..22cd01b2 100644 --- a/libs/vkd3d-shader/ir.c +++ b/libs/vkd3d-shader/ir.c @@ -1334,6 +1334,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 22cd01b2..79295794 100644 --- a/libs/vkd3d-shader/ir.c +++ b/libs/vkd3d-shader/ir.c @@ -1364,6 +1364,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
I think setting src_count to 0 like this is fine if the comment is updated, but I wouldn't be opposed to instead setting src_count to 0 in the opcode info tables and adjusting sm1->ptr in either shader_sm1_skip_opcode() or shader_sm1_read_semantic() either.
I implemented skipping in `shader_sm1_skip_opcode()`. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/317#note_45017
Looks good to me like this. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/317#note_45104
participants (3)
-
Giovanni Mascellani -
Giovanni Mascellani (＠giomasce)
-
Henri Verbeet (＠hverbeet)