Skip to content
Snippets Groups Projects

vkd3d-shader/hlsl: Initialize unused constant values in compilation passes.

Merged Francisco Casas requested to merge fcasas/vkd3d:fixv into master
1 unresolved thread

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:

Merge request reports

Loading
Loading

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
  • Francisco Casas added 1 commit

    added 1 commit

    • 7732afbf - vkd3d-shader/hlsl: Initialize unused constant components to zero.

    Compare with previous version

  • Author Developer

    :arrow_up: As an afterthought, I added 2/2 to also initialize the struct hlsl_constant_value components to zero in other passes where this issue may also happen.

  • Francisco Casas changed title from vkd3d-shader/hlsl: Initialize all constant values in hlsl_fold_constant_swizzles(). to vkd3d-shader/hlsl: Initialize unused constant values in compilation passes.

    changed title from vkd3d-shader/hlsl: Initialize all constant values in hlsl_fold_constant_swizzles(). to vkd3d-shader/hlsl: Initialize unused constant values in compilation passes.

  • Giovanni Mascellani approved this merge request

    approved this merge request

  • Henri Verbeet approved this merge request

    approved this merge request

  • If the concern is unused components being written in immconst registers, this doesn't seem like the right place to fix it.

  • If the concern is unused components being written in immconst registers, this doesn't seem like the right place to fix it.

    You're probably right; I don't think we should be writing these in the backend. I was thinking it's not great to carry uninitialised data in the HLSL IR either, but we should actually simply never access values beyond what's defined by the associated types.

  • Henri Verbeet unapproved this merge request

    unapproved this merge request

  • Francisco Casas added 3 commits

    added 3 commits

    • e6093389 - vkd3d-shader/hlsl: Initialize all constant values in hlsl_fold_constant_swizzles().
    • afdf12ec - vkd3d-shader/hlsl: Initialize unused constant components to zero.
    • dfe57d2c - vkd3d-shader/tpf: Avoid reading constant value components beyond type's width.

    Compare with previous version

  • Author Developer

    :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.

  • 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)."

  • Francisco Casas added 1 commit

    added 1 commit

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

    Compare with previous version

  • Author Developer

    :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.

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

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

    • Author Developer

      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.

    • 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.

      Fair enough. [To be pedantic, we don't do that for sm4_src_from_constant_value(), but the precedent is set by sm4_src_from_node().]

    • Please register or sign in to reply
  • Francisco Casas added 1 commit

    added 1 commit

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

    Compare with previous version

  • Francisco Casas added 1 commit

    added 1 commit

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

    Compare with previous version

  • Elizabeth Figura approved this merge request

    approved this merge request

  • Henri Verbeet approved this merge request

    approved this merge request

  • Alexandre Julliard added 14 commits

    added 14 commits

    • e6ac5c3e...89eda518 - 13 commits from branch wine:master
    • ed9e236b - vkd3d-shader/tpf: Avoid reading constant value components beyond type's width (Valgrind).

    Compare with previous version

  • Alexandre Julliard approved this merge request

    approved this merge request

Please register or sign in to reply
Loading