vkd3d-shader: Break long SPIR-V assembly lines.
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
Activity
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.
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
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.
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.
- Resolved by navi
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.
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!
added 1 commit
- d7acb9b3 - vkd3d-shader: Break long SPIR-V assembly lines.
added 1 commit
- f9502b33 - 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.
added 1 commit
- 8cecede4 - vkd3d-shader: Break long SPIR-V assembly lines.
added 18 commits
-
8cecede4...a39227c7 - 17 commits from branch
wine:master
- 3d8fc1a4 - vkd3d-shader/spirv: Break long assembly lines.
-
8cecede4...a39227c7 - 17 commits from branch