[PATCH 0/1] MR227: vkd3d-shader/hlsl: Initialize some unused constant values to a sentinel.
I feel lukewarm about this, but it did help me find 974528cbe and 7b476573f. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/227
From: Zebediah Figura <zfigura(a)codeweavers.com> --- 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; } } } -- GitLab https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/227
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. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/227#note_35252
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) -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/227#note_35253
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.
-- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/227#note_35280
This merge request was approved by Zebediah Figura. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/227
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? -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/227#note_35423
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. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/227#note_35427
This merge request was closed by Zebediah Figura. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/227
participants (4)
-
Giovanni Mascellani (＠giomasce) -
Henri Verbeet (＠hverbeet)
-
Zebediah Figura
-
Zebediah Figura (＠zfigura)