[PATCH 0/6] MR444: vkd3d-shader/ir: Check that blocks are correctly nested.
From: Giovanni Mascellani <gmascellani(a)codeweavers.com> --- libs/vkd3d-shader/ir.c | 1 + 1 file changed, 1 insertion(+) diff --git a/libs/vkd3d-shader/ir.c b/libs/vkd3d-shader/ir.c index 8cf92656..ac7605bd 100644 --- a/libs/vkd3d-shader/ir.c +++ b/libs/vkd3d-shader/ir.c @@ -1489,6 +1489,7 @@ static void VKD3D_PRINTF_FUNC(3, 4) validator_error(struct validation_context *c va_end(args); vkd3d_shader_parser_error(ctx->parser, error, "instruction %zu: %s", ctx->instruction_idx + 1, buf.buffer); + FIXME("VSIR validation error: instruction %zu: %s\n", ctx->instruction_idx + 1, buf.buffer); vkd3d_string_buffer_cleanup(&buf); } -- GitLab https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/444
From: Giovanni Mascellani <gmascellani(a)codeweavers.com> Hull shaders have a different temps count for each phase, and the parser only reports the count for the patch constant phase. In order to properly check for temps count on hull shaders, we first need to decode its phases. --- libs/vkd3d-shader/ir.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/libs/vkd3d-shader/ir.c b/libs/vkd3d-shader/ir.c index ac7605bd..109c8ad0 100644 --- a/libs/vkd3d-shader/ir.c +++ b/libs/vkd3d-shader/ir.c @@ -1628,7 +1628,8 @@ static void vsir_validate_instruction(struct validation_context *ctx) if (ctx->dcl_temps_found && ctx->parser->shader_version.type != VKD3D_SHADER_TYPE_HULL) validator_error(ctx, VKD3D_SHADER_ERROR_VSIR_DUPLICATE_DCL_TEMPS, "Duplicate DCL_TEMPS instruction."); ctx->dcl_temps_found = true; - if (instruction->declaration.count != ctx->parser->shader_desc.temp_count) + if (instruction->declaration.count != ctx->parser->shader_desc.temp_count && + ctx->parser->shader_version.type != VKD3D_SHADER_TYPE_HULL) validator_error(ctx, VKD3D_SHADER_ERROR_VSIR_INVALID_DCL_TEMPS, "Invalid DCL_TEMPS count %u, expected %u.", instruction->declaration.count, ctx->parser->shader_desc.temp_count); break; -- GitLab https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/444
From: Giovanni Mascellani <gmascellani(a)codeweavers.com> --- libs/vkd3d-shader/ir.c | 33 ++++++++++++++++++++++++ libs/vkd3d-shader/vkd3d_shader_private.h | 1 + 2 files changed, 34 insertions(+) diff --git a/libs/vkd3d-shader/ir.c b/libs/vkd3d-shader/ir.c index 109c8ad0..23485b09 100644 --- a/libs/vkd3d-shader/ir.c +++ b/libs/vkd3d-shader/ir.c @@ -1474,6 +1474,10 @@ struct validation_context struct vkd3d_shader_parser *parser; size_t instruction_idx; bool dcl_temps_found; + + enum vkd3d_shader_opcode *blocks; + size_t depth; + size_t blocks_capacity; }; static void VKD3D_PRINTF_FUNC(3, 4) validator_error(struct validation_context *ctx, @@ -1634,6 +1638,30 @@ static void vsir_validate_instruction(struct validation_context *ctx) instruction->declaration.count, ctx->parser->shader_desc.temp_count); break; + case VKD3DSIH_IF: + vsir_validate_dst_count(ctx, instruction, 0); + vsir_validate_src_count(ctx, instruction, 1); + if (!vkd3d_array_reserve((void **)&ctx->blocks, &ctx->blocks_capacity, ctx->depth + 1, sizeof(*ctx->blocks))) + return; + ctx->blocks[ctx->depth++] = instruction->handler_idx; + break; + + case VKD3DSIH_ELSE: + vsir_validate_dst_count(ctx, instruction, 0); + vsir_validate_src_count(ctx, instruction, 0); + if (ctx->depth == 0 || ctx->blocks[ctx->depth - 1] != VKD3DSIH_IF) + validator_error(ctx, VKD3D_SHADER_ERROR_VSIR_INVALID_INSTRUCTION_NESTING, "ELSE instruction doesn't terminate IF block."); + ctx->blocks[ctx->depth - 1] = instruction->handler_idx; + break; + + case VKD3DSIH_ENDIF: + vsir_validate_dst_count(ctx, instruction, 0); + vsir_validate_src_count(ctx, instruction, 0); + if (ctx->depth == 0 || (ctx->blocks[ctx->depth - 1] != VKD3DSIH_IF && ctx->blocks[ctx->depth - 1] != VKD3DSIH_ELSE)) + validator_error(ctx, VKD3D_SHADER_ERROR_VSIR_INVALID_INSTRUCTION_NESTING, "ENDIF instruction doesn't terminate IF/ELSE block."); + --ctx->depth; + break; + default: break; } @@ -1648,4 +1676,9 @@ void 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); + + 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); } diff --git a/libs/vkd3d-shader/vkd3d_shader_private.h b/libs/vkd3d-shader/vkd3d_shader_private.h index 912267e0..67303fef 100644 --- a/libs/vkd3d-shader/vkd3d_shader_private.h +++ b/libs/vkd3d-shader/vkd3d_shader_private.h @@ -209,6 +209,7 @@ enum vkd3d_shader_error VKD3D_SHADER_ERROR_VSIR_DUPLICATE_DCL_TEMPS = 9013, VKD3D_SHADER_ERROR_VSIR_INVALID_DCL_TEMPS = 9014, VKD3D_SHADER_ERROR_VSIR_INVALID_INDEX = 9015, + VKD3D_SHADER_ERROR_VSIR_INVALID_INSTRUCTION_NESTING = 9016, }; enum vkd3d_shader_opcode -- GitLab https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/444
From: Giovanni Mascellani <gmascellani(a)codeweavers.com> --- libs/vkd3d-shader/ir.c | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/libs/vkd3d-shader/ir.c b/libs/vkd3d-shader/ir.c index 23485b09..1a3f3d67 100644 --- a/libs/vkd3d-shader/ir.c +++ b/libs/vkd3d-shader/ir.c @@ -1662,6 +1662,22 @@ static void vsir_validate_instruction(struct validation_context *ctx) --ctx->depth; break; + case VKD3DSIH_LOOP: + vsir_validate_dst_count(ctx, instruction, 0); + vsir_validate_src_count(ctx, instruction, 0); + if (!vkd3d_array_reserve((void **)&ctx->blocks, &ctx->blocks_capacity, ctx->depth + 1, sizeof(*ctx->blocks))) + return; + ctx->blocks[ctx->depth++] = instruction->handler_idx; + break; + + case VKD3DSIH_ENDLOOP: + vsir_validate_dst_count(ctx, instruction, 0); + vsir_validate_src_count(ctx, instruction, 0); + if (ctx->depth == 0 || ctx->blocks[ctx->depth - 1] != VKD3DSIH_LOOP) + validator_error(ctx, VKD3D_SHADER_ERROR_VSIR_INVALID_INSTRUCTION_NESTING, "ENDLOOP instruction doesn't terminate LOOP block."); + --ctx->depth; + break; + default: break; } -- GitLab https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/444
From: Giovanni Mascellani <gmascellani(a)codeweavers.com> --- libs/vkd3d-shader/ir.c | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/libs/vkd3d-shader/ir.c b/libs/vkd3d-shader/ir.c index 1a3f3d67..de689e19 100644 --- a/libs/vkd3d-shader/ir.c +++ b/libs/vkd3d-shader/ir.c @@ -1678,6 +1678,22 @@ static void vsir_validate_instruction(struct validation_context *ctx) --ctx->depth; break; + case VKD3DSIH_REP: + vsir_validate_dst_count(ctx, instruction, 0); + vsir_validate_src_count(ctx, instruction, 1); + if (!vkd3d_array_reserve((void **)&ctx->blocks, &ctx->blocks_capacity, ctx->depth + 1, sizeof(*ctx->blocks))) + return; + ctx->blocks[ctx->depth++] = instruction->handler_idx; + break; + + case VKD3DSIH_ENDREP: + vsir_validate_dst_count(ctx, instruction, 0); + vsir_validate_src_count(ctx, instruction, 0); + if (ctx->depth == 0 || ctx->blocks[ctx->depth - 1] != VKD3DSIH_REP) + validator_error(ctx, VKD3D_SHADER_ERROR_VSIR_INVALID_INSTRUCTION_NESTING, "ENDREP instruction doesn't terminate REP block."); + --ctx->depth; + break; + default: break; } -- GitLab https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/444
From: Giovanni Mascellani <gmascellani(a)codeweavers.com> --- libs/vkd3d-shader/ir.c | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/libs/vkd3d-shader/ir.c b/libs/vkd3d-shader/ir.c index de689e19..f958f757 100644 --- a/libs/vkd3d-shader/ir.c +++ b/libs/vkd3d-shader/ir.c @@ -1694,6 +1694,22 @@ static void vsir_validate_instruction(struct validation_context *ctx) --ctx->depth; break; + case VKD3DSIH_SWITCH: + vsir_validate_dst_count(ctx, instruction, 0); + vsir_validate_src_count(ctx, instruction, 1); + if (!vkd3d_array_reserve((void **)&ctx->blocks, &ctx->blocks_capacity, ctx->depth + 1, sizeof(*ctx->blocks))) + return; + ctx->blocks[ctx->depth++] = instruction->handler_idx; + break; + + case VKD3DSIH_ENDSWITCH: + vsir_validate_dst_count(ctx, instruction, 0); + vsir_validate_src_count(ctx, instruction, 0); + if (ctx->depth == 0 || ctx->blocks[ctx->depth - 1] != VKD3DSIH_SWITCH) + validator_error(ctx, VKD3D_SHADER_ERROR_VSIR_INVALID_INSTRUCTION_NESTING, "ENDSWITCH instruction doesn't terminate SWITCH block."); + --ctx->depth; + break; + default: break; } -- GitLab https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/444
```diff @@ -1489,6 +1489,7 @@ static void VKD3D_PRINTF_FUNC(3, 4) validator_error(struct validation_context *c va_end(args);
vkd3d_shader_parser_error(ctx->parser, error, "instruction %zu: %s", ctx->instruction_idx + 1, buf.buffer); + FIXME("VSIR validation error: instruction %zu: %s\n", ctx->instruction_idx + 1, buf.buffer);
vkd3d_string_buffer_cleanup(&buf); } ```
ERR seems appropriate. There's perhaps also an argument to be made for dumping the messages buffer to ERR on failure in vsir_validate() instead of duplicating the message here; I don't feel strongly about it though.
``` Hull shaders have a different temps count for each phase, and the parser only reports the count for the patch constant phase. In order to properly check for temps count on hull shaders, we first need to decode its phases. ```
Right, though in principle DCL_TEMPS should at least never have a count greater than "shader_desc.temp_count" either.
```diff @@ -209,6 +209,7 @@ enum vkd3d_shader_error VKD3D_SHADER_ERROR_VSIR_DUPLICATE_DCL_TEMPS = 9013, VKD3D_SHADER_ERROR_VSIR_INVALID_DCL_TEMPS = 9014, VKD3D_SHADER_ERROR_VSIR_INVALID_INDEX = 9015, + VKD3D_SHADER_ERROR_VSIR_INVALID_INSTRUCTION_NESTING = 9016, ```
Stray space. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/444#note_51105
ERR seems appropriate. There's perhaps also an argument to be made for dumping the messages buffer to ERR on failure in vsir_validate() instead of duplicating the message here; I don't feel strongly about it though.
So far I converted to `ERR()`. The problem with dumping the error buffer is that if the parser is created with logging disabled (which, for example, always happens when vkd3d wants to convert a binary shader to SPIR-V) the buffer will always be empty. Since that behavior makes sense I wouldn't want to change it, but at the same time it can be useful for developers to see validation errors.
Right, though in principle DCL_TEMPS should at least never have a count greater than "shader_desc.temp_count" either.
I'll soon parse HS phases, so I will able to validate this properly. So, `temp_count` is intended to be the maximum of all the DCL_TEMPS, right? I also fixed a bug with underflowing `depth` when blocks are not matched properly. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/444#note_51175
participants (3)
-
Giovanni Mascellani -
Giovanni Mascellani (＠giomasce)
-
Henri Verbeet (＠hverbeet)