[PATCH v4 0/1] MR308: vkd3d-shader/hlsl: Initialize unused constant values in compilation passes.
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
From: Francisco Casas <fcasas(a)codeweavers.com> 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; } } } -- GitLab https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/308
: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. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/308#note_43199
Shouldn't the condition be "j < width" rather than "i < width"? Actually, for that matter, shouldn't "j < width" be an assertion? -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/308#note_43205
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`. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/308#note_43247
participants (3)
-
Francisco Casas -
Francisco Casas (＠fcasas)
-
Zebediah Figura (＠zfigura)