[PATCH 0/6] MR444: vkd3d-shader/ir: Check that blocks are correctly nested.

[PATCH 1/6] vkd3d-shader/ir: Emit a FIXME() on validation errors.

[Giovanni Mascellani]

From: Giovanni Mascellani

---
 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);
 }

[PATCH 2/6] vkd3d-shader/ir: Do not enfore DCL_TEMPS count for hull shaders.

[Giovanni Mascellani]

From: Giovanni Mascellani

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;

[PATCH 3/6] vkd3d-shader/ir: Check that IF blocks are correctly nested.

[Giovanni Mascellani]

From: Giovanni Mascellani

---
 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

[PATCH 4/6] vkd3d-shader/ir: Check that LOOP blocks are correctly nested.

[Giovanni Mascellani]

From: Giovanni Mascellani

---
 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;
     }

[PATCH 5/6] vkd3d-shader/ir: Check that REP blocks are correctly nested.

[Giovanni Mascellani]

From: Giovanni Mascellani

---
 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;
     }

[PATCH 6/6] vkd3d-shader/ir: Check that SWITCH blocks are correctly nested.

[Giovanni Mascellani]

From: Giovanni Mascellani

---
 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;
     }

[Henri Verbeet (＠hverbeet)]

> @@ -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.
> @@ -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.

[Giovanni Mascellani (＠giomasce)]

> 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.