[PATCH v5 0/9] 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** -- v5: vkd3d-shader: Validate source parameters. vkd3d-shader: Validate destination parameters. vkd3d-shader: Validate register types. vkd3d-shader: Validate instruction handlers. vkd3d-shader: Introduce a boilerplate to validate the generated IR. 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 2b02d51f..0c625d9b 100644 --- a/libs/vkd3d-shader/d3dbc.c +++ b/libs/vkd3d-shader/d3dbc.c @@ -1333,12 +1333,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 | 25 ++++++++++++++++-------- libs/vkd3d-shader/tpf.c | 2 +- libs/vkd3d-shader/vkd3d_shader_private.h | 16 ++++++++------- 5 files changed, 30 insertions(+), 19 deletions(-) diff --git a/libs/vkd3d-shader/d3dbc.c b/libs/vkd3d-shader/d3dbc.c index 0c625d9b..440e6abd 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; + shader_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..665cd60d 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); + shader_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); + shader_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 6d7c8965..7aa2fa0f 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 shader_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); + shader_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); + shader_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); + shader_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); + shader_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: @@ -1013,6 +1018,7 @@ static void shader_src_param_io_normalise(struct vkd3d_shader_src_param *src_par static void shader_instruction_normalise_io_params(struct vkd3d_shader_instruction *ins, struct io_normaliser *normaliser) { + struct vkd3d_shader_location location; struct vkd3d_shader_register *reg; bool keep = true; unsigned int i; @@ -1062,7 +1068,10 @@ static void shader_instruction_normalise_io_params(struct vkd3d_shader_instructi } if (!keep) - shader_instruction_init(ins, VKD3DSIH_NOP); + { + location = ins->location; + shader_instruction_init(ins, &location, 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 6d8c4507..92bcf65e 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; + shader_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..5ae5cfda 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 shader_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 440e6abd..dc7fd984 100644 --- a/libs/vkd3d-shader/d3dbc.c +++ b/libs/vkd3d-shader/d3dbc.c @@ -1336,6 +1336,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) + vkd3d_shader_validate_vsir(&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 665cd60d..a26d79d5 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) + vkd3d_shader_validate_vsir(&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 7aa2fa0f..9324121c 100644 --- a/libs/vkd3d-shader/ir.c +++ b/libs/vkd3d-shader/ir.c @@ -1299,5 +1299,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) + vkd3d_shader_validate_vsir(parser); + + if (result >= 0 && parser->failed) + result = VKD3D_ERROR_INVALID_SHADER; + return result; } + +struct validation_context +{ + struct vkd3d_shader_parser *parser; +}; + +void vkd3d_shader_validate_vsir(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 92bcf65e..775d1d81 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) + vkd3d_shader_validate_vsir(&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 5ae5cfda..ccfa84eb 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 vkd3d_shader_validate_vsir(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 | 43 ++++++++++++++++++++++++ libs/vkd3d-shader/vkd3d_shader_private.h | 1 + 2 files changed, 44 insertions(+) diff --git a/libs/vkd3d-shader/ir.c b/libs/vkd3d-shader/ir.c index 9324121c..ce725672 100644 --- a/libs/vkd3d-shader/ir.c +++ b/libs/vkd3d-shader/ir.c @@ -1311,10 +1311,53 @@ 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 validator_error(struct validation_context *ctx, enum vkd3d_shader_error error, + const char *format, ...) VKD3D_PRINTF_FUNC(3, 4); + +static void 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 vkd3d_shader_validate_instruction(struct validation_context *ctx) +{ + const struct vkd3d_shader_instruction *instruction = &ctx->parser->instructions.elements[ctx->instruction_idx]; + + ctx->parser->location = instruction->location; + + switch (instruction->handler_idx) + { + case VKD3DSIH_INVALID: + validator_error(ctx, VKD3D_SHADER_ERROR_VSIR_INVALID_HANDLER, "Invalid handler.\n"); + break; + + default: + break; + } +} + void vkd3d_shader_validate_vsir(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) + vkd3d_shader_validate_instruction(&ctx); } diff --git a/libs/vkd3d-shader/vkd3d_shader_private.h b/libs/vkd3d-shader/vkd3d_shader_private.h index ccfa84eb..3525f6f4 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 | 34 ++++++++++++++++++++++++ libs/vkd3d-shader/vkd3d_shader_private.h | 1 + 2 files changed, 35 insertions(+) diff --git a/libs/vkd3d-shader/ir.c b/libs/vkd3d-shader/ir.c index ce725672..2015ab25 100644 --- a/libs/vkd3d-shader/ir.c +++ b/libs/vkd3d-shader/ir.c @@ -1334,12 +1334,46 @@ static void validator_error(struct validation_context *ctx, enum vkd3d_shader_er vkd3d_string_buffer_cleanup(&buf); } +static void vkd3d_shader_validate_register(struct validation_context *ctx, + const struct vkd3d_shader_register *reg) +{ + switch (reg->type) + { + case VKD3DSPR_COUNT: + case VKD3DSPR_INVALID: + validator_error(ctx, VKD3D_SHADER_ERROR_VSIR_INVALID_REGISTER_TYPE, "Invalid register type.\n"); + break; + + default: + break; + } +} + +static void vkd3d_shader_validate_dst_param(struct validation_context *ctx, + const struct vkd3d_shader_dst_param *dst) +{ + vkd3d_shader_validate_register(ctx, &dst->reg); +} + +static void vkd3d_shader_validate_src_param(struct validation_context *ctx, + const struct vkd3d_shader_src_param *src) +{ + vkd3d_shader_validate_register(ctx, &src->reg); +} + static void vkd3d_shader_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) + vkd3d_shader_validate_dst_param(ctx, &instruction->dst[i]); + + for (i = 0; i < instruction->src_count; ++i) + vkd3d_shader_validate_src_param(ctx, &instruction->src[i]); + switch (instruction->handler_idx) { case VKD3DSIH_INVALID: diff --git a/libs/vkd3d-shader/vkd3d_shader_private.h b/libs/vkd3d-shader/vkd3d_shader_private.h index 3525f6f4..cf106693 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 2015ab25..df04b852 100644 --- a/libs/vkd3d-shader/ir.c +++ b/libs/vkd3d-shader/ir.c @@ -1353,6 +1353,30 @@ static void vkd3d_shader_validate_dst_param(struct validation_context *ctx, const struct vkd3d_shader_dst_param *dst) { vkd3d_shader_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.\n", + dst->write_mask); + + if (dst->modifiers & ~VKD3DSPDM_MASK) + validator_error(ctx, VKD3D_SHADER_ERROR_VSIR_INVALID_MODIFIERS, "Destination has invalid modifiers %#x.\n", + 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.\n", + dst->shift); + } } static void vkd3d_shader_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 cf106693..e0a0eec3 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 df04b852..3e634119 100644 --- a/libs/vkd3d-shader/ir.c +++ b/libs/vkd3d-shader/ir.c @@ -1383,6 +1383,14 @@ static void vkd3d_shader_validate_src_param(struct validation_context *ctx, const struct vkd3d_shader_src_param *src) { vkd3d_shader_validate_register(ctx, &src->reg); + + if (src->swizzle & ~0x03030303u) + validator_error(ctx, VKD3D_SHADER_ERROR_VSIR_INVALID_SWIZZLE, "Source has invalid swizzle %#x.\n", + src->swizzle); + + if (src->modifiers >= VKD3DSPSM_COUNT) + validator_error(ctx, VKD3D_SHADER_ERROR_VSIR_INVALID_MODIFIERS, "Source has invalid modifiers %#x.\n", + src->modifiers); } static void vkd3d_shader_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 e0a0eec3..e29d88ea 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
This seems generally fine to me. Couple of points:
```diff -void shader_instruction_init(struct vkd3d_shader_instruction *ins, enum vkd3d_shader_opcode handler_idx) +void shader_instruction_init(struct vkd3d_shader_instruction *ins, const struct vkd3d_shader_location *location, + enum vkd3d_shader_opcode handler_idx) ```
If we're touching it anyway, we might as well rename it to vsir_instruction_init(), IMO.
```diff @@ -1062,7 +1068,10 @@ static void shader_instruction_normalise_io_params(struct vkd3d_shader_instructi }
if (!keep) - shader_instruction_init(ins, VKD3DSIH_NOP); + { + location = ins->location; + shader_instruction_init(ins, &location, VKD3DSIH_NOP); + } }
static enum vkd3d_result instruction_array_normalise_io_registers(struct vkd3d_shader_instruction_array *instructions,
```
This is not introduced by this series, so in that regard it's fine, but this looks odd. In particular: - Why shader_instruction_init(..., VKD3DSIH_NOP) instead of vkd3d_shader_instruction_make_nop()? - Why the "keep" variable if we could just nop out the instruction in the two places that might set it to false?
```diff +void vkd3d_shader_validate_vsir(struct vkd3d_shader_parser *parser) ```
I.e., vsir_validate()? :)
```diff +static void validator_error(struct validation_context *ctx, enum vkd3d_shader_error error, + const char *format, ...) VKD3D_PRINTF_FUNC(3, 4); + +static void validator_error(struct validation_context *ctx, enum vkd3d_shader_error error, + const char *format, ...) +{ ```
I.e., ```c static void VKD3D_PRINTF_FUNC(3, 4) validator_error(struct validation_context *ctx, enum vkd3d_shader_error error, const char *format, ...) { ... ``` right?
```diff + case VKD3DSIH_INVALID: + validator_error(ctx, VKD3D_SHADER_ERROR_VSIR_INVALID_HANDLER, "Invalid handler.\n"); ```
Note that vkd3d_shader_verror() already adds a newline, so validator_error() shouldn't need to add an extra one. That happens in a few other places in this series as well.
```diff +static void vkd3d_shader_validate_register(struct validation_context *ctx, + const struct vkd3d_shader_register *reg) +{ + switch (reg->type) + { + case VKD3DSPR_COUNT: + case VKD3DSPR_INVALID: + validator_error(ctx, VKD3D_SHADER_ERROR_VSIR_INVALID_REGISTER_TYPE, "Invalid register type.\n"); + break; + + default: + break; + } +} ```
Should we just check for "reg->type >= VKD3DSPR_COUNT" and print the actual value here? -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/317#note_44562
That's not that choice here though. I have no issue with aborting compilation when validation fails; the issue is with using assert() to do that, because it may either not trigger (i.e., in NDEBUG builds) or potentially escalate the consequences by calling abort() in an inappropriate context.
I feel like we've probably had this discussion before, but do you see any place for assert()? I see it as a way to communicate and/or double-check the internal assumptions the code is making; in that sense it makes sense that it's disabled on NDEBUG, and also that it aborts the program entirely, since it's only meant to be a developer tool. I get the impression you don't see that as sensible, though—and that *any* internal assumptions should either just not be written out, or should be validated *always*, even for "release" builds, and shouldn't crash the program? -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/317#note_44590
participants (4)
-
Giovanni Mascellani -
Giovanni Mascellani (＠giomasce)
-
Henri Verbeet (＠hverbeet)
-
Zebediah Figura (＠zfigura)