[PATCH v3 0/1] MR129: tests: Read integer uniforms with strtol() and strtoul().
Because %i sscanf() converters are deprecated, and in practice clamp to [-2^31, 2^31) on 32 bit. -- v3: tests: Read integer uniforms with strtol() and strtoul(). https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/129
From: Giovanni Mascellani <gmascellani(a)codeweavers.com> Because %i sscanf() converters are deprecated, and in practice clamp to [-2^31, 2^31) on 32 bit. --- tests/d3d12.c | 5 --- tests/shader_runner.c | 74 +++++++++++++++++++++++++++++++++++++------ tests/utils.h | 5 +++ 3 files changed, 70 insertions(+), 14 deletions(-) diff --git a/tests/d3d12.c b/tests/d3d12.c index 405c9419..f8d2ec60 100644 --- a/tests/d3d12.c +++ b/tests/d3d12.c @@ -28,11 +28,6 @@ struct test_options test_options = {0}; static PFN_D3D12_CREATE_VERSIONED_ROOT_SIGNATURE_DESERIALIZER pfn_D3D12CreateVersionedRootSignatureDeserializer; static PFN_D3D12_SERIALIZE_VERSIONED_ROOT_SIGNATURE pfn_D3D12SerializeVersionedRootSignature; -struct ivec4 -{ - int x, y, z, w; -}; - struct dvec2 { double x, y; diff --git a/tests/shader_runner.c b/tests/shader_runner.c index 10c6dd21..3db9b82a 100644 --- a/tests/shader_runner.c +++ b/tests/shader_runner.c @@ -371,6 +371,58 @@ static void set_uniforms(struct shader_runner *runner, size_t offset, size_t cou memcpy(runner->uniforms + offset, uniforms, count * sizeof(*runner->uniforms)); } +static void read_int(const char **line, int *i) +{ + char *rest; + long val; + + errno = 0; + val = strtol(*line, &rest, 0); + + if (errno != 0) + fatal_error("Malformed int constant '%s'.\n", *line); + + *i = val; + if (*i != val) + fatal_error("Out of range int constant '%s'.\n", *line); + + *line = rest; +} + +static void read_uint(const char **line, unsigned int *u) +{ + char *rest; + unsigned long val; + + errno = 0; + val = strtoul(*line, &rest, 0); + + if (errno != 0) + fatal_error("Malformed uint constant '%s'.\n", *line); + + *u = val; + if (*u != val) + fatal_error("Out of range uint constant '%s'.\n", *line); + + *line = rest; +} + +static void read_int4(const char **line, struct ivec4 *v) +{ + read_int(line, &v->x); + read_int(line, &v->y); + read_int(line, &v->z); + read_int(line, &v->w); +} + +static void read_uint4(const char **line, struct uvec4 *v) +{ + read_uint(line, &v->x); + read_uint(line, &v->y); + read_uint(line, &v->z); + read_uint(line, &v->w); +} + static void parse_test_directive(struct shader_runner *runner, const char *line) { char *rest; @@ -590,28 +642,32 @@ static void parse_test_directive(struct shader_runner *runner, const char *line) fatal_error("Malformed float constant '%s'.\n", line); set_uniforms(runner, offset, 1, &f); } - else if (match_string(line, "int4", &line) || match_string(line, "uint4", &line)) + else if (match_string(line, "int4", &line)) + { + struct ivec4 v; + + read_int4(&line, &v); + set_uniforms(runner, offset, 4, &v); + } + else if (match_string(line, "uint4", &line)) { - int v[4]; + struct uvec4 v; - if (sscanf(line, "%i %i %i %i", &v[0], &v[1], &v[2], &v[3]) < 4) - fatal_error("Malformed (u)int4 constant '%s'.\n", line); - set_uniforms(runner, offset, 4, v); + read_uint4(&line, &v); + set_uniforms(runner, offset, 4, &v); } else if (match_string(line, "int", &line)) { int i; - if (sscanf(line, "%i", &i) < 1) - fatal_error("Malformed int constant '%s'.\n", line); + read_int(&line, &i); set_uniforms(runner, offset, 1, &i); } else if (match_string(line, "uint", &line)) { unsigned int u; - if (sscanf(line, "%u", &u) < 1) - fatal_error("Malformed uint constant '%s'.\n", line); + read_uint(&line, &u); set_uniforms(runner, offset, 1, &u); } else diff --git a/tests/utils.h b/tests/utils.h index 99f1d3a3..5fd7b086 100644 --- a/tests/utils.h +++ b/tests/utils.h @@ -35,6 +35,11 @@ struct vec4 float x, y, z, w; }; +struct ivec4 +{ + int x, y, z, w; +}; + struct uvec4 { unsigned int x, y, z, w; -- GitLab https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/129
On Mon Mar 20 15:53:23 2023 +0000, Henri Verbeet wrote:
```diff +static void read_int(const char **line, int *ptr) ```
I think "ptr" is a bit of an awkward name for this. Likewise for read_uint().
```diff +static void read_int4(const char **line, int (*v)[4]) ```
The "natural" type for "v" would seem struct ivec4. I imagine the choice of passing an array was largely dictated by the set_uniforms() interface, but perhaps it shouldn't be? Likewise for read_uint4(). Fixed. I guess we already assume that `struct ivec4` and `struct uvec4` will always be packed, right? More or less as we already assumed that `int[4]` is packed.
-- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/129#note_27301
Fixed. I guess we already assume that `struct ivec4` and `struct uvec4` will always be packed, right? More or less as we already assumed that `int[4]` is packed.
We do in practice assume a specific memory layout for these structures, yes. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/129#note_27302
I just noticed this, but:
```diff +static void read_int(const char **line, int *i) +{ + char *rest; + long val; + + errno = 0; + val = strtol(*line, &rest, 0); + + if (errno != 0) + fatal_error("Malformed int constant '%s'.\n", *line); + + *i = val; + if (*i != val) + fatal_error("Out of range int constant '%s'.\n", *line); ```
Here and to some extent above, I don't think we can just print "*line"; there may be additional data beyond "rest" that we'd also output. We may also want to make sure "*rest" is either '\0' or a whitespace character. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/129#note_27329
Here and to some extent above, I don't think we can just print "\*line"; there may be additional data beyond "rest" that we'd also output.
This is already happening in various other places. See for example how the `probe` command is parsed. I guess the point of view is that the shader runner is a developer-facing tool, so we prefer a simpler code even if it results in somewhat less polished error messages.
We may also want to make sure "\*rest" is either '\\0' or a whitespace character.
Ok, that makes sense. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/129#note_27407
participants (3)
-
Giovanni Mascellani -
Giovanni Mascellani (＠giomasce)
-
Henri Verbeet (＠hverbeet)