[PATCH v3 0/3] 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: -- v3: vkd3d-shader/tpf: Avoid reading constant value components beyond type's width. vkd3d-shader/hlsl: Initialize unused constant components to zero. vkd3d-shader/hlsl: Initialize all constant values in hlsl_fold_constant_swizzles(). https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/308
From: Francisco Casas <fcasas(a)codeweavers.com> 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) --- libs/vkd3d-shader/hlsl_constant_ops.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/libs/vkd3d-shader/hlsl_constant_ops.c b/libs/vkd3d-shader/hlsl_constant_ops.c index 41a72ab6..28df074d 100644 --- a/libs/vkd3d-shader/hlsl_constant_ops.c +++ b/libs/vkd3d-shader/hlsl_constant_ops.c @@ -1008,7 +1008,7 @@ bool hlsl_fold_constant_exprs(struct hlsl_ctx *ctx, struct hlsl_ir_node *instr, bool hlsl_fold_constant_swizzles(struct hlsl_ctx *ctx, struct hlsl_ir_node *instr, void *context) { - struct hlsl_constant_value value; + struct hlsl_constant_value value = {}; struct hlsl_ir_swizzle *swizzle; struct hlsl_ir_constant *src; struct hlsl_ir_node *dst; -- GitLab https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/308
From: Francisco Casas <fcasas(a)codeweavers.com> While some of the components of vec4 immconsts may not be used, they are still written in the immconst declaration, so it is better if they are initialized as zero. --- libs/vkd3d-shader/hlsl_codegen.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/libs/vkd3d-shader/hlsl_codegen.c b/libs/vkd3d-shader/hlsl_codegen.c index bfa605f4..d3676c09 100644 --- a/libs/vkd3d-shader/hlsl_codegen.c +++ b/libs/vkd3d-shader/hlsl_codegen.c @@ -2355,7 +2355,7 @@ static bool lower_round(struct hlsl_ctx *ctx, struct hlsl_ir_node *instr, void * { struct hlsl_ir_node *arg, *neg, *sum, *frc, *half, *replacement; struct hlsl_type *type = instr->data_type; - struct hlsl_constant_value half_value; + struct hlsl_constant_value half_value = {}; unsigned int i, component_count; struct hlsl_ir_expr *expr; @@ -2466,7 +2466,7 @@ static bool lower_int_division(struct hlsl_ctx *ctx, struct hlsl_ir_node *instr, { struct hlsl_ir_node *arg1, *arg2, *xor, *and, *abs1, *abs2, *div, *neg, *cast1, *cast2, *cast3, *high_bit; struct hlsl_type *type = instr->data_type, *utype; - struct hlsl_constant_value high_bit_value; + struct hlsl_constant_value high_bit_value = {}; struct hlsl_ir_expr *expr; unsigned int i; @@ -2532,7 +2532,7 @@ static bool lower_int_modulus(struct hlsl_ctx *ctx, struct hlsl_ir_node *instr, { struct hlsl_ir_node *arg1, *arg2, *and, *abs1, *abs2, *div, *neg, *cast1, *cast2, *cast3, *high_bit; struct hlsl_type *type = instr->data_type, *utype; - struct hlsl_constant_value high_bit_value; + struct hlsl_constant_value high_bit_value = {}; struct hlsl_ir_expr *expr; unsigned int i; @@ -2675,7 +2675,7 @@ static bool lower_float_modulus(struct hlsl_ctx *ctx, struct hlsl_ir_node *instr { struct hlsl_ir_node *arg1, *arg2, *mul1, *neg1, *ge, *neg2, *div, *mul2, *frc, *cond, *one, *mul3; struct hlsl_type *type = instr->data_type, *btype; - struct hlsl_constant_value one_value; + struct hlsl_constant_value one_value = {}; struct hlsl_ir_expr *expr; unsigned int i; -- GitLab 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, just in case. --- 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: I added 3/3 to avoid reading undefined values just before writing the immconst registers. I verified that this patch alone is enough to solve the issue but yep, we probably want to avoid uninitialized data in the HLSL too, so I kept the other 2 patches. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/308#note_43056
Personally I would just drop 1/3 and 2/3, but I'm willing to be gainsaid.
This error was reported by valgrind:
Incidentally, for the future, the convention for bugs found by tools is to put the tool name in parentheses at the end of the subject line, e.g. "vkd3d-shader/hlsl: Initialize something (Valgrind)." -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/308#note_43130
participants (3)
-
Francisco Casas -
Francisco Casas (＠fcasas)
-
Zebediah Figura (＠zfigura)