[PATCH v2 0/6] MR444: vkd3d-shader/ir: Check that blocks are correctly nested.
-- v2: vkd3d-shader/ir: Check that SWITCH blocks are correctly nested. vkd3d-shader/ir: Check that REP blocks are correctly nested. vkd3d-shader/ir: Check that LOOP blocks are correctly nested. vkd3d-shader/ir: Check that IF blocks are correctly nested. vkd3d-shader/ir: Do not enfore DCL_TEMPS count for hull shaders. vkd3d-shader/ir: Emit an ERR() on validation errors. https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/444
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 8cf92656b..6cfad1147 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); + ERR("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 6cfad1147..171dbb53b 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 | 35 ++++++++++++++++++++++++ libs/vkd3d-shader/vkd3d_shader_private.h | 1 + 2 files changed, 36 insertions(+) diff --git a/libs/vkd3d-shader/ir.c b/libs/vkd3d-shader/ir.c index 171dbb53b..52ddc481f 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,32 @@ 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."); + else + 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."); + else + --ctx->depth; + break; + default: break; } @@ -1648,4 +1678,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 912267e0e..5ab728816 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 | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/libs/vkd3d-shader/ir.c b/libs/vkd3d-shader/ir.c index 52ddc481f..052232e35 100644 --- a/libs/vkd3d-shader/ir.c +++ b/libs/vkd3d-shader/ir.c @@ -1664,6 +1664,23 @@ 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."); + else + --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 | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/libs/vkd3d-shader/ir.c b/libs/vkd3d-shader/ir.c index 052232e35..e76a5c93e 100644 --- a/libs/vkd3d-shader/ir.c +++ b/libs/vkd3d-shader/ir.c @@ -1681,6 +1681,23 @@ 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."); + else + --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 | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/libs/vkd3d-shader/ir.c b/libs/vkd3d-shader/ir.c index e76a5c93e..afa8d8538 100644 --- a/libs/vkd3d-shader/ir.c +++ b/libs/vkd3d-shader/ir.c @@ -1698,6 +1698,23 @@ 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."); + else + --ctx->depth; + break; + default: break; } -- GitLab https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/444
This merge request was approved by Giovanni Mascellani. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/444
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.
We might need to create a secondary output buffer with an independent log level for debug output. I think the problem is solvable, but no need to worry about it for now.
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?
Effectively, yes. More formally it's the number of unique temporaries used by the shader. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/444#note_51357
This merge request was approved by Henri Verbeet. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/444
participants (3)
-
Giovanni Mascellani -
Giovanni Mascellani (＠giomasce)
-
Henri Verbeet (＠hverbeet)