[PATCH 0/1] MR227: vkd3d-shader/hlsl: Initialize some unused constant values to a sentinel.

[Zebediah Figura (＠zfigura)]

I feel lukewarm about this, but it did help me find 974528cbe and 7b476573f.

[PATCH 1/1] vkd3d-shader/hlsl: Initialize some unused constant values to a sentinel.

[Zebediah Figura]

From: Zebediah Figura

---
 libs/vkd3d-shader/hlsl.c | 2 ++
 libs/vkd3d-shader/tpf.c  | 2 ++
 2 files changed, 4 insertions(+)

diff --git a/libs/vkd3d-shader/hlsl.c b/libs/vkd3d-shader/hlsl.c
index f439c9f3..5adeae57 100644
--- a/libs/vkd3d-shader/hlsl.c
+++ b/libs/vkd3d-shader/hlsl.c
@@ -1158,6 +1158,8 @@ struct hlsl_ir_node *hlsl_new_constant(struct hlsl_ctx *ctx, struct hlsl_type *t

init_node(&c->node, HLSL_IR_CONSTANT, type, loc);
     c->value = *value;
+    for (unsigned int i = type->dimx; i < 4; ++i)
+        c->value.u[i].u = 0x55555555;

return &c->node;
 }
diff --git a/libs/vkd3d-shader/tpf.c b/libs/vkd3d-shader/tpf.c
index d066b13e..9cbd9897 100644
--- a/libs/vkd3d-shader/tpf.c
+++ b/libs/vkd3d-shader/tpf.c
@@ -3531,6 +3531,8 @@ static void sm4_src_from_constant_value(struct sm4_src_register *src,
         {
             if (map_writemask & (1u << i))
                 src->reg.immconst_uint[i] = value->u[j++].u;
+            else
+                src->reg.immconst_uint[i] = 0x55555555;
         }
     }
 }

[Giovanni Mascellani (＠giomasce)]

Giovanni Mascellani (@giomasce) commented about libs/vkd3d-shader/tpf.c:
>      {
>          if (map_writemask & (1u << i))
>              src->reg.immconst_uint[i] = value->u[j++].u;
> -
>        else
> -
>            src->reg.immconst_uint[i] = 0x55555555;

This would be visible in the generated TPF code. While it should be harmless, I think I'd prefer to not have it. Hopefully there won't be many other bugs like this, and it's trivial for a developer to add back this gadget in their code if required.

[Giovanni Mascellani (＠giomasce)]

Giovanni Mascellani (@giomasce) commented about libs/vkd3d-shader/hlsl.c:
>  init_node(&c->node, HLSL_IR_CONSTANT, type, loc);
>  c->value = *value;
> - for (unsigned int i = type->dimx; i < 4; ++i)
> -
>    c->value.u[i].u = 0x55555555;

I don't feel really enthusiastic about this either. I guess it can help, but maybe valgrind can catch this kind of problems with even better tooling? (incidentally, with this change valgrind's job becomes harder)

[Zebediah Figura (＠zfigura)]

On Fri Jun  9 09:56:12 2023 +0000, Giovanni Mascellani wrote:
> I don't feel really enthusiastic about this either. I guess it can help,
> but maybe valgrind can catch this kind of problems with even better
> tooling? (incidentally, with this change valgrind's job becomes harder)

Valgrind already misses this, because we zero-initialize instructions (and we probably want to keep doing that). We could use valgrind escapes to mark it as uninitialized, though.

[PATCH 0/1] MR227: vkd3d-shader/hlsl: Initialize some unused constant values to a sentinel. - approved

[Zebediah Figura (＠zfigura)]

This merge request was approved by Zebediah Figura.

[Henri Verbeet (＠hverbeet)]

> This would be visible in the generated TPF code. While it should be harmless, I think I'd prefer to not have it. Hopefully there won't be many other bugs like this, and it's trivial for a developer to add back this gadget in their code if required.

We could make this kind of thing a compilation option, I suppose?

[Giovanni Mascellani (＠giomasce)]

> We could make this kind of thing a compilation option, I suppose?

That would be much better for me.

Incidentally, if we want to do this I'd then suggest to use different values, so it's easier to see where a poisoned value comes from. Also, one value could be used for the first `dimx` components and another value for the others. Just saying, though, anything is fine for me.

[PATCH 0/1] MR227: vkd3d-shader/hlsl: Initialize some unused constant values to a sentinel. - closed

[Zebediah Figura (＠zfigura)]

This merge request was closed by Zebediah Figura.