[PATCH v8 0/7] MR198: vkd3d-shader: Add more TPF validations.
Introduces an `idx_count` field to `struct vkd3d_shader_register`. Another patch set is needed for further validations and to use this field where applicable. -- v8: vkd3d-shader/tpf: Validate input and output index ranges for default control point phases. vkd3d-shader/tpf: Remove an unnecessary carriage return from a parser error message. vkd3d-shader/tpf: Validate index range declarations. vkd3d-shader/tpf: Validate input/output registers. vkd3d-shader/tpf: Validate signature element masks. vkd3d-shader/tpf: Validate signature element register indices. vkd3d-shader/tpf: Validate input/output register index counts. https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/198
From: Conor McCarthy <cmccarthy(a)codeweavers.com> --- libs/vkd3d-shader/d3dbc.c | 3 ++ libs/vkd3d-shader/ir.c | 41 +++++++----------- libs/vkd3d-shader/tpf.c | 55 ++++++++++++++++++++++++ libs/vkd3d-shader/vkd3d_shader_private.h | 2 + 4 files changed, 75 insertions(+), 26 deletions(-) diff --git a/libs/vkd3d-shader/d3dbc.c b/libs/vkd3d-shader/d3dbc.c index 823df6f6..d4f3ef66 100644 --- a/libs/vkd3d-shader/d3dbc.c +++ b/libs/vkd3d-shader/d3dbc.c @@ -465,6 +465,7 @@ static void shader_sm1_parse_src_param(uint32_t param, const struct vkd3d_shader src->reg.idx[1].rel_addr = NULL; src->reg.idx[2].offset = ~0u; src->reg.idx[2].rel_addr = NULL; + src->reg.idx_count = 1; src->swizzle = swizzle_from_sm1((param & VKD3D_SM1_SWIZZLE_MASK) >> VKD3D_SM1_SWIZZLE_SHIFT); src->modifiers = (param & VKD3D_SM1_SRC_MODIFIER_MASK) >> VKD3D_SM1_SRC_MODIFIER_SHIFT; } @@ -483,6 +484,7 @@ static void shader_sm1_parse_dst_param(uint32_t param, const struct vkd3d_shader dst->reg.idx[1].rel_addr = NULL; dst->reg.idx[2].offset = ~0u; dst->reg.idx[2].rel_addr = NULL; + dst->reg.idx_count = 1; dst->write_mask = (param & VKD3D_SM1_WRITEMASK_MASK) >> VKD3D_SM1_WRITEMASK_SHIFT; dst->modifiers = (param & VKD3D_SM1_DST_MODIFIER_MASK) >> VKD3D_SM1_DST_MODIFIER_SHIFT; dst->shift = (param & VKD3D_SM1_DSTSHIFT_MASK) >> VKD3D_SM1_DSTSHIFT_SHIFT; @@ -664,6 +666,7 @@ static void shader_sm1_read_immconst(struct vkd3d_shader_sm1_parser *sm1, const src_param->reg.idx[1].rel_addr = NULL; src_param->reg.idx[2].offset = ~0u; src_param->reg.idx[2].rel_addr = NULL; + src_param->reg.idx_count = 0; src_param->reg.immconst_type = type; memcpy(src_param->reg.u.immconst_uint, *ptr, count * sizeof(uint32_t)); src_param->swizzle = VKD3D_SHADER_NO_SWIZZLE; diff --git a/libs/vkd3d-shader/ir.c b/libs/vkd3d-shader/ir.c index a3ab03f2..e431dddd 100644 --- a/libs/vkd3d-shader/ir.c +++ b/libs/vkd3d-shader/ir.c @@ -43,7 +43,7 @@ static void shader_register_eliminate_phase_addressing(struct vkd3d_shader_regis { unsigned int i; - for (i = 0; i < ARRAY_SIZE(reg->idx) && reg->idx[i].offset != ~0u; ++i) + for (i = 0; i < reg->idx_count; ++i) { if (reg->idx[i].rel_addr && shader_register_is_phase_instance_id(®->idx[i].rel_addr->reg)) { @@ -73,6 +73,7 @@ static void shader_instruction_eliminate_phase_instance_id(struct vkd3d_shader_i reg->idx[1].rel_addr = NULL; reg->idx[2].offset = ~0u; reg->idx[2].rel_addr = NULL; + reg->idx_count = 0; reg->immconst_type = VKD3D_IMMCONST_SCALAR; reg->u.immconst_uint[0] = instance_id; continue; @@ -222,8 +223,8 @@ static enum vkd3d_result shader_normaliser_flatten_phases(struct vkd3d_shader_no return VKD3D_OK; } -static void shader_register_init(struct vkd3d_shader_register *reg, - enum vkd3d_shader_register_type reg_type, enum vkd3d_data_type data_type) +static void shader_register_init(struct vkd3d_shader_register *reg, enum vkd3d_shader_register_type reg_type, + enum vkd3d_data_type data_type, unsigned int idx_count) { reg->type = reg_type; reg->precision = VKD3D_SHADER_REGISTER_PRECISION_DEFAULT; @@ -235,6 +236,7 @@ static void shader_register_init(struct vkd3d_shader_register *reg, reg->idx[1].rel_addr = NULL; reg->idx[2].offset = ~0u; reg->idx[2].rel_addr = NULL; + reg->idx_count = idx_count; reg->immconst_type = VKD3D_IMMCONST_SCALAR; } @@ -286,46 +288,37 @@ static struct vkd3d_shader_src_param *shader_normaliser_create_outpointid_param( if (!(rel_addr = shader_src_param_allocator_get(&normaliser->instructions.src_params, 1))) return NULL; - shader_register_init(&rel_addr->reg, VKD3DSPR_OUTPOINTID, VKD3D_DATA_UINT); + shader_register_init(&rel_addr->reg, VKD3DSPR_OUTPOINTID, VKD3D_DATA_UINT, 0); rel_addr->swizzle = 0; rel_addr->modifiers = 0; return rel_addr; } -static bool shader_dst_param_normalise_outpointid(struct vkd3d_shader_dst_param *dst_param, +static void shader_dst_param_normalise_outpointid(struct vkd3d_shader_dst_param *dst_param, struct vkd3d_shader_normaliser *normaliser) { struct vkd3d_shader_register *reg = &dst_param->reg; if (normaliser_is_in_control_point_phase(normaliser) && reg->type == VKD3DSPR_OUTPUT) { - if (reg->idx[2].offset != ~0u) - { - FIXME("Cannot insert phase id.\n"); - return false; - } - if (reg->idx[1].offset != ~0u) - { - WARN("Unexpected address at index 1.\n"); - reg->idx[2] = reg->idx[1]; - } + /* The TPF reader validates idx_count. */ + assert(reg->idx_count == 1); reg->idx[1] = reg->idx[0]; /* The control point id param is implicit here. Avoid later complications by inserting it. */ reg->idx[0].offset = 0; reg->idx[0].rel_addr = normaliser->outpointid_param; + ++reg->idx_count; } - - return true; } -static void shader_dst_param_io_init(struct vkd3d_shader_dst_param *param, - const struct signature_element *e, enum vkd3d_shader_register_type reg_type) +static void shader_dst_param_io_init(struct vkd3d_shader_dst_param *param, const struct signature_element *e, + enum vkd3d_shader_register_type reg_type, unsigned int idx_count) { param->write_mask = e->mask; param->modifiers = 0; param->shift = 0; - shader_register_init(¶m->reg, reg_type, vkd3d_data_type_from_component_type(e->component_type)); + shader_register_init(¶m->reg, reg_type, vkd3d_data_type_from_component_type(e->component_type), idx_count); } static enum vkd3d_result shader_normaliser_emit_hs_input(struct vkd3d_shader_normaliser *normaliser, @@ -369,7 +362,7 @@ static enum vkd3d_result shader_normaliser_emit_hs_input(struct vkd3d_shader_nor param = &ins->declaration.dst; } - shader_dst_param_io_init(param, e, VKD3DSPR_INPUT); + shader_dst_param_io_init(param, e, VKD3DSPR_INPUT, 2); param->reg.idx[0].offset = input_control_point_count; param->reg.idx[1].offset = i; @@ -409,11 +402,7 @@ enum vkd3d_result shader_normaliser_normalise_hull_shader_control_point_io(struc if (shader_instruction_is_dcl(ins)) break; for (j = 0; j < ins->dst_count; ++j) - { - if (!shader_dst_param_normalise_outpointid((struct vkd3d_shader_dst_param *)&ins->dst[j], - normaliser)) - return VKD3D_ERROR_INVALID_ARGUMENT; - } + shader_dst_param_normalise_outpointid((struct vkd3d_shader_dst_param *)&ins->dst[j], normaliser); break; } } diff --git a/libs/vkd3d-shader/tpf.c b/libs/vkd3d-shader/tpf.c index fcd7cf93..b77942ff 100644 --- a/libs/vkd3d-shader/tpf.c +++ b/libs/vkd3d-shader/tpf.c @@ -564,6 +564,8 @@ struct vkd3d_shader_sm4_parser unsigned int output_map[MAX_REG_OUTPUT]; + enum vkd3d_shader_opcode phase; + struct vkd3d_shader_parser p; }; @@ -1669,6 +1671,8 @@ static bool shader_sm4_read_param(struct vkd3d_shader_sm4_parser *priv, const ui return false; } + param->idx_count = order; + if (register_type == VKD3D_SM4_RT_IMMCONST || register_type == VKD3D_SM4_RT_IMMCONST64) { enum vkd3d_sm4_dimension dimension = (token & VKD3D_SM4_DIMENSION_MASK) >> VKD3D_SM4_DIMENSION_SHIFT; @@ -1710,6 +1714,7 @@ static bool shader_sm4_read_param(struct vkd3d_shader_sm4_parser *priv, const ui * other values up one slot. Normalize to SM5.1. */ param->idx[2] = param->idx[1]; param->idx[1] = param->idx[0]; + ++param->idx_count; } map_register(priv, param); @@ -1742,6 +1747,47 @@ static uint32_t swizzle_from_sm4(uint32_t s) return vkd3d_shader_create_swizzle(s & 0x3, (s >> 2) & 0x3, (s >> 4) & 0x3, (s >> 6) & 0x3); } +static bool register_is_input_output(const struct vkd3d_shader_register *reg) +{ + switch (reg->type) + { + case VKD3DSPR_INPUT: + case VKD3DSPR_OUTPUT: + case VKD3DSPR_COLOROUT: + case VKD3DSPR_INCONTROLPOINT: + case VKD3DSPR_OUTCONTROLPOINT: + case VKD3DSPR_PATCHCONST: + return true; + + default: + return false; + } +} + +static bool register_is_control_point_input(const struct vkd3d_shader_register *reg, + const struct vkd3d_shader_sm4_parser *priv) +{ + return reg->type == VKD3DSPR_INCONTROLPOINT || reg->type == VKD3DSPR_OUTCONTROLPOINT + || (reg->type == VKD3DSPR_INPUT && (priv->phase == VKD3DSIH_HS_CONTROL_POINT_PHASE + || priv->p.shader_version.type == VKD3D_SHADER_TYPE_GEOMETRY)); +} + +static bool shader_sm4_validate_input_output_register(struct vkd3d_shader_sm4_parser *priv, + const struct vkd3d_shader_register *reg) +{ + unsigned int idx_count = 1 + register_is_control_point_input(reg, priv); + + if (reg->idx_count != idx_count) + { + vkd3d_shader_parser_error(&priv->p, VKD3D_SHADER_ERROR_TPF_INVALID_REGISTER_INDEX_COUNT, + "Invalid index count %u for register type %#x; expected count %u.", + reg->idx_count, reg->type, idx_count); + return false; + } + + return true; +} + static bool shader_sm4_read_src_param(struct vkd3d_shader_sm4_parser *priv, const uint32_t **ptr, const uint32_t *end, enum vkd3d_data_type data_type, struct vkd3d_shader_src_param *src_param) { @@ -1793,6 +1839,9 @@ static bool shader_sm4_read_src_param(struct vkd3d_shader_sm4_parser *priv, cons } } + if (register_is_input_output(&src_param->reg) && !shader_sm4_validate_input_output_register(priv, &src_param->reg)) + return false; + return true; } @@ -1830,6 +1879,9 @@ static bool shader_sm4_read_dst_param(struct vkd3d_shader_sm4_parser *priv, cons dst_param->modifiers = 0; dst_param->shift = 0; + if (register_is_input_output(&dst_param->reg) && !shader_sm4_validate_input_output_register(priv, &dst_param->reg)) + return false; + return true; } @@ -1967,6 +2019,9 @@ static void shader_sm4_read_instruction(struct vkd3d_shader_sm4_parser *sm4, str } ins->handler_idx = 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; ins->flags = 0; ins->coissue = false; ins->raw = false; diff --git a/libs/vkd3d-shader/vkd3d_shader_private.h b/libs/vkd3d-shader/vkd3d_shader_private.h index f1641c1e..2546c0d2 100644 --- a/libs/vkd3d-shader/vkd3d_shader_private.h +++ b/libs/vkd3d-shader/vkd3d_shader_private.h @@ -74,6 +74,7 @@ enum vkd3d_shader_error VKD3D_SHADER_ERROR_TPF_MISMATCHED_CF = 1000, VKD3D_SHADER_ERROR_TPF_INVALID_REGISTER_RANGE = 1001, VKD3D_SHADER_ERROR_TPF_OUT_OF_MEMORY = 1002, + VKD3D_SHADER_ERROR_TPF_INVALID_REGISTER_INDEX_COUNT = 1003, VKD3D_SHADER_ERROR_SPV_DESCRIPTOR_BINDING_NOT_FOUND = 2000, VKD3D_SHADER_ERROR_SPV_INVALID_REGISTER_TYPE = 2001, @@ -676,6 +677,7 @@ struct vkd3d_shader_register bool non_uniform; enum vkd3d_data_type data_type; struct vkd3d_shader_register_index idx[3]; + unsigned int idx_count; enum vkd3d_immconst_type immconst_type; union { -- GitLab https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/198
From: Conor McCarthy <cmccarthy(a)codeweavers.com> --- libs/vkd3d-shader/tpf.c | 31 ++++++++++++++++++++++++ libs/vkd3d-shader/vkd3d_shader_private.h | 1 + 2 files changed, 32 insertions(+) diff --git a/libs/vkd3d-shader/tpf.c b/libs/vkd3d-shader/tpf.c index b77942ff..3dd2b446 100644 --- a/libs/vkd3d-shader/tpf.c +++ b/libs/vkd3d-shader/tpf.c @@ -2200,6 +2200,29 @@ static bool shader_sm4_init(struct vkd3d_shader_sm4_parser *sm4, const uint32_t return true; } +static bool shader_sm4_parser_validate_signature(struct vkd3d_shader_sm4_parser *sm4, + const struct shader_signature *signature, const char *name) +{ + unsigned int i, register_idx, register_count; + + for (i = 0; i < signature->element_count; ++i) + { + register_idx = signature->elements[i].register_index; + register_count = signature->elements[i].register_count; + if (register_idx != ~0u && (register_idx >= MAX_REG_OUTPUT || MAX_REG_OUTPUT - register_idx < register_count)) + { + WARN("%s signature element %u unhandled register index %u, count %u.\n", + name, i, register_idx, register_count); + vkd3d_shader_parser_error(&sm4->p, VKD3D_SHADER_ERROR_TPF_TOO_MANY_REGISTERS, + "%s signature element %u register index %u, count %u exceeds maximum index of %u.", name, + i, register_idx, register_count, MAX_REG_OUTPUT - 1); + return false; + } + } + + return true; +} + int vkd3d_shader_sm4_parser_create(const struct vkd3d_shader_compile_info *compile_info, struct vkd3d_shader_message_context *message_context, struct vkd3d_shader_parser **parser) { @@ -2233,6 +2256,14 @@ int vkd3d_shader_sm4_parser_create(const struct vkd3d_shader_compile_info *compi return VKD3D_ERROR_INVALID_ARGUMENT; } + if (!shader_sm4_parser_validate_signature(sm4, &shader_desc->input_signature, "Input") + || !shader_sm4_parser_validate_signature(sm4, &shader_desc->output_signature, "Output") + || !shader_sm4_parser_validate_signature(sm4, &shader_desc->patch_constant_signature, "Patch constant")) + { + shader_sm4_destroy(&sm4->p); + return VKD3D_ERROR_INVALID_SHADER; + } + instructions = &sm4->p.instructions; while (sm4->ptr != sm4->end) { diff --git a/libs/vkd3d-shader/vkd3d_shader_private.h b/libs/vkd3d-shader/vkd3d_shader_private.h index 2546c0d2..3b421813 100644 --- a/libs/vkd3d-shader/vkd3d_shader_private.h +++ b/libs/vkd3d-shader/vkd3d_shader_private.h @@ -75,6 +75,7 @@ enum vkd3d_shader_error VKD3D_SHADER_ERROR_TPF_INVALID_REGISTER_RANGE = 1001, VKD3D_SHADER_ERROR_TPF_OUT_OF_MEMORY = 1002, VKD3D_SHADER_ERROR_TPF_INVALID_REGISTER_INDEX_COUNT = 1003, + VKD3D_SHADER_ERROR_TPF_TOO_MANY_REGISTERS = 1004, VKD3D_SHADER_ERROR_SPV_DESCRIPTOR_BINDING_NOT_FOUND = 2000, VKD3D_SHADER_ERROR_SPV_INVALID_REGISTER_TYPE = 2001, -- GitLab https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/198
From: Conor McCarthy <cmccarthy(a)codeweavers.com> --- libs/vkd3d-shader/tpf.c | 9 ++++++++- libs/vkd3d-shader/vkd3d_shader_private.h | 2 ++ 2 files changed, 10 insertions(+), 1 deletion(-) diff --git a/libs/vkd3d-shader/tpf.c b/libs/vkd3d-shader/tpf.c index 3dd2b446..0daadb9c 100644 --- a/libs/vkd3d-shader/tpf.c +++ b/libs/vkd3d-shader/tpf.c @@ -2203,7 +2203,7 @@ static bool shader_sm4_init(struct vkd3d_shader_sm4_parser *sm4, const uint32_t static bool shader_sm4_parser_validate_signature(struct vkd3d_shader_sm4_parser *sm4, const struct shader_signature *signature, const char *name) { - unsigned int i, register_idx, register_count; + unsigned int i, register_idx, register_count, mask; for (i = 0; i < signature->element_count; ++i) { @@ -2218,6 +2218,13 @@ static bool shader_sm4_parser_validate_signature(struct vkd3d_shader_sm4_parser i, register_idx, register_count, MAX_REG_OUTPUT - 1); return false; } + + if (!vkd3d_bitmask_is_contiguous(mask = signature->elements[i].mask)) + { + WARN("%s signature element %u mask %#x is not contiguous.\n", name, i, mask); + vkd3d_shader_parser_warning(&sm4->p, VKD3D_SHADER_WARNING_TPF_MASK_NOT_CONTIGUOUS, + "%s signature element %u mask %#x is not contiguous.", name, i, mask); + } } return true; diff --git a/libs/vkd3d-shader/vkd3d_shader_private.h b/libs/vkd3d-shader/vkd3d_shader_private.h index 3b421813..d2ea97e8 100644 --- a/libs/vkd3d-shader/vkd3d_shader_private.h +++ b/libs/vkd3d-shader/vkd3d_shader_private.h @@ -77,6 +77,8 @@ enum vkd3d_shader_error VKD3D_SHADER_ERROR_TPF_INVALID_REGISTER_INDEX_COUNT = 1003, VKD3D_SHADER_ERROR_TPF_TOO_MANY_REGISTERS = 1004, + VKD3D_SHADER_WARNING_TPF_MASK_NOT_CONTIGUOUS = 1300, + VKD3D_SHADER_ERROR_SPV_DESCRIPTOR_BINDING_NOT_FOUND = 2000, VKD3D_SHADER_ERROR_SPV_INVALID_REGISTER_TYPE = 2001, VKD3D_SHADER_ERROR_SPV_INVALID_DESCRIPTOR_BINDING = 2002, -- GitLab https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/198
From: Conor McCarthy <cmccarthy(a)codeweavers.com> --- libs/vkd3d-shader/tpf.c | 72 +++++++++++++++++++++--- libs/vkd3d-shader/vkd3d_shader_private.h | 1 + 2 files changed, 65 insertions(+), 8 deletions(-) diff --git a/libs/vkd3d-shader/tpf.c b/libs/vkd3d-shader/tpf.c index 0daadb9c..8b796dc1 100644 --- a/libs/vkd3d-shader/tpf.c +++ b/libs/vkd3d-shader/tpf.c @@ -565,6 +565,9 @@ struct vkd3d_shader_sm4_parser unsigned int output_map[MAX_REG_OUTPUT]; enum vkd3d_shader_opcode phase; + unsigned int input_register_masks[MAX_REG_OUTPUT]; + unsigned int output_register_masks[MAX_REG_OUTPUT]; + unsigned int patch_constant_register_masks[MAX_REG_OUTPUT]; struct vkd3d_shader_parser p; }; @@ -1427,7 +1430,7 @@ static void map_register(const struct vkd3d_shader_sm4_parser *sm4, struct vkd3d if (reg_idx >= ARRAY_SIZE(sm4->output_map)) { - ERR("Invalid output index %u.\n", reg_idx); + /* Validated later */ break; } @@ -1772,10 +1775,20 @@ static bool register_is_control_point_input(const struct vkd3d_shader_register * || priv->p.shader_version.type == VKD3D_SHADER_TYPE_GEOMETRY)); } +static unsigned int mask_from_swizzle(unsigned int swizzle) +{ + return (1u << vkd3d_swizzle_get_component(swizzle, 0)) + | (1u << vkd3d_swizzle_get_component(swizzle, 1)) + | (1u << vkd3d_swizzle_get_component(swizzle, 2)) + | (1u << vkd3d_swizzle_get_component(swizzle, 3)); +} + static bool shader_sm4_validate_input_output_register(struct vkd3d_shader_sm4_parser *priv, - const struct vkd3d_shader_register *reg) + const struct vkd3d_shader_register *reg, unsigned int mask) { unsigned int idx_count = 1 + register_is_control_point_input(reg, priv); + const unsigned int *masks; + unsigned int register_idx; if (reg->idx_count != idx_count) { @@ -1785,6 +1798,41 @@ static bool shader_sm4_validate_input_output_register(struct vkd3d_shader_sm4_pa return false; } + switch (reg->type) + { + case VKD3DSPR_INPUT: + case VKD3DSPR_INCONTROLPOINT: + masks = priv->input_register_masks; + break; + case VKD3DSPR_OUTPUT: + masks = (priv->phase == VKD3DSIH_HS_FORK_PHASE || priv->phase == VKD3DSIH_HS_JOIN_PHASE) + ? priv->patch_constant_register_masks + : priv->output_register_masks; + break; + case VKD3DSPR_COLOROUT: + case VKD3DSPR_OUTCONTROLPOINT: + masks = priv->output_register_masks; + break; + case VKD3DSPR_PATCHCONST: + masks = priv->patch_constant_register_masks; + break; + + default: + vkd3d_unreachable(); + } + + register_idx = reg->idx[reg->idx_count - 1].offset; + /* The signature element registers have already been checked against MAX_REG_OUTPUT. */ + if (register_idx >= MAX_REG_OUTPUT || (masks[register_idx] & mask) != mask) + { + WARN("Failed to find signature element for register type %#x, index %u and mask %#x.\n", + reg->type, register_idx, mask); + vkd3d_shader_parser_error(&priv->p, VKD3D_SHADER_ERROR_TPF_INVALID_IO_REGISTER, + "Could not find signature element matching register type %#x, index %u and mask %#x.", + reg->type, register_idx, mask); + return false; + } + return true; } @@ -1839,7 +1887,8 @@ static bool shader_sm4_read_src_param(struct vkd3d_shader_sm4_parser *priv, cons } } - if (register_is_input_output(&src_param->reg) && !shader_sm4_validate_input_output_register(priv, &src_param->reg)) + if (register_is_input_output(&src_param->reg) && !shader_sm4_validate_input_output_register(priv, + &src_param->reg, mask_from_swizzle(src_param->swizzle))) return false; return true; @@ -1879,7 +1928,8 @@ static bool shader_sm4_read_dst_param(struct vkd3d_shader_sm4_parser *priv, cons dst_param->modifiers = 0; dst_param->shift = 0; - if (register_is_input_output(&dst_param->reg) && !shader_sm4_validate_input_output_register(priv, &dst_param->reg)) + if (register_is_input_output(&dst_param->reg) && !shader_sm4_validate_input_output_register(priv, + &dst_param->reg, dst_param->write_mask)) return false; return true; @@ -2201,7 +2251,7 @@ static bool shader_sm4_init(struct vkd3d_shader_sm4_parser *sm4, const uint32_t } static bool shader_sm4_parser_validate_signature(struct vkd3d_shader_sm4_parser *sm4, - const struct shader_signature *signature, const char *name) + const struct shader_signature *signature, unsigned int *masks, const char *name) { unsigned int i, register_idx, register_count, mask; @@ -2225,6 +2275,9 @@ static bool shader_sm4_parser_validate_signature(struct vkd3d_shader_sm4_parser vkd3d_shader_parser_warning(&sm4->p, VKD3D_SHADER_WARNING_TPF_MASK_NOT_CONTIGUOUS, "%s signature element %u mask %#x is not contiguous.", name, i, mask); } + + if (register_idx != ~0u) + masks[register_idx] |= mask; } return true; @@ -2263,9 +2316,12 @@ int vkd3d_shader_sm4_parser_create(const struct vkd3d_shader_compile_info *compi return VKD3D_ERROR_INVALID_ARGUMENT; } - if (!shader_sm4_parser_validate_signature(sm4, &shader_desc->input_signature, "Input") - || !shader_sm4_parser_validate_signature(sm4, &shader_desc->output_signature, "Output") - || !shader_sm4_parser_validate_signature(sm4, &shader_desc->patch_constant_signature, "Patch constant")) + if (!shader_sm4_parser_validate_signature(sm4, &shader_desc->input_signature, + sm4->input_register_masks, "Input") + || !shader_sm4_parser_validate_signature(sm4, &shader_desc->output_signature, + sm4->output_register_masks, "Output") + || !shader_sm4_parser_validate_signature(sm4, &shader_desc->patch_constant_signature, + sm4->patch_constant_register_masks, "Patch constant")) { shader_sm4_destroy(&sm4->p); return VKD3D_ERROR_INVALID_SHADER; diff --git a/libs/vkd3d-shader/vkd3d_shader_private.h b/libs/vkd3d-shader/vkd3d_shader_private.h index d2ea97e8..55c25f64 100644 --- a/libs/vkd3d-shader/vkd3d_shader_private.h +++ b/libs/vkd3d-shader/vkd3d_shader_private.h @@ -76,6 +76,7 @@ enum vkd3d_shader_error VKD3D_SHADER_ERROR_TPF_OUT_OF_MEMORY = 1002, VKD3D_SHADER_ERROR_TPF_INVALID_REGISTER_INDEX_COUNT = 1003, VKD3D_SHADER_ERROR_TPF_TOO_MANY_REGISTERS = 1004, + VKD3D_SHADER_ERROR_TPF_INVALID_IO_REGISTER = 1005, VKD3D_SHADER_WARNING_TPF_MASK_NOT_CONTIGUOUS = 1300, -- GitLab https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/198
From: Conor McCarthy <cmccarthy(a)codeweavers.com> --- libs/vkd3d-shader/spirv.c | 23 ----- libs/vkd3d-shader/tpf.c | 121 ++++++++++++++++++++++- libs/vkd3d-shader/vkd3d_shader_private.h | 2 + 3 files changed, 119 insertions(+), 27 deletions(-) diff --git a/libs/vkd3d-shader/spirv.c b/libs/vkd3d-shader/spirv.c index e13ce0a2..fa4d056e 100644 --- a/libs/vkd3d-shader/spirv.c +++ b/libs/vkd3d-shader/spirv.c @@ -6195,8 +6195,6 @@ static bool spirv_compiler_check_index_range(struct spirv_compiler *compiler, struct vkd3d_shader_register_info reg_info; struct vkd3d_shader_register current_reg; struct vkd3d_symbol reg_symbol; - unsigned int i; - uint32_t id; current_reg = *reg; vkd3d_symbol_make_register(®_symbol, ¤t_reg); @@ -6212,27 +6210,6 @@ static bool spirv_compiler_check_index_range(struct spirv_compiler *compiler, FIXME("Unhandled register %#x.\n", reg->type); return false; } - id = reg_info.id; - - for (i = reg->idx[0].offset; i < reg->idx[0].offset + range->register_count; ++i) - { - current_reg.idx[0].offset = i; - vkd3d_symbol_make_register(®_symbol, ¤t_reg); - - if (range->dst.write_mask != reg_info.write_mask - || vkd3d_write_mask_component_count(reg_info.write_mask) != 1) - { - FIXME("Unhandled index range write mask %#x (%#x).\n", - range->dst.write_mask, reg_info.write_mask); - return false; - } - - if (reg_info.id != id) - { - FIXME("Unhandled index range %#x, %u.\n", reg->type, i); - return false; - } - } return true; } diff --git a/libs/vkd3d-shader/tpf.c b/libs/vkd3d-shader/tpf.c index 8b796dc1..4e26f247 100644 --- a/libs/vkd3d-shader/tpf.c +++ b/libs/vkd3d-shader/tpf.c @@ -558,6 +558,19 @@ enum vkd3d_sm4_shader_data_type VKD3D_SM4_SHADER_DATA_MESSAGE = 0x4, }; +struct sm4_index_range +{ + unsigned int index; + unsigned int count; + unsigned int mask; +}; + +struct sm4_index_range_array +{ + unsigned int count; + struct sm4_index_range ranges[MAX_REG_OUTPUT * 2]; +}; + struct vkd3d_shader_sm4_parser { const uint32_t *start, *end, *ptr; @@ -569,6 +582,10 @@ struct vkd3d_shader_sm4_parser unsigned int output_register_masks[MAX_REG_OUTPUT]; unsigned int patch_constant_register_masks[MAX_REG_OUTPUT]; + struct sm4_index_range_array input_index_ranges; + struct sm4_index_range_array output_index_ranges; + struct sm4_index_range_array patch_constant_index_ranges; + struct vkd3d_shader_parser p; }; @@ -823,12 +840,109 @@ static void shader_sm4_read_dcl_sampler(struct vkd3d_shader_instruction *ins, ui shader_sm4_read_register_space(priv, &tokens, end, &ins->declaration.sampler.range.space); } +static bool sm4_parser_is_in_fork_or_join_phase(const struct vkd3d_shader_sm4_parser *sm4) +{ + return sm4->phase == VKD3DSIH_HS_FORK_PHASE || sm4->phase == VKD3DSIH_HS_JOIN_PHASE; +} + static void shader_sm4_read_dcl_index_range(struct vkd3d_shader_instruction *ins, uint32_t opcode, uint32_t opcode_token, const uint32_t *tokens, unsigned int token_count, struct vkd3d_shader_sm4_parser *priv) { + struct vkd3d_shader_index_range *index_range = &ins->declaration.index_range; + unsigned int i, register_idx, register_count, write_mask; + enum vkd3d_shader_register_type type; + struct sm4_index_range_array *ranges; + unsigned int *io_masks; + shader_sm4_read_dst_param(priv, &tokens, &tokens[token_count], VKD3D_DATA_OPAQUE, - &ins->declaration.index_range.dst); - ins->declaration.index_range.register_count = *tokens; + &index_range->dst); + index_range->register_count = *tokens; + + register_idx = index_range->dst.reg.idx[index_range->dst.reg.idx_count - 1].offset; + register_count = index_range->register_count; + write_mask = index_range->dst.write_mask; + + if (vkd3d_write_mask_component_count(write_mask) != 1) + { + WARN("Unhandled write mask %#x.\n", write_mask); + vkd3d_shader_parser_warning(&priv->p, VKD3D_SHADER_WARNING_TPF_UNHANDLED_INDEX_RANGE_MASK, + "Index range mask %#x is not scalar.", write_mask); + } + + switch ((type = index_range->dst.reg.type)) + { + case VKD3DSPR_INPUT: + case VKD3DSPR_INCONTROLPOINT: + io_masks = priv->input_register_masks; + ranges = &priv->input_index_ranges; + break; + case VKD3DSPR_OUTPUT: + if (sm4_parser_is_in_fork_or_join_phase(priv)) + { + io_masks = priv->patch_constant_register_masks; + ranges = &priv->patch_constant_index_ranges; + } + else + { + io_masks = priv->output_register_masks; + ranges = &priv->output_index_ranges; + } + break; + case VKD3DSPR_COLOROUT: + case VKD3DSPR_OUTCONTROLPOINT: + io_masks = priv->output_register_masks; + ranges = &priv->output_index_ranges; + break; + case VKD3DSPR_PATCHCONST: + io_masks = priv->patch_constant_register_masks; + ranges = &priv->patch_constant_index_ranges; + break; + + default: + WARN("Unhandled register type %#x.\n", type); + vkd3d_shader_parser_error(&priv->p, VKD3D_SHADER_ERROR_TPF_INVALID_INDEX_RANGE_DCL, + "Invalid register type %#x for index range base %u, count %u, mask %#x.", + type, register_idx, register_count, write_mask); + return; + } + + for (i = 0; i < ranges->count; ++i) + { + struct sm4_index_range r = ranges->ranges[i]; + + if (!(r.mask & write_mask)) + continue; + /* Ranges with the same base but different lengths are not an issue. */ + if (register_idx == r.index) + continue; + + if ((r.index <= register_idx && register_idx - r.index < r.count) + || (register_idx < r.index && r.index - register_idx < register_count)) + { + WARN("Detected index range collision for base %u, count %u, mask %#x.\n", + register_idx, register_count, write_mask); + vkd3d_shader_parser_error(&priv->p, VKD3D_SHADER_ERROR_TPF_INVALID_INDEX_RANGE_DCL, + "Register index range base %u, count %u, mask %#x collides with a previous declaration.", + register_idx, register_count, write_mask); + return; + } + } + ranges->ranges[ranges->count].index = register_idx; + ranges->ranges[ranges->count].count = register_count; + ranges->ranges[ranges->count++].mask = write_mask; + + for (i = 0; i < register_count; ++i) + { + if ((io_masks[register_idx + i] & write_mask) != write_mask) + { + WARN("No matching declaration for index range base %u, count %u, mask %#x.\n", + register_idx, register_count, write_mask); + vkd3d_shader_parser_error(&priv->p, VKD3D_SHADER_ERROR_TPF_INVALID_INDEX_RANGE_DCL, + "Input/output registers matching index range base %u, count %u, mask %#x were not declared.", + register_idx, register_count, write_mask); + return; + } + } } static void shader_sm4_read_dcl_output_topology(struct vkd3d_shader_instruction *ins, uint32_t opcode, @@ -1805,8 +1919,7 @@ static bool shader_sm4_validate_input_output_register(struct vkd3d_shader_sm4_pa masks = priv->input_register_masks; break; case VKD3DSPR_OUTPUT: - masks = (priv->phase == VKD3DSIH_HS_FORK_PHASE || priv->phase == VKD3DSIH_HS_JOIN_PHASE) - ? priv->patch_constant_register_masks + masks = sm4_parser_is_in_fork_or_join_phase(priv) ? priv->patch_constant_register_masks : priv->output_register_masks; break; case VKD3DSPR_COLOROUT: diff --git a/libs/vkd3d-shader/vkd3d_shader_private.h b/libs/vkd3d-shader/vkd3d_shader_private.h index 55c25f64..f085deb8 100644 --- a/libs/vkd3d-shader/vkd3d_shader_private.h +++ b/libs/vkd3d-shader/vkd3d_shader_private.h @@ -77,8 +77,10 @@ enum vkd3d_shader_error VKD3D_SHADER_ERROR_TPF_INVALID_REGISTER_INDEX_COUNT = 1003, VKD3D_SHADER_ERROR_TPF_TOO_MANY_REGISTERS = 1004, VKD3D_SHADER_ERROR_TPF_INVALID_IO_REGISTER = 1005, + VKD3D_SHADER_ERROR_TPF_INVALID_INDEX_RANGE_DCL = 1006, VKD3D_SHADER_WARNING_TPF_MASK_NOT_CONTIGUOUS = 1300, + VKD3D_SHADER_WARNING_TPF_UNHANDLED_INDEX_RANGE_MASK = 1301, VKD3D_SHADER_ERROR_SPV_DESCRIPTOR_BINDING_NOT_FOUND = 2000, VKD3D_SHADER_ERROR_SPV_INVALID_REGISTER_TYPE = 2001, -- GitLab https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/198
From: Conor McCarthy <cmccarthy(a)codeweavers.com> --- libs/vkd3d-shader/tpf.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/libs/vkd3d-shader/tpf.c b/libs/vkd3d-shader/tpf.c index 4e26f247..fb309ea9 100644 --- a/libs/vkd3d-shader/tpf.c +++ b/libs/vkd3d-shader/tpf.c @@ -742,7 +742,7 @@ static void shader_sm4_set_descriptor_register_range(struct vkd3d_shader_sm4_par { FIXME("Invalid register range [%u:%u].\n", range->first, range->last); vkd3d_shader_parser_error(&sm4->p, VKD3D_SHADER_ERROR_TPF_INVALID_REGISTER_RANGE, - "Last register %u must not be less than first register %u in range.\n", range->last, range->first); + "Last register %u must not be less than first register %u in range.", range->last, range->first); } } -- GitLab https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/198
From: Conor McCarthy <cmccarthy(a)codeweavers.com> --- libs/vkd3d-shader/tpf.c | 32 ++++++++++++++++++++++++++++++++ 1 file changed, 32 insertions(+) diff --git a/libs/vkd3d-shader/tpf.c b/libs/vkd3d-shader/tpf.c index fb309ea9..4e0c1798 100644 --- a/libs/vkd3d-shader/tpf.c +++ b/libs/vkd3d-shader/tpf.c @@ -578,6 +578,7 @@ struct vkd3d_shader_sm4_parser unsigned int output_map[MAX_REG_OUTPUT]; enum vkd3d_shader_opcode phase; + bool has_control_point_phase; unsigned int input_register_masks[MAX_REG_OUTPUT]; unsigned int output_register_masks[MAX_REG_OUTPUT]; unsigned int patch_constant_register_masks[MAX_REG_OUTPUT]; @@ -2185,6 +2186,7 @@ static void shader_sm4_read_instruction(struct vkd3d_shader_sm4_parser *sm4, str 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; + sm4->has_control_point_phase |= ins->handler_idx == VKD3DSIH_HS_CONTROL_POINT_PHASE; ins->flags = 0; ins->coissue = false; ins->raw = false; @@ -2396,6 +2398,34 @@ static bool shader_sm4_parser_validate_signature(struct vkd3d_shader_sm4_parser return true; } +static int index_range_compare(const void *a, const void *b) +{ + return memcmp(a, b, sizeof(struct sm4_index_range)); +} + +static void shader_sm4_validate_default_phase_index_ranges(struct vkd3d_shader_sm4_parser *sm4) +{ + if (!sm4->input_index_ranges.count || !sm4->output_index_ranges.count) + return; + + if (sm4->input_index_ranges.count == sm4->output_index_ranges.count) + { + qsort(sm4->input_index_ranges.ranges, sm4->input_index_ranges.count, sizeof(sm4->input_index_ranges.ranges[0]), + index_range_compare); + qsort(sm4->output_index_ranges.ranges, sm4->output_index_ranges.count, sizeof(sm4->output_index_ranges.ranges[0]), + index_range_compare); + if (!memcmp(sm4->input_index_ranges.ranges, sm4->output_index_ranges.ranges, + sm4->input_index_ranges.count * sizeof(sm4->input_index_ranges.ranges[0]))) + return; + } + + /* This is very unlikely to occur and would complicate the default control point phase implementation. */ + WARN("Default phase index ranges are not identical.\n"); + vkd3d_shader_parser_error(&sm4->p, VKD3D_SHADER_ERROR_TPF_INVALID_INDEX_RANGE_DCL, + "Default control point phase input and output index range declarations are not identical."); + return; +} + int vkd3d_shader_sm4_parser_create(const struct vkd3d_shader_compile_info *compile_info, struct vkd3d_shader_message_context *message_context, struct vkd3d_shader_parser **parser) { @@ -2461,6 +2491,8 @@ int vkd3d_shader_sm4_parser_create(const struct vkd3d_shader_compile_info *compi } ++instructions->count; } + 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); *parser = &sm4->p; -- GitLab https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/198
This merge request was approved by Henri Verbeet. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/198
Giovanni Mascellani (@giomasce) commented about libs/vkd3d-shader/tpf.c:
* other values up one slot. Normalize to SM5.1. */ param->idx[2] = param->idx[1]; param->idx[1] = param->idx[0]; + ++param->idx_count;
I don't really know what I'm talking about here, but after reading that my first though is: "Shouldn't we check that `idx_count` is not already three? And shouldn't we zero `idx[0]`?". -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/198#note_32272
Giovanni Mascellani (@giomasce) commented about libs/vkd3d-shader/tpf.c:
+ return memcmp(a, b, sizeof(struct sm4_index_range)); +} + +static void shader_sm4_validate_default_phase_index_ranges(struct vkd3d_shader_sm4_parser *sm4) +{ + if (!sm4->input_index_ranges.count || !sm4->output_index_ranges.count) + return; + + if (sm4->input_index_ranges.count == sm4->output_index_ranges.count) + { + qsort(sm4->input_index_ranges.ranges, sm4->input_index_ranges.count, sizeof(sm4->input_index_ranges.ranges[0]), + index_range_compare); + qsort(sm4->output_index_ranges.ranges, sm4->output_index_ranges.count, sizeof(sm4->output_index_ranges.ranges[0]), + index_range_compare); + if (!memcmp(sm4->input_index_ranges.ranges, sm4->output_index_ranges.ranges, + sm4->input_index_ranges.count * sizeof(sm4->input_index_ranges.ranges[0]))) I think that's correct, but quite fragile: as soon as, for any reason, `sm4_index_range_array` is modified in a way that causes it to have some padding, the compare might break.
No need to change for me, let's just keep this in mind. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/198#note_32273
I don't really know what I'm talking about here, but after reading that my first though is: "Shouldn't we check that `idx_count` is not already three? And shouldn't we zero `idx[0]`?".
We shouldn't zero idx[0] here; duplicating it is the intended behaviour. It's true that this code could be a bit more robust, although that's not an issue introduced by this series, and probably best addressed by extending the index count validation code to cover the affected register types.
I think that's correct, but quite fragile: as soon as, for any reason, `sm4_index_range_array` is modified in a way that causes it to have some padding, the compare might break.
Yeah. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/198#note_32275
participants (4)
-
Conor McCarthy -
Conor McCarthy (＠cmccarthy)
-
Giovanni Mascellani (＠giomasce)
-
Henri Verbeet (＠hverbeet)