Skip to content
Snippets Groups Projects

vkd3d-shader: Break long SPIR-V assembly lines.

Merged navi requested to merge navi/vkd3d:shader-flush-log into master
3 unresolved threads

Avoid overflowing the output buffer when output is too long.

Same issue as !990 (merged), happens on cyberpunk 2077 when dumping SPIR-V asm. OpConstantComposite opcodes have so many operands they go over the 1020 bytes on wine's output buffer.

Merge request reports

Merge request pipeline #31268 skipped

Merge request pipeline skipped for 3d8fc1a4

Approval is optional

Merged by Henri VerbeetHenri Verbeet 7 months ago (Sep 3, 2024 3:18pm UTC)

Merge details

  • Changes merged into with 3d8fc1a4.
  • Deleted the source branch.

Activity

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

    added 1 commit

    • 91c3db37 - vkd3d-shader: Flush logs periodically.

    Compare with previous version

  • Giovanni Mascellani approved this merge request

    approved this merge request

    • That's a bit unfortunate, I think. Some thoughts:

      • Wine's logging callback choking on long lines is a Wine problem; vkd3d's implementation of the logging function (i.e., vfprintf()) doesn't have this problem, and the vkd3d API certainly doesn't specify a maximum line length for the logging callback. (Incidentally, there are other things it also doesn't specify, like which format specifiers it should accept, and that's also unfortunate.)
      • The SPIR-V disassembler should produce reasonable output. Unfortunately we rely on spirv-tools for this, so this is not completely under our own control. vkd3d_shader_trace_text_() is too late though; if we're going to fixup the SPIR-V disassembly, it should probably happen in vkd3d_spirv_binary_to_text().
      • I don't think SPIR-V assembly distinguishes much between different kinds of whitespace, so I think in principle something like this should be valid SPIR-V assembly:
      %17 = OpConstantComposite %1 %2 %3 %4
              %5 %6 %7 %8
              %9 %10 %11 %12
              %13 %14 %15 %16

      Not sure what the formal specification (is there one?) says though.

      • It's somewhat uncommon to have a Win32/PE build of vkd3d with spirv-tools support. As I imagine you've found out, it's a bit of a pain to setup, and Wine's bundled version of vkd3d doesn't include spirv-tools support either.
    • Author Contributor

      Wine's logging callback choking on long lines is a Wine problem

      yes, i would love to fix this issue there, since it would avoid those edge cases like this and !990 (merged), tho when i questioned on irc, the suggestion was to break up logging calls instead

      if we're going to fixup the SPIR-V disassembly, it should probably happen in vkd3d_spirv_binary_to_text().

      i can move this there, and do some logic to break it up neatly like in the example, altho it would still leave tracing open to overflows if somewhere else also prints something long one day (again tho, fixing this in wine would be so much better)

      It's somewhat uncommon to have a Win32/PE build of vkd3d with spirv-tools support. As I imagine you've found out, it's a bit of a pain to setup

      due to my setup it was actually quite painless, since gentoo supports crossbuild nativelly, i just built the package for spirv-tools with mingw and added it to WINEPATH

      Edited by navi
    • Please register or sign in to reply
  • Wine's logging callback choking on long lines is a Wine problem

    yes, i would love to fix this issue there, since it would avoid those edge cases like this and !990 (merged), tho when i questioned on irc, the suggestion was to break up logging calls instead

    I think that was more of an "as well" than an "instead".

    It's somewhat uncommon to have a Win32/PE build of vkd3d with spirv-tools support. As I imagine you've found out, it's a bit of a pain to setup

    due to my setup it was actually quite painless, since gentoo supports crossbuild nativelly, i just built the package for spirv-tools with mingw and added it to WINEPATH

    Ah, that helps. Generally distributions don't have that packaged though, so you tend to end up either building a crossbuild of spirv-tools yourself or downloading binaries from somewhere, and then fiddling with include and library paths.

  • navi added 1 commit

    added 1 commit

    • 99ca79a6 - vkd3d-shader: Flush logs periodically.

    Compare with previous version

  • Author Contributor

    i moved the splitting into vkd3d_spirv_binary_to_text, tho that required me to do an extra loop over the output with some code duplication.

    and instead of simply splitting and indicating with ..., now it looks for a space and replaces with a new line, which according to https://github.com/KhronosGroup/SPIRV-Tools/blob/main/docs/syntax.md, should still be valid SPIR-V ASM:

    A module is a sequence of instructions, separated by whitespace. An instruction is an opcode name followed by operands, separated by whitespace. Typically each instruction is presented on its own line, but the assembler does not enforce this rule.

  • Giovanni Mascellani
  • navi added 1 commit

    added 1 commit

    • 3f4988b2 - vkd3d-shader: Flush logs periodically.

    Compare with previous version

    • Subject: [PATCH] vkd3d-shader: Flush logs periodically.
      
      Avoid overflowing the output buffer when output is too long.

      I think that's not quite accurate for the current version.

      +                for (; q - p > 512; p += 512)
      +                {
      +                    char *truncate = memchr(p + 512, ' ', q - p - 512);
      +                    if (!truncate)
      +                        break;
      +                    *truncate = '\n';
      +                }

      It's not necessarily wrong, but 512 seems a bit longer than what I'd call "comfortable to read". Perhaps something closer to 100 makes more sense?

      Out of curiosity, did you happen to test whether spirv-as can correctly assemble the resulting output?

      The approach taken here has the advantage that it can be done relatively easily and in-place, and I think that's worth something. It does mean the continuation starts in the first column though, and that's perhaps not the most readable output we can produce either. Since we need to copy "text" anyway, how would you feel about replacing the vkd3d_malloc() and memcpy() with a vkd3d_string_buffer, and then adding some (8 space) indentation?

    • It's not necessarily wrong, but 512 seems a bit longer than what I'd call "comfortable to read". Perhaps something closer to 100 makes more sense?

      Out of curiosity, did you happen to test whether spirv-as can correctly assemble the resulting output?

      Notice that in practice most questions might have an easier answer by simply dumping the shaders and disassembling that.

    • Author Contributor

      Out of curiosity, did you happen to test whether spirv-as can correctly assemble the resulting output?

      i ran the game to get a shader dump that got truncated, removed the log prefix, run thru spirv-as and then spirv-dis, aside from using different temporary numbers for the SSA, seems to output the same thing

      The approach taken here has the advantage that it can be done relatively easily and in-place, and I think that's worth something. It does mean the continuation starts in the first column though, and that's perhaps not the most readable output we can produce either. Since we need to copy "text" anyway, how would you feel about replacing the vkd3d_malloc() and memcpy() with a vkd3d_string_buffer, and then adding some (8 space) indentation?

      that's a great suggestion. on it!

    • Author Contributor

      should be done

    • Please register or sign in to reply
  • navi added 1 commit

    added 1 commit

    • d7acb9b3 - vkd3d-shader: Break long SPIR-V assembly lines.

    Compare with previous version

  • navi added 1 commit

    added 1 commit

    • f9502b33 - vkd3d-shader: Break long SPIR-V assembly lines.

    Compare with previous version

  • navi changed title from vkd3d-shader: Flush logs periodically. to vkd3d-shader: Break long SPIR-V assembly lines.

    changed title from vkd3d-shader: Flush logs periodically. to vkd3d-shader: Break long SPIR-V assembly lines.

    • +        out->size = buffer.content_size;
      +        out->code = buffer.buffer;

      You'll want vkd3d_shader_code_from_string_buffer().

      +                vkd3d_string_buffer_printf(&buffer, pad ? "        %.*s\n" : "%.*s\n", (int)(truncate - p), p);

      It may be a bit nicer to do something like this:

      vkd3d_string_buffer_printf(&buffer, "%s%.*s\n", pad, (int)(truncate - p), p);

      and then make "pad" a pointer to a string containing the number of spaces to add.

    • Author Contributor

      done!

    • Please register or sign in to reply
  • navi added 1 commit

    added 1 commit

    • 8cecede4 - vkd3d-shader: Break long SPIR-V assembly lines.

    Compare with previous version

  • Henri Verbeet approved this merge request

    approved this merge request

  • Henri Verbeet added 18 commits

    added 18 commits

    Compare with previous version

  • Please register or sign in to reply
    Loading