[PATCH v4 0/1] MR308: vkd3d-shader/hlsl: Initialize unused constant values in compilation passes.

[Francisco Casas (＠fcasas)]

Even though the uninitialized values shouldn't be used in the output
binary program, they still show up in it, and affect the checksum, so
we better make them zeroes.

This error was reported by valgrind:

```
libtool --mode=execute valgrind --track-origins=yes vkd3d-build/tests/shader_runner vkd3d/tests/hlsl/static-initializer.shader_test
```

```
==46180== Conditional jump or move depends on uninitialised value(s)
==46180==    at 0x48D98C7: parse_dxbc.isra.0 (dxbc.c:182)
==46180==    by 0x48DA044: vkd3d_shader_parse_dxbc (dxbc.c:308)
==46180==    by 0x488D1B6: vkd3d_shader_parse_dxbc_source_type (vkd3d_shader_utils.h:37)
==46180==    by 0x488D1B6: create_shader_stage.isra.0 (state.c:1988)
==46180==    by 0x48926B6: d3d12_pipeline_state_init_graphics (state.c:3084)
==46180==    by 0x4893A96: d3d12_pipeline_state_create_graphics (state.c:3280)
==46180==    by 0x4878498: d3d12_device_CreateGraphicsPipelineState (device.c:2619)
==46180==    by 0x1FFEFFECC7: ???
==46180==    by 0xE8: ???
==46180==    by 0x47: ???
==46180==    by 0x61: ???
==46180==    by 0x660066000000023: ???
==46180==    by 0x661066100000044: ???
==46180==  Uninitialised value was created by a stack allocation
==46180==    at 0x48F3FF0: hlsl_fold_constant_swizzles (hlsl_constant_ops.c:1010)
```

Thank you valgrind! :smile:

--
  v4: vkd3d-shader/tpf: Avoid reading constant value components beyond type's width (Valgrind).

https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/308

[PATCH v4 1/1] vkd3d-shader/tpf: Avoid reading constant value components beyond type's width (Valgrind).

[Francisco Casas]

From: Francisco Casas

We are passing map writemasks that may have more components than the
constant's data type, so a (i < width) check is added to avoid reading
components from the constant that are not intended to be used.

Remaining values in the register are initialized to zero.
---
 libs/vkd3d-shader/tpf.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/libs/vkd3d-shader/tpf.c b/libs/vkd3d-shader/tpf.c
index 550f9b27..ce21eb24 100644
--- a/libs/vkd3d-shader/tpf.c
+++ b/libs/vkd3d-shader/tpf.c
@@ -3715,8 +3715,10 @@ static void sm4_src_from_constant_value(struct sm4_src_register *src,
         src->reg.dim = VKD3D_SM4_DIMENSION_VEC4;
         for (i = 0; i < 4; ++i)
         {
-            if (map_writemask & (1u << i))
+            if ((map_writemask & (1u << i)) && (i < width))
                 src->reg.immconst_uint[i] = value->u[j++].u;
+            else
+                src->reg.immconst_uint[i] = 0;
         }
     }
 }

[Francisco Casas (＠fcasas)]

:arrow_up: Dropping 1/3 and 2/3 because we agreed that valgrind could help us detect errors if those uninitialized components are used in other parts of the compiler. Provided we actually remember to run the tests with valgrind once in a while.

[Zebediah Figura (＠zfigura)]

Shouldn't the condition be "j < width" rather than "i < width"?

Actually, for that matter, shouldn't "j < width" be an assertion?

[Francisco Casas (＠fcasas)]

> Shouldn't the condition be "j < width" rather than "i < width"?

Yes, in fact, I failed to realize that 3 tests were not passing, because I just looked to valgrind's output...
> Actually, for that matter, shouldn't "j < width" be an assertion?

No, because we are deliberately passing map_writemask with more than "width" bites on in some places, usually using `VKD3DSP_WRITEMASK_ALL`.