[PATCH 0/3] MR582: vkd3d-dxbc: Add options to fix a DXBC shader checksum.
From: Giovanni Mascellani <gmascellani(a)codeweavers.com> --- include/vkd3d_shader.h | 19 ++++++++++++++++--- libs/vkd3d-shader/dxbc.c | 9 +++++---- 2 files changed, 21 insertions(+), 7 deletions(-) diff --git a/include/vkd3d_shader.h b/include/vkd3d_shader.h index a9c9ccc4a..26a4acecf 100644 --- a/include/vkd3d_shader.h +++ b/include/vkd3d_shader.h @@ -204,6 +204,20 @@ enum vkd3d_shader_compile_option_feature_flags VKD3D_FORCE_32_BIT_ENUM(VKD3D_SHADER_COMPILE_OPTION_FEATURE_FLAGS), }; +/** + * Flags for vkd3d_shader_parse_dxbc(). + * + * \since 1.11 + */ +enum vkd3d_shader_parse_dxbc_flags +{ + /** Ignore the checksum and continue parsing even if it is + * incorrect. */ + VKD3D_SHADER_PARSE_DXBC_IGNORE_CHECKSUM = 0x00000001, + + VKD3D_FORCE_32_BIT_ENUM(VKD3D_SHADER_PARSE_DXBC_FLAGS), +}; + enum vkd3d_shader_compile_option_name { /** @@ -2360,9 +2374,8 @@ VKD3D_SHADER_API void vkd3d_shader_free_dxbc(struct vkd3d_shader_dxbc_desc *dxbc * * \param dxbc A vkd3d_shader_code structure containing the DXBC blob to parse. * - * \param flags A set of flags modifying the behaviour of the function. No - * flags are defined for this version of vkd3d-shader, and this parameter - * should be set to 0. + * \param flags A combination of zero or more elements of enum + * vkd3d_shader_parse_dxbc_flags. * * \param desc A vkd3d_shader_dxbc_desc structure describing the contents of * the DXBC blob. Its vkd3d_shader_dxbc_section_desc structures will contain diff --git a/libs/vkd3d-shader/dxbc.c b/libs/vkd3d-shader/dxbc.c index edb65d2e9..8991d037f 100644 --- a/libs/vkd3d-shader/dxbc.c +++ b/libs/vkd3d-shader/dxbc.c @@ -150,7 +150,7 @@ static const char *shader_get_string(const char *data, size_t data_size, size_t } static int parse_dxbc(const struct vkd3d_shader_code *dxbc, struct vkd3d_shader_message_context *message_context, - const char *source_name, struct vkd3d_shader_dxbc_desc *desc) + const char *source_name, uint32_t flags, struct vkd3d_shader_dxbc_desc *desc) { const struct vkd3d_shader_location location = {.source_name = source_name}; struct vkd3d_shader_dxbc_section_desc *sections, *section; @@ -187,7 +187,8 @@ static int parse_dxbc(const struct vkd3d_shader_code *dxbc, struct vkd3d_shader_ checksum[2] = read_u32(&ptr); checksum[3] = read_u32(&ptr); vkd3d_compute_dxbc_checksum(data, data_size, calculated_checksum); - if (memcmp(checksum, calculated_checksum, sizeof(checksum))) + if (memcmp(checksum, calculated_checksum, sizeof(checksum)) + && !(flags & VKD3D_SHADER_PARSE_DXBC_IGNORE_CHECKSUM)) { WARN("Checksum {0x%08x, 0x%08x, 0x%08x, 0x%08x} does not match " "calculated checksum {0x%08x, 0x%08x, 0x%08x, 0x%08x}.\n", @@ -287,7 +288,7 @@ static int for_each_dxbc_section(const struct vkd3d_shader_code *dxbc, unsigned int i; int ret; - if ((ret = parse_dxbc(dxbc, message_context, source_name, &desc)) < 0) + if ((ret = parse_dxbc(dxbc, message_context, source_name, 0, &desc)) < 0) return ret; for (i = 0; i < desc.section_count; ++i) @@ -313,7 +314,7 @@ int vkd3d_shader_parse_dxbc(const struct vkd3d_shader_code *dxbc, *messages = NULL; vkd3d_shader_message_context_init(&message_context, VKD3D_SHADER_LOG_INFO); - ret = parse_dxbc(dxbc, &message_context, NULL, desc); + ret = parse_dxbc(dxbc, &message_context, NULL, flags, desc); vkd3d_shader_message_context_trace_messages(&message_context); if (!vkd3d_shader_message_context_copy_messages(&message_context, messages) && ret >= 0) -- GitLab https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/582
From: Giovanni Mascellani <gmascellani(a)codeweavers.com> --- programs/vkd3d-dxbc/main.c | 29 +++++++++++++++++++++-------- 1 file changed, 21 insertions(+), 8 deletions(-) diff --git a/programs/vkd3d-dxbc/main.c b/programs/vkd3d-dxbc/main.c index b6786b717..040c4a792 100644 --- a/programs/vkd3d-dxbc/main.c +++ b/programs/vkd3d-dxbc/main.c @@ -41,6 +41,7 @@ enum OPTION_LIST_DATA, OPTION_NO_COLOUR, OPTION_VERSION, + OPTION_IGNORE_CHECKSUM, }; struct options @@ -50,6 +51,7 @@ struct options bool list; bool list_data; bool print_version; + bool ignore_checksum; struct colours { @@ -86,13 +88,14 @@ static bool parse_command_line(int argc, char **argv, struct options *options) static struct option long_options[] = { - {"colour", no_argument, NULL, OPTION_COLOUR}, - {"help", no_argument, NULL, OPTION_HELP}, - {"list", no_argument, NULL, OPTION_LIST}, - {"list-data", no_argument, NULL, OPTION_LIST_DATA}, - {"no-colour", no_argument, NULL, OPTION_NO_COLOUR}, - {"version", no_argument, NULL, OPTION_VERSION}, - {NULL, 0, NULL, 0}, + {"colour", no_argument, NULL, OPTION_COLOUR}, + {"help", no_argument, NULL, OPTION_HELP}, + {"list", no_argument, NULL, OPTION_LIST}, + {"list-data", no_argument, NULL, OPTION_LIST_DATA}, + {"no-colour", no_argument, NULL, OPTION_NO_COLOUR}, + {"version", no_argument, NULL, OPTION_VERSION}, + {"ignore-checksum", no_argument, NULL, OPTION_IGNORE_CHECKSUM}, + {NULL, 0, NULL, 0}, }; static const struct colours colours = @@ -151,6 +154,10 @@ static bool parse_command_line(int argc, char **argv, struct options *options) options->print_version = true; return true; + case OPTION_IGNORE_CHECKSUM: + options->ignore_checksum = true; + break; + default: return false; } @@ -173,6 +180,7 @@ static void print_usage(const char *program_name) " --list-data List the data contained in the DXBC sections.\n" " --no-colour Disable colour, even when supported by the output.\n" " -V, --version Display version information and exit.\n" + " --ignore-checksum Do not validate the checksum when parsing the DXBC blob.\n" " -- Stop option processing. Any subsequent argument is\n" " interpreted as a filename.\n" "\n" @@ -376,6 +384,7 @@ int main(int argc, char **argv) struct options options; bool close_input; char *messages; + uint32_t flags; int fail = 1; FILE *input; int ret; @@ -409,7 +418,11 @@ int main(int argc, char **argv) goto done; } - ret = vkd3d_shader_parse_dxbc(&dxbc, 0, &dxbc_desc, &messages); + flags = 0; + if (options.ignore_checksum) + flags |= VKD3D_SHADER_PARSE_DXBC_IGNORE_CHECKSUM; + + ret = vkd3d_shader_parse_dxbc(&dxbc, flags, &dxbc_desc, &messages); if (messages) fputs(messages, stderr); vkd3d_shader_free_messages(messages); -- GitLab https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/582
From: Giovanni Mascellani <gmascellani(a)codeweavers.com> --- programs/vkd3d-dxbc/main.c | 80 ++++++++++++++++++++++++++++++++++++-- 1 file changed, 76 insertions(+), 4 deletions(-) diff --git a/programs/vkd3d-dxbc/main.c b/programs/vkd3d-dxbc/main.c index 040c4a792..87091ca32 100644 --- a/programs/vkd3d-dxbc/main.c +++ b/programs/vkd3d-dxbc/main.c @@ -42,11 +42,13 @@ enum OPTION_NO_COLOUR, OPTION_VERSION, OPTION_IGNORE_CHECKSUM, + OPTION_OUTPUT, }; struct options { const char *input_filename; + const char *output_filename; bool print_help; bool list; bool list_data; @@ -95,6 +97,7 @@ static bool parse_command_line(int argc, char **argv, struct options *options) {"no-colour", no_argument, NULL, OPTION_NO_COLOUR}, {"version", no_argument, NULL, OPTION_VERSION}, {"ignore-checksum", no_argument, NULL, OPTION_IGNORE_CHECKSUM}, + {"output", required_argument, NULL, OPTION_OUTPUT}, {NULL, 0, NULL, 0}, }; @@ -122,7 +125,7 @@ static bool parse_command_line(int argc, char **argv, struct options *options) for (;;) { - if ((option = getopt_long(argc, argv, "htV", long_options, NULL)) == -1) + if ((option = getopt_long(argc, argv, "htVo:", long_options, NULL)) == -1) break; switch (option) @@ -158,6 +161,11 @@ static bool parse_command_line(int argc, char **argv, struct options *options) options->ignore_checksum = true; break; + case OPTION_OUTPUT: + case 'o': + options->output_filename = optarg; + break; + default: return false; } @@ -181,6 +189,9 @@ static void print_usage(const char *program_name) " --no-colour Disable colour, even when supported by the output.\n" " -V, --version Display version information and exit.\n" " --ignore-checksum Do not validate the checksum when parsing the DXBC blob.\n" + " -o, --output=<file> Re-serialize the DXBC shader to <file>. This is useful\n" + " in combination with --ignore-checksum to fix bad or\n" + " missing checksums.\n" " -- Stop option processing. Any subsequent argument is\n" " interpreted as a filename.\n" "\n" @@ -264,6 +275,40 @@ static bool read_input(FILE *f, struct vkd3d_shader_code *dxbc) return true; } +static FILE *open_output(const char *filename, bool *close) +{ + FILE *f; + + *close = false; + + if (!strcmp(filename, "-")) + return stdout; + + if (!(f = fopen(filename, "wb"))) + { + fprintf(stderr, "Unable to open '%s' for writing.\n", filename); + return NULL; + } + + *close = true; + return f; +} + +static bool write_output(FILE *f, const struct vkd3d_shader_code *dxbc) +{ + const char *code = dxbc->code; + size_t pos = 0, ret; + + while (pos != dxbc->size) + { + if (!(ret = fwrite(&code[pos], 1, dxbc->size - pos, f))) + return false; + pos += ret; + } + + return true; +} + static const char *dump_tag(char *out, uint32_t tag) { unsigned int i; @@ -379,14 +424,14 @@ static void dump_dxbc(const struct vkd3d_shader_code *dxbc, const struct vkd3d_s int main(int argc, char **argv) { + struct vkd3d_shader_code dxbc, serialized; struct vkd3d_shader_dxbc_desc dxbc_desc; - struct vkd3d_shader_code dxbc; + bool close_input, close_output = false; struct options options; - bool close_input; + FILE *input, *output; char *messages; uint32_t flags; int fail = 1; - FILE *input; int ret; if (!parse_command_line(argc, argv, &options)) @@ -432,11 +477,38 @@ int main(int argc, char **argv) if (options.list || options.list_data) dump_dxbc(&dxbc, &dxbc_desc, &options); + if (options.output_filename) + { + ret = vkd3d_shader_serialize_dxbc(dxbc_desc.section_count, dxbc_desc.sections, &serialized, &messages); + if (messages) + fputs(messages, stderr); + vkd3d_shader_free_messages(messages); + if (ret < 0) + goto done; + + if (!(output = open_output(options.output_filename, &close_output))) + { + vkd3d_shader_free_shader_code(&serialized); + goto done; + } + + if (!write_output(output, &serialized)) + { + fprintf(stderr, "Failed to write output blob.\n"); + vkd3d_shader_free_shader_code(&serialized); + goto done; + } + + vkd3d_shader_free_shader_code(&serialized); + } + vkd3d_shader_free_dxbc(&dxbc_desc); vkd3d_shader_free_shader_code(&dxbc); fail = 0; done: if (close_input) fclose(input); + if (close_output) + fclose(output); return fail; } -- GitLab https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/582
This merge request was approved by Giovanni Mascellani. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/582
```diff - {"colour", no_argument, NULL, OPTION_COLOUR}, - {"help", no_argument, NULL, OPTION_HELP}, - {"list", no_argument, NULL, OPTION_LIST}, - {"list-data", no_argument, NULL, OPTION_LIST_DATA}, - {"no-colour", no_argument, NULL, OPTION_NO_COLOUR}, - {"version", no_argument, NULL, OPTION_VERSION}, - {NULL, 0, NULL, 0}, + {"colour", no_argument, NULL, OPTION_COLOUR}, + {"help", no_argument, NULL, OPTION_HELP}, + {"list", no_argument, NULL, OPTION_LIST}, + {"list-data", no_argument, NULL, OPTION_LIST_DATA}, + {"no-colour", no_argument, NULL, OPTION_NO_COLOUR}, + {"version", no_argument, NULL, OPTION_VERSION}, + {"ignore-checksum", no_argument, NULL, OPTION_IGNORE_CHECKSUM}, + {NULL, 0, NULL, 0}, ```
No big deal, but these were ordered before.
```diff @@ -173,6 +180,7 @@ static void print_usage(const char *program_name) " --list-data List the data contained in the DXBC sections.\n" " --no-colour Disable colour, even when supported by the output.\n" " -V, --version Display version information and exit.\n" + " --ignore-checksum Do not validate the checksum when parsing the DXBC blob.\n" " -- Stop option processing. Any subsequent argument is\n" " interpreted as a filename.\n" "\n" ```
Likewise.
```diff - ret = vkd3d_shader_parse_dxbc(&dxbc, 0, &dxbc_desc, &messages); + flags = 0; + if (options.ignore_checksum) + flags |= VKD3D_SHADER_PARSE_DXBC_IGNORE_CHECKSUM; + + ret = vkd3d_shader_parse_dxbc(&dxbc, flags, &dxbc_desc, &messages); ```
There may be some value in trying without VKD3D_SHADER_PARSE_DXBC_IGNORE_CHECKSUM first, to determine whether the checksum was valid or not.
```diff + " -o, --output=<file> Re-serialize the DXBC shader to <file>. This is useful\n" + " in combination with --ignore-checksum to fix bad or\n" + " missing checksums.\n" ```
I'm a little worried about painting ourselves into a corner with this. In particular, there are a couple of features I'd like to add in the future: - Updating the contents of DXBC sections. - Appending DXBC sections. - Extracting the contents of DXBC sections. That last feature in particular potentially implies having multiple output files. You could e.g. imagine syntax like this: ```bash # Extract the ISGN, OSGN, and SHDR sections to the corresponding files. vkd3d-dxbc -o shader.ISGN -x t:ISGN -o shader.OSGN -x t:OSGN -o shader.SHDR -x t:SHDR shader.dxbc # Extract the SHEX section, pipe to hexdump. vkd3d-dxbc -x t:SHEX shader.dxbc | hexdump -C # Update the SHDR section from shader.SHDR, write the new DXBC to shader2.dxbc. vkd3d-dxbc -i shader.SHDR -u t:SHDR -o shader2.dxbc shader.dxbc # Update the SHDR section from shader.SHDR, pipe to xxd. vkd3d-dxbc -u t:SHDR shader.dxbc < shader.SHDR | xxd -i ``` I.e., -i specifies the input file for subsequent operations, -o specifies the output file for subsequent operations, when unspecified stdin/stdout is used, and there's an implied "write output if modified" at the end. The question then becomes how rewriting the checksum fits in. One option would be to make that an explicit operation. E.g.: ```bash vkd3d-dxbc --ignore-checksum --recalculate-checksum shader.dxbc > shader2.dxbc vkd3d-dxbc --ignore-checksum -o shader2.dxbc --recalculate-checksum shader.dxbc ``` Thoughts? -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/582#note_58449
There may be some value in trying without VKD3D_SHADER_PARSE_DXBC_IGNORE_CHECKSUM first, to determine whether the checksum was valid or not.
And what would happen depending on whether it's valid or not?
I'm a little worried about painting ourselves into a corner with this. In particular, there are a couple of features I'd like to add in the future:
* Updating the contents of DXBC sections. * Appending DXBC sections. * Extracting the contents of DXBC sections.
That last feature in particular potentially implies having multiple output files. You could e.g. imagine syntax like this:
```shell # Extract the ISGN, OSGN, and SHDR sections to the corresponding files. vkd3d-dxbc -o shader.ISGN -x t:ISGN -o shader.OSGN -x t:OSGN -o shader.SHDR -x t:SHDR shader.dxbc # Extract the SHEX section, pipe to hexdump. vkd3d-dxbc -x t:SHEX shader.dxbc | hexdump -C # Update the SHDR section from shader.SHDR, write the new DXBC to shader2.dxbc. vkd3d-dxbc -i shader.SHDR -u t:SHDR -o shader2.dxbc shader.dxbc # Update the SHDR section from shader.SHDR, pipe to xxd. vkd3d-dxbc -u t:SHDR shader.dxbc < shader.SHDR | xxd -i ```
I.e., -i specifies the input file for subsequent operations, -o specifies the output file for subsequent operations, when unspecified stdin/stdout is used, and there's an implied "write output if modified" at the end. The question then becomes how rewriting the checksum fits in. One option would be to make that an explicit operation. E.g.:
```shell vkd3d-dxbc --ignore-checksum --recalculate-checksum shader.dxbc > shader2.dxbc vkd3d-dxbc --ignore-checksum -o shader2.dxbc --recalculate-checksum shader.dxbc ```
Thoughts?
* I don't completely understand how you perform input: you seem to have the concept of a "main input" (using the positional argument at the end), which is interpreted as the beginning of a pipeline on which a few filters operate, possibly using auxiliary inputs (using `-o`). Is that the intention? The main input is allowed to be the stdin too? * You seem the consider `-o` an option (changing the command line parsing state but not performing actions themselves) and the various filter (`-x`, `-u`) as the actions, i.e., points at which the program does something for real. Maybe it might be more expressive to reverse the point of view? I.e., `-x` and `-u` merely configure a filter on the pipeline (with the possibility to chain many, or zero) and `-o` takes the current pipeline state, executes it and writes to the specified file. My proposal rather fits this model (except that no filter is currently defined, i.e., you can only write the same shader you just read), which looks more flexible to me. For example you can chain or otherwise compose filters and operators, which seems harder if filters immediately trigger an action. * `--recalculate-checksum` doesn't sound right to me: in my model the checksum is recalculated merely by virtue of the fact that each time we emit a DXBC file we put the right checksum. There is no explicit action to do so. To me `vkd3d-dxbc --ignore-checksum -o shader2.dxbc shader.dxbc` means: "Read `shader.dxbc` and rewrite it to `shader2.dxbc` without changing nothing, and without caring if when reading the checksum is not correct". The write will naturally put the correct checksum, because it's the only thing it's able to do. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/582#note_58489
There may be some value in trying without VKD3D_SHADER_PARSE_DXBC_IGNORE_CHECKSUM first, to determine whether the checksum was valid or not.
And what would happen depending on whether it's valid or not?
We could print a "(valid)" or "(invalid)" next to the checksum in the --list output, I guess.
* I don't completely understand how you perform input: you seem to have the concept of a "main input" (using the positional argument at the end), which is interpreted as the beginning of a pipeline on which a few filters operate, possibly using auxiliary inputs (using `-o`). Is that the intention? The main input is allowed to be the stdin too?
I'm not sure pipeline/filters is the quite the right model, but essentially, yes. The current input/output start out as stdin/stdout, and -i/-o change them. Add/update/extract etc. just use the current input output file, and after parsing the command line we'd have a list like this: ```c struct { enum operation op; const char *section; const char *file; } operations[] = { {EXTRACT, "t:SHDR", "shader.SHDR"}, {UPDATE, "t:ISGN", "shader.ISGN"}, {DELETE, "t:SHEX", NULL}, {ADD, "t:OSGN", "shader.OSGN"}, }; ```
* You seem the consider `-o` an option (changing the command line parsing state but not performing actions themselves) and the various filter (`-x`, `-u`) as the actions, i.e., points at which the program does something for real. Maybe it might be more expressive to reverse the point of view? I.e., `-x` and `-u` merely configure a filter on the pipeline (with the possibility to chain many, or zero) and `-o` takes the current pipeline state, executes it and writes to the specified file. My proposal rather fits this model (except that no filter is currently defined, i.e., you can only write the same shader you just read), which looks more flexible to me. For example you can chain or otherwise compose filters and operators, which seems harder if filters immediately trigger an action.
I think you're essentially arguing for something like ```bash # Extract the SHDR section to shader.SHDR vkd3d-dxbc -x t:SHDR -o shader.SHDR shader.dxbc # Update the SHDR section from shader.SHDR, write the new DXBC to shader2.dxbc. vkd3d-dxbc -i shader.SHDR -u t:SHDR -o shader2.dxbc shader.dxbc ``` right? I don't necessarily have an issue with that, but I guess the question then becomes what e.g. these do: ```bash vkd3d-dxbc -i shader.SHDR -u t:SHDR -i shader.ISGN -u t:SHDR -o shader2.dxbc shader.dxbc vkd3d-dxbc -i shader.SHDR -u t:SHDR -i shader.ISGN -u t:ISGN -x t:ISGN -o shader2.dxbc shader.dxbc vkd3d-dxbc -i shader.SHDR -u t:SHDR -i shader.ISGN -o shader2.dxbc -u t:ISGN -x t:ISGN shader.dxbc vkd3d-dxbc -x t:ISGN -o shader.ISGN -x t:OSGN -o shader.OSGN shader.dxbc vkd3d-dxbc -o shader.txt shader.dxbc vkd3d-dxbc shader.dxbc > shader.txt ``` and whether that's better than the alternative. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/582#note_58657
participants (3)
-
Giovanni Mascellani -
Giovanni Mascellani (＠giomasce)
-
Henri Verbeet (＠hverbeet)