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**
-- v7: 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(). vkd3d-shader/ir: Fully reinitialize an instruction when making it a NOP. vkd3d-shader: Rename shader_instruction_init().
From: Giovanni Mascellani gmascellani@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) {
From: Giovanni Mascellani gmascellani@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,
From: Giovanni Mascellani gmascellani@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,
From: Giovanni Mascellani gmascellani@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 661efde7..e7bced09 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) {
From: Giovanni Mascellani gmascellani@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 e7bced09..84435161 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,
From: Giovanni Mascellani gmascellani@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);
From: Giovanni Mascellani gmascellani@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");
From: Giovanni Mascellani gmascellani@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 84435161..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; }
- 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;
From: Giovanni Mascellani gmascellani@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 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) {
From: Giovanni Mascellani gmascellani@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
From: Giovanni Mascellani gmascellani@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
From: Giovanni Mascellani gmascellani@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
From: Giovanni Mascellani gmascellani@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 */
This merge request was approved by Francisco Casas.
I feel like we've probably had this discussion before, but do you see any place for assert()?
I think it's fine in programs like vkd3d-compiler, vkd3d-dxbc, or the demos. I generally don't think assert() is appropriate in libraries like libvkd3d or libvkd3d-shader, no.
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;
They're not source parameters. These are the "usage_token" and "dst_token" read by shader_sm1_read_semantic(). src_count is set to 2 in order to make shader_sm1_skip_opcode() do the right thing for 1.x shaders, but arguably that function has outlived its usefulness.
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.
-
Francisco Casas (＠fcasas) -
Giovanni Mascellani
-
Giovanni Mascellani (＠giomasce)
-
Henri Verbeet (＠hverbeet)