-- v2: vkd3d-shader/ir: Check that SSA registers are used validly. vkd3d-shader/ir: Check that SSA registers have consistent dimensions. vkd3d-shader/ir: Check that TEMP registers have consistent dimensions.
From: Giovanni Mascellani gmascellani@codeweavers.com
--- libs/vkd3d-shader/ir.c | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-)
diff --git a/libs/vkd3d-shader/ir.c b/libs/vkd3d-shader/ir.c index bac426919..887782f5a 100644 --- a/libs/vkd3d-shader/ir.c +++ b/libs/vkd3d-shader/ir.c @@ -1593,26 +1593,32 @@ static void vsir_validate_register(struct validation_context *ctx, { case VKD3DSPR_TEMP: if (reg->idx_count != 1) + { validator_error(ctx, VKD3D_SHADER_ERROR_VSIR_INVALID_INDEX_COUNT, "Invalid index count %u for a TEMP register.", reg->idx_count); + break; + }
- if (reg->idx_count >= 1 && reg->idx[0].rel_addr) + if (reg->idx[0].rel_addr) validator_error(ctx, VKD3D_SHADER_ERROR_VSIR_INVALID_INDEX, "Non-NULL relative address for a TEMP register.");
- if (reg->idx_count >= 1 && reg->idx[0].offset >= temp_count) + if (reg->idx[0].offset >= temp_count) validator_error(ctx, VKD3D_SHADER_ERROR_VSIR_INVALID_INDEX, "TEMP register index %u exceeds the maximum count %u.", reg->idx[0].offset, temp_count); break;
case VKD3DSPR_SSA: if (reg->idx_count != 1) + { validator_error(ctx, VKD3D_SHADER_ERROR_VSIR_INVALID_INDEX_COUNT, "Invalid index count %u for a SSA register.", reg->idx_count); + break; + }
- if (reg->idx_count >= 1 && reg->idx[0].rel_addr) + if (reg->idx[0].rel_addr) validator_error(ctx, VKD3D_SHADER_ERROR_VSIR_INVALID_INDEX, "Non-NULL relative address for a SSA register.");
- if (reg->idx_count >= 1 && reg->idx[0].offset >= ctx->parser->shader_desc.ssa_count) + if (reg->idx[0].offset >= ctx->parser->shader_desc.ssa_count) validator_error(ctx, VKD3D_SHADER_ERROR_VSIR_INVALID_INDEX, "SSA register index %u exceeds the maximum count %u.", reg->idx[0].offset, ctx->parser->shader_desc.ssa_count); break;
From: Giovanni Mascellani gmascellani@codeweavers.com
--- libs/vkd3d-shader/ir.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/libs/vkd3d-shader/ir.c b/libs/vkd3d-shader/ir.c index 887782f5a..d03e48427 100644 --- a/libs/vkd3d-shader/ir.c +++ b/libs/vkd3d-shader/ir.c @@ -1900,5 +1900,5 @@ void vsir_validate(struct vkd3d_shader_parser *parser) if (ctx.depth != 0) validator_error(&ctx, VKD3D_SHADER_ERROR_VSIR_INVALID_INSTRUCTION_NESTING, "%zu nested blocks were not closed.", ctx.depth);
- free(ctx.blocks); + vkd3d_free(ctx.blocks); }
From: Giovanni Mascellani gmascellani@codeweavers.com
--- libs/vkd3d-shader/d3dbc.c | 11 +++-- libs/vkd3d-shader/dxil.c | 2 +- libs/vkd3d-shader/ir.c | 59 ++++++++++++++++++++++-- libs/vkd3d-shader/vkd3d_shader_private.h | 2 +- 4 files changed, 65 insertions(+), 9 deletions(-)
diff --git a/libs/vkd3d-shader/d3dbc.c b/libs/vkd3d-shader/d3dbc.c index 8fec1e637..aa0dd8f4b 100644 --- a/libs/vkd3d-shader/d3dbc.c +++ b/libs/vkd3d-shader/d3dbc.c @@ -1360,18 +1360,21 @@ int vkd3d_shader_sm1_parser_create(const struct vkd3d_shader_compile_info *compi sm1->p.shader_desc.flat_constant_count[i].external = get_external_constant_count(sm1, i);
if (!sm1->p.failed) - vsir_validate(&sm1->p); + ret = vsir_validate(&sm1->p);
- if (sm1->p.failed) + if (sm1->p.failed && ret >= 0) + ret = VKD3D_ERROR_INVALID_SHADER; + + if (ret < 0) { WARN("Failed to parse shader.\n"); shader_sm1_destroy(&sm1->p); - return VKD3D_ERROR_INVALID_SHADER; + return ret; }
*parser = &sm1->p;
- return VKD3D_OK; + return ret; }
bool hlsl_sm1_register_from_semantic(struct hlsl_ctx *ctx, const struct hlsl_semantic *semantic, diff --git a/libs/vkd3d-shader/dxil.c b/libs/vkd3d-shader/dxil.c index c744dfced..1977b96c4 100644 --- a/libs/vkd3d-shader/dxil.c +++ b/libs/vkd3d-shader/dxil.c @@ -6282,7 +6282,7 @@ int vkd3d_shader_sm6_parser_create(const struct vkd3d_shader_compile_info *compi vkd3d_free(byte_code);
if (!sm6->p.failed && ret >= 0) - vsir_validate(&sm6->p); + ret = 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 d03e48427..b0fd2c1eb 100644 --- a/libs/vkd3d-shader/ir.c +++ b/libs/vkd3d-shader/ir.c @@ -1511,7 +1511,7 @@ enum vkd3d_result vkd3d_shader_normalise(struct vkd3d_shader_parser *parser, vkd3d_shader_trace(instructions, &parser->shader_version);
if (result >= 0 && !parser->failed) - vsir_validate(parser); + result = vsir_validate(parser);
if (result >= 0 && parser->failed) result = VKD3D_ERROR_INVALID_SHADER; @@ -1527,6 +1527,12 @@ struct validation_context unsigned int temp_count; enum vkd3d_shader_opcode phase;
+ struct validation_context_temp_data + { + enum vsir_dimension dimension; + size_t first_seen; + } *temps; + enum vkd3d_shader_opcode *blocks; size_t depth; size_t blocks_capacity; @@ -1592,6 +1598,9 @@ static void vsir_validate_register(struct validation_context *ctx, switch (reg->type) { case VKD3DSPR_TEMP: + { + struct validation_context_temp_data *data; + if (reg->idx_count != 1) { validator_error(ctx, VKD3D_SHADER_ERROR_VSIR_INVALID_INDEX_COUNT, "Invalid index count %u for a TEMP register.", @@ -1603,9 +1612,41 @@ static void vsir_validate_register(struct validation_context *ctx, validator_error(ctx, VKD3D_SHADER_ERROR_VSIR_INVALID_INDEX, "Non-NULL relative address for a TEMP register.");
if (reg->idx[0].offset >= temp_count) + { validator_error(ctx, VKD3D_SHADER_ERROR_VSIR_INVALID_INDEX, "TEMP register index %u exceeds the maximum count %u.", reg->idx[0].offset, temp_count); + break; + } + + /* parser->shader_desc.temp_count might be smaller then + * temp_count if the parser made a mistake; we still don't + * want to overflow the array. */ + if (reg->idx[0].offset >= ctx->parser->shader_desc.temp_count) + break; + data = &ctx->temps[reg->idx[0].offset]; + + if (reg->dimension == VSIR_DIMENSION_NONE) + { + validator_error(ctx, VKD3D_SHADER_ERROR_VSIR_INVALID_DIMENSION, "Invalid dimension NONE for a TEMP register."); + break; + } + + /* TEMP registers can be scalar or vec4, provided that + * each individual register always appears with the same + * dimension. */ + if (data->dimension == VSIR_DIMENSION_NONE) + { + data->dimension = reg->dimension; + data->first_seen = ctx->instruction_idx; + } + else if (data->dimension != reg->dimension) + { + validator_error(ctx, VKD3D_SHADER_ERROR_VSIR_INVALID_DIMENSION, "Invalid dimension %#x for a TEMP register: " + "it has already been seen with dimension %#x at instruction %zu.", + reg->dimension, data->dimension, data->first_seen); + } break; + }
case VKD3DSPR_SSA: if (reg->idx_count != 1) @@ -1883,7 +1924,7 @@ static void vsir_validate_instruction(struct validation_context *ctx) } }
-void vsir_validate(struct vkd3d_shader_parser *parser) +enum vkd3d_result vsir_validate(struct vkd3d_shader_parser *parser) { struct validation_context ctx = { @@ -1892,7 +1933,10 @@ void vsir_validate(struct vkd3d_shader_parser *parser) };
if (!(parser->config_flags & VKD3D_SHADER_CONFIG_FLAG_FORCE_VALIDATION)) - return; + return VKD3D_OK; + + if (!(ctx.temps = vkd3d_calloc(parser->shader_desc.temp_count, sizeof(*ctx.temps)))) + goto fail;
for (ctx.instruction_idx = 0; ctx.instruction_idx < parser->instructions.count; ++ctx.instruction_idx) vsir_validate_instruction(&ctx); @@ -1901,4 +1945,13 @@ void vsir_validate(struct vkd3d_shader_parser *parser) validator_error(&ctx, VKD3D_SHADER_ERROR_VSIR_INVALID_INSTRUCTION_NESTING, "%zu nested blocks were not closed.", ctx.depth);
vkd3d_free(ctx.blocks); + vkd3d_free(ctx.temps); + + return VKD3D_OK; + +fail: + vkd3d_free(ctx.blocks); + vkd3d_free(ctx.temps); + + return VKD3D_ERROR_OUT_OF_MEMORY; } diff --git a/libs/vkd3d-shader/vkd3d_shader_private.h b/libs/vkd3d-shader/vkd3d_shader_private.h index 199a47a76..8ef4f3302 100644 --- a/libs/vkd3d-shader/vkd3d_shader_private.h +++ b/libs/vkd3d-shader/vkd3d_shader_private.h @@ -1448,7 +1448,7 @@ 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); +enum vkd3d_result 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 | 41 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 41 insertions(+)
diff --git a/libs/vkd3d-shader/ir.c b/libs/vkd3d-shader/ir.c index b0fd2c1eb..b53c2ee57 100644 --- a/libs/vkd3d-shader/ir.c +++ b/libs/vkd3d-shader/ir.c @@ -1533,6 +1533,12 @@ struct validation_context size_t first_seen; } *temps;
+ struct validation_context_ssa_data + { + enum vsir_dimension dimension; + size_t first_seen; + } *ssas; + enum vkd3d_shader_opcode *blocks; size_t depth; size_t blocks_capacity; @@ -1649,6 +1655,9 @@ static void vsir_validate_register(struct validation_context *ctx, }
case VKD3DSPR_SSA: + { + struct validation_context_ssa_data *data; + if (reg->idx_count != 1) { validator_error(ctx, VKD3D_SHADER_ERROR_VSIR_INVALID_INDEX_COUNT, "Invalid index count %u for a SSA register.", @@ -1660,9 +1669,36 @@ static void vsir_validate_register(struct validation_context *ctx, validator_error(ctx, VKD3D_SHADER_ERROR_VSIR_INVALID_INDEX, "Non-NULL relative address for a SSA register.");
if (reg->idx[0].offset >= ctx->parser->shader_desc.ssa_count) + { validator_error(ctx, VKD3D_SHADER_ERROR_VSIR_INVALID_INDEX, "SSA register index %u exceeds the maximum count %u.", reg->idx[0].offset, ctx->parser->shader_desc.ssa_count); + break; + } + + data = &ctx->ssas[reg->idx[0].offset]; + + if (reg->dimension == VSIR_DIMENSION_NONE) + { + validator_error(ctx, VKD3D_SHADER_ERROR_VSIR_INVALID_DIMENSION, "Invalid dimension NONE for a SSA register."); + break; + } + + /* SSA registers can be scalar or vec4, provided that each + * individual register always appears with the same + * dimension. */ + if (data->dimension == VSIR_DIMENSION_NONE) + { + data->dimension = reg->dimension; + data->first_seen = ctx->instruction_idx; + } + else if (data->dimension != reg->dimension) + { + validator_error(ctx, VKD3D_SHADER_ERROR_VSIR_INVALID_DIMENSION, "Invalid dimension %#x for a SSA register: " + "it has already been seen with dimension %#x at instruction %zu.", + reg->dimension, data->dimension, data->first_seen); + } break; + }
case VKD3DSPR_NULL: if (reg->idx_count != 0) @@ -1938,6 +1974,9 @@ enum vkd3d_result vsir_validate(struct vkd3d_shader_parser *parser) if (!(ctx.temps = vkd3d_calloc(parser->shader_desc.temp_count, sizeof(*ctx.temps)))) goto fail;
+ if (!(ctx.ssas = vkd3d_calloc(parser->shader_desc.ssa_count, sizeof(*ctx.ssas)))) + goto fail; + for (ctx.instruction_idx = 0; ctx.instruction_idx < parser->instructions.count; ++ctx.instruction_idx) vsir_validate_instruction(&ctx);
@@ -1946,12 +1985,14 @@ enum vkd3d_result vsir_validate(struct vkd3d_shader_parser *parser)
vkd3d_free(ctx.blocks); vkd3d_free(ctx.temps); + vkd3d_free(ctx.ssas);
return VKD3D_OK;
fail: vkd3d_free(ctx.blocks); vkd3d_free(ctx.temps); + vkd3d_free(ctx.ssas);
return VKD3D_ERROR_OUT_OF_MEMORY; }
From: Giovanni Mascellani gmascellani@codeweavers.com
Specifically, they are assigned only once and only assigned components are used.
Right now we don't check that the assignment dominates all usages. --- libs/vkd3d-shader/ir.c | 55 +++++++++++++++++++++++- libs/vkd3d-shader/vkd3d_shader_private.h | 1 + 2 files changed, 54 insertions(+), 2 deletions(-)
diff --git a/libs/vkd3d-shader/ir.c b/libs/vkd3d-shader/ir.c index b53c2ee57..358197d27 100644 --- a/libs/vkd3d-shader/ir.c +++ b/libs/vkd3d-shader/ir.c @@ -1523,6 +1523,7 @@ struct validation_context { struct vkd3d_shader_parser *parser; size_t instruction_idx; + bool invalid_instruction_idx; bool dcl_temps_found; unsigned int temp_count; enum vkd3d_shader_opcode phase; @@ -1537,6 +1538,9 @@ struct validation_context { enum vsir_dimension dimension; size_t first_seen; + uint32_t write_mask; + uint32_t read_mask; + size_t first_assigned; } *ssas;
enum vkd3d_shader_opcode *blocks; @@ -1556,8 +1560,16 @@ static void VKD3D_PRINTF_FUNC(3, 4) validator_error(struct validation_context *c 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); - ERR("VSIR validation error: instruction %zu: %s\n", ctx->instruction_idx + 1, buf.buffer); + if (ctx->invalid_instruction_idx) + { + vkd3d_shader_parser_error(ctx->parser, error, "instruction %zu: %s", ctx->instruction_idx + 1, buf.buffer); + ERR("VSIR validation error: instruction %zu: %s\n", ctx->instruction_idx + 1, buf.buffer); + } + else + { + vkd3d_shader_parser_error(ctx->parser, error, "%s", buf.buffer); + ERR("VSIR validation error: %s\n", buf.buffer); + }
vkd3d_string_buffer_cleanup(&buf); } @@ -1771,6 +1783,23 @@ static void vsir_validate_dst_param(struct validation_context *ctx, validator_error(ctx, VKD3D_SHADER_ERROR_VSIR_INVALID_SHIFT, "Destination has invalid shift %#x.", dst->shift); } + + if (dst->reg.type == VKD3DSPR_SSA && dst->reg.idx[0].offset < ctx->parser->shader_desc.ssa_count) + { + struct validation_context_ssa_data *data = &ctx->ssas[dst->reg.idx[0].offset]; + + if (data->write_mask == 0) + { + data->write_mask = dst->write_mask; + data->first_assigned = ctx->instruction_idx; + } + else + { + validator_error(ctx, VKD3D_SHADER_ERROR_VSIR_INVALID_SSA_USAGE, + "SSA register is already assigned at instruction %zu.", + data->first_assigned); + } + } }
static void vsir_validate_src_param(struct validation_context *ctx, @@ -1789,6 +1818,15 @@ static void vsir_validate_src_param(struct validation_context *ctx, if (src->modifiers >= VKD3DSPSM_COUNT) validator_error(ctx, VKD3D_SHADER_ERROR_VSIR_INVALID_MODIFIERS, "Source has invalid modifiers %#x.", src->modifiers); + + if (src->reg.type == VKD3DSPR_SSA && src->reg.idx[0].offset < ctx->parser->shader_desc.ssa_count) + { + struct validation_context_ssa_data *data = &ctx->ssas[src->reg.idx[0].offset]; + unsigned int i; + + for (i = 0; i < VKD3D_VEC4_SIZE; ++i) + data->read_mask |= (1u << vsir_swizzle_get_component(src->swizzle, i)); + } }
static void vsir_validate_dst_count(struct validation_context *ctx, @@ -1967,6 +2005,7 @@ enum vkd3d_result vsir_validate(struct vkd3d_shader_parser *parser) .parser = parser, .phase = VKD3DSIH_INVALID, }; + unsigned int i;
if (!(parser->config_flags & VKD3D_SHADER_CONFIG_FLAG_FORCE_VALIDATION)) return VKD3D_OK; @@ -1980,9 +2019,21 @@ enum vkd3d_result vsir_validate(struct vkd3d_shader_parser *parser) for (ctx.instruction_idx = 0; ctx.instruction_idx < parser->instructions.count; ++ctx.instruction_idx) vsir_validate_instruction(&ctx);
+ ctx.invalid_instruction_idx = true; + if (ctx.depth != 0) validator_error(&ctx, VKD3D_SHADER_ERROR_VSIR_INVALID_INSTRUCTION_NESTING, "%zu nested blocks were not closed.", ctx.depth);
+ for (i = 0; i < parser->shader_desc.ssa_count; ++i) + { + struct validation_context_ssa_data *data = &ctx.ssas[i]; + + if ((data->write_mask | data->read_mask) != data->write_mask) + validator_error(&ctx, VKD3D_SHADER_ERROR_VSIR_INVALID_SSA_USAGE, + "SSA register %u has invalid read mask %#x, which is not a subset of the write mask %#x " + "at the point of definition.", i, data->read_mask, data->write_mask); + } + vkd3d_free(ctx.blocks); vkd3d_free(ctx.temps); vkd3d_free(ctx.ssas); diff --git a/libs/vkd3d-shader/vkd3d_shader_private.h b/libs/vkd3d-shader/vkd3d_shader_private.h index 8ef4f3302..4de2581ba 100644 --- a/libs/vkd3d-shader/vkd3d_shader_private.h +++ b/libs/vkd3d-shader/vkd3d_shader_private.h @@ -216,6 +216,7 @@ enum vkd3d_shader_error VKD3D_SHADER_ERROR_VSIR_INVALID_DCL_TEMPS = 9014, VKD3D_SHADER_ERROR_VSIR_INVALID_INDEX = 9015, VKD3D_SHADER_ERROR_VSIR_INVALID_INSTRUCTION_NESTING = 9016, + VKD3D_SHADER_ERROR_VSIR_INVALID_SSA_USAGE = 9017,
VKD3D_SHADER_WARNING_VSIR_DYNAMIC_DESCRIPTOR_ARRAY = 9300, };
On Thu Jan 11 22:07:15 2024 +0000, Henri Verbeet wrote:
That probably makes sense. I imagine it may be helpful to have that clarification somewhere in the code as well though, for future reference.
I hope the new proposal is better.
Let me point out that I'm not completely convinced myself of what is the right choice here. I can see good reasons for mandating the TEMPs are always vec4. So for the moment I'm sticking with the more permissive choice and we'll see in the future. The option of introducing a pass that converts all TEMPs to vec4 remains on the plate, also (though it is already validated that scalar registers always have the obvious write mask and swizzle, so the backend can probably just ignore the dimension if so they wish).
-
Giovanni Mascellani -
Giovanni Mascellani (＠giomasce)