[PATCH 0/1] MR305: Draft: vkd3d-shader/hlsl: Implement strings
Strings are opaque objects in HLSL. This implements recognizing string literals and the `string` type. This should fix the bug marked below but I'm not sure how to get my hands on a copy of Lego Star Wars Saga to test it properly. This is a super-duper draft MR; I'm certain there's problems with it. I do at least have a passing test, though. Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=50382 -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/305
From: Petrichor Park <ppark(a)codeweavers.com> Strings are opaque objects in HLSL. This implements recognizing string literals and the `string` type. Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=50382 --- Makefile.am | 1 + libs/vkd3d-shader/hlsl.c | 9 ++++++++- libs/vkd3d-shader/hlsl.h | 5 +++-- libs/vkd3d-shader/hlsl.l | 4 +++- libs/vkd3d-shader/hlsl.y | 12 ++++++++++++ tests/hlsl/strings.shader_test | 11 +++++++++++ 6 files changed, 38 insertions(+), 4 deletions(-) create mode 100644 tests/hlsl/strings.shader_test diff --git a/Makefile.am b/Makefile.am index 27738f3bf..7ebcd485a 100644 --- a/Makefile.am +++ b/Makefile.am @@ -159,6 +159,7 @@ vkd3d_shader_tests = \ tests/hlsl/static-initializer.shader_test \ tests/hlsl/step.shader_test \ tests/hlsl/storage-qualifiers.shader_test \ + tests/hlsl/strings.shader_test \ tests/hlsl/struct-array.shader_test \ tests/hlsl/struct-assignment.shader_test \ tests/hlsl/struct-semantics.shader_test \ diff --git a/libs/vkd3d-shader/hlsl.c b/libs/vkd3d-shader/hlsl.c index 8b706e1e6..290d087f2 100644 --- a/libs/vkd3d-shader/hlsl.c +++ b/libs/vkd3d-shader/hlsl.c @@ -1229,6 +1229,13 @@ struct hlsl_ir_node *hlsl_new_bool_constant(struct hlsl_ctx *ctx, bool b, const return hlsl_new_constant(ctx, hlsl_get_scalar_type(ctx, HLSL_TYPE_BOOL), &value, loc); } +/* Strings are opaque objects, so don't put anything in the constant. */ +struct hlsl_ir_node *hlsl_new_string_constant(struct hlsl_ctx *ctx, const struct vkd3d_shader_location *loc) +{ + struct hlsl_constant_value value; + return hlsl_new_constant(ctx, hlsl_get_scalar_type(ctx, HLSL_TYPE_STRING), &value, loc); +} + struct hlsl_ir_node *hlsl_new_float_constant(struct hlsl_ctx *ctx, float f, const struct vkd3d_shader_location *loc) { @@ -3159,6 +3166,7 @@ static void declare_predefined_types(struct hlsl_ctx *ctx) "int", "uint", "bool", + "string", }; char name[15]; @@ -3189,7 +3197,6 @@ static void declare_predefined_types(struct hlsl_ctx *ctx) {"float", HLSL_CLASS_SCALAR, HLSL_TYPE_FLOAT, 1, 1}, {"vector", HLSL_CLASS_VECTOR, HLSL_TYPE_FLOAT, 4, 1}, {"matrix", HLSL_CLASS_MATRIX, HLSL_TYPE_FLOAT, 4, 4}, - {"STRING", HLSL_CLASS_OBJECT, HLSL_TYPE_STRING, 1, 1}, {"TEXTURE", HLSL_CLASS_OBJECT, HLSL_TYPE_TEXTURE, 1, 1}, {"PIXELSHADER", HLSL_CLASS_OBJECT, HLSL_TYPE_PIXELSHADER, 1, 1}, {"VERTEXSHADER", HLSL_CLASS_OBJECT, HLSL_TYPE_VERTEXSHADER, 1, 1}, diff --git a/libs/vkd3d-shader/hlsl.h b/libs/vkd3d-shader/hlsl.h index e78602e8d..ccfe20253 100644 --- a/libs/vkd3d-shader/hlsl.h +++ b/libs/vkd3d-shader/hlsl.h @@ -92,13 +92,13 @@ enum hlsl_base_type HLSL_TYPE_INT, HLSL_TYPE_UINT, HLSL_TYPE_BOOL, - HLSL_TYPE_LAST_SCALAR = HLSL_TYPE_BOOL, + HLSL_TYPE_STRING, + HLSL_TYPE_LAST_SCALAR = HLSL_TYPE_STRING, HLSL_TYPE_SAMPLER, HLSL_TYPE_TEXTURE, HLSL_TYPE_UAV, HLSL_TYPE_PIXELSHADER, HLSL_TYPE_VERTEXSHADER, - HLSL_TYPE_STRING, HLSL_TYPE_VOID, }; @@ -1158,6 +1158,7 @@ struct hlsl_ir_node *hlsl_new_store_index(struct hlsl_ctx *ctx, const struct hls struct hlsl_ir_node *idx, struct hlsl_ir_node *rhs, unsigned int writemask, const struct vkd3d_shader_location *loc); bool hlsl_new_store_component(struct hlsl_ctx *ctx, struct hlsl_block *block, const struct hlsl_deref *lhs, unsigned int comp, struct hlsl_ir_node *rhs); +struct hlsl_ir_node *hlsl_new_string_constant(struct hlsl_ctx *ctx, const struct vkd3d_shader_location *loc); bool hlsl_index_is_noncontiguous(struct hlsl_ir_index *index); bool hlsl_index_is_resource_access(struct hlsl_ir_index *index); diff --git a/libs/vkd3d-shader/hlsl.l b/libs/vkd3d-shader/hlsl.l index e9ae3ccf3..c8014fc68 100644 --- a/libs/vkd3d-shader/hlsl.l +++ b/libs/vkd3d-shader/hlsl.l @@ -121,7 +121,6 @@ shared {return KW_SHARED; } stateblock {return KW_STATEBLOCK; } stateblock_state {return KW_STATEBLOCK_STATE; } static {return KW_STATIC; } -string {return KW_STRING; } struct {return KW_STRUCT; } switch {return KW_SWITCH; } tbuffer {return KW_TBUFFER; } @@ -215,6 +214,9 @@ row_major {return KW_ROW_MAJOR; } yylval->intval = vkd3d_parse_integer(yytext); return C_INTEGER; } +{STRING} { + return STRING; + } {DOUBLESLASHCOMMENT} {} diff --git a/libs/vkd3d-shader/hlsl.y b/libs/vkd3d-shader/hlsl.y index 0d9946731..6601bbf19 100644 --- a/libs/vkd3d-shader/hlsl.y +++ b/libs/vkd3d-shader/hlsl.y @@ -4589,6 +4589,7 @@ static void validate_texture_format_type(struct hlsl_ctx *ctx, struct hlsl_type FLOAT floatval; bool boolval; char *name; + void *string; /* Always null, as strings are opaque objects in HLSL */ DWORD modifiers; struct hlsl_ir_node *instr; struct hlsl_block *block; @@ -6225,6 +6226,17 @@ primary_expr: YYABORT; } } + | STRING + { + struct hlsl_ir_node *c; + if (!(c = hlsl_new_string_constant(ctx, &@1))) + YYABORT; + if (!($$ = make_block(ctx, c))) + { + hlsl_free_instr(c); + YYABORT; + } + } | VAR_IDENTIFIER { struct hlsl_ir_load *load; diff --git a/tests/hlsl/strings.shader_test b/tests/hlsl/strings.shader_test new file mode 100644 index 000000000..d7b778d61 --- /dev/null +++ b/tests/hlsl/strings.shader_test @@ -0,0 +1,11 @@ +[pixel shader] +float4 main(uniform float4 x) : sv_target +{ + string description = "Hello, VKD3D!"; + return float4(0.0, 0.0, 0.0, 0.0); +} + +[test] +uniform 0 float4 0.0 0.0 0.0 0.0 +draw quad +probe all rgba (0.0, 0.0, 0.0, 0.0) -- GitLab https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/305
Zebediah Figura (@zfigura) commented about libs/vkd3d-shader/hlsl.c:
{"float", HLSL_CLASS_SCALAR, HLSL_TYPE_FLOAT, 1, 1}, {"vector", HLSL_CLASS_VECTOR, HLSL_TYPE_FLOAT, 4, 1}, {"matrix", HLSL_CLASS_MATRIX, HLSL_TYPE_FLOAT, 4, 4}, - {"STRING", HLSL_CLASS_OBJECT, HLSL_TYPE_STRING, 1, 1},
Why remove this? -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/305#note_42189
Zebediah Figura (@zfigura) commented about libs/vkd3d-shader/hlsl.h:
HLSL_TYPE_INT, HLSL_TYPE_UINT, HLSL_TYPE_BOOL, - HLSL_TYPE_LAST_SCALAR = HLSL_TYPE_BOOL, + HLSL_TYPE_STRING, + HLSL_TYPE_LAST_SCALAR = HLSL_TYPE_STRING, HLSL_TYPE_SAMPLER, HLSL_TYPE_TEXTURE, HLSL_TYPE_UAV, HLSL_TYPE_PIXELSHADER, HLSL_TYPE_VERTEXSHADER, - HLSL_TYPE_STRING,
Strings aren't numeric types, are they? Why was this change made? -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/305#note_42190
Zebediah Figura (@zfigura) commented about libs/vkd3d-shader/hlsl.l:
stateblock {return KW_STATEBLOCK; } stateblock_state {return KW_STATEBLOCK_STATE; } static {return KW_STATIC; } -string {return KW_STRING; } If "string" is a keyword (can't be a variable name) then this needs to stay.
-- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/305#note_42191
Zebediah Figura (@zfigura) commented about libs/vkd3d-shader/hlsl.l:
yylval->intval = vkd3d_parse_integer(yytext); return C_INTEGER; } +{STRING} { + return STRING; + }
Our current STRING pattern is probably wrong for C strings; it forbids any backslashes inside the string. The preprocessor has a pattern which is probably more correct. Adding an extra test or two here would not be a bad idea. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/305#note_42192
Zebediah Figura (@zfigura) commented about libs/vkd3d-shader/hlsl.y:
FLOAT floatval; bool boolval; char *name; + void *string; /* Always null, as strings are opaque objects in HLSL */
This doesn't do anything; no rules or tokens return a value of this type. (This is governed by the %type and %token directives.) -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/305#note_42193
Zebediah Figura (@zfigura) commented about libs/vkd3d-shader/hlsl.y:
YYABORT; } } + | STRING + { + struct hlsl_ir_node *c; + if (!(c = hlsl_new_string_constant(ctx, &@1))) + YYABORT; + if (!($$ = make_block(ctx, c))) + { + hlsl_free_instr(c); + YYABORT; + } + }
This leaks $1. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/305#note_42194
On Mon Aug 14 17:42:55 2023 +0000, Zebediah Figura wrote:
Strings aren't numeric types, are they? Why was this change made? Strings count as scalars.
https://learn.microsoft.com/en-us/windows/win32/direct3dhlsl/dx-graphics-hls... -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/305#note_42198
On Mon Aug 14 17:42:55 2023 +0000, Zebediah Figura wrote:
If "string" is a keyword (can't be a variable name) then this needs to stay. I believe this is handled by the `type_no_void: TYPE_IDENTIFIER` in the yacc file, line 5644. There also aren't keywords for other inbuilt types (like `float`).
-- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/305#note_42199
Francisco Casas (@fcasas) commented about tests/hlsl/strings.shader_test:
+[pixel shader] +float4 main(uniform float4 x) : sv_target +{ + string description = "Hello, VKD3D!"; + return float4(0.0, 0.0, 0.0, 0.0); We use 4 spaces instead of tabs for indentation.
-- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/305#note_42200
Francisco Casas (@fcasas) commented about tests/hlsl/strings.shader_test:
+[pixel shader] +float4 main(uniform float4 x) : sv_target Specifying a uniform doesn't seem necessary for this test.
But if we want to test that its value it is not corrupted for some reason, then we should return it (and probably give it a different value than (0, 0 , 0 ,0). -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/305#note_42201
Francisco Casas (@fcasas) commented about libs/vkd3d-shader/hlsl.c:
return hlsl_new_constant(ctx, hlsl_get_scalar_type(ctx, HLSL_TYPE_BOOL), &value, loc); }
+/* Strings are opaque objects, so don't put anything in the constant. */ +struct hlsl_ir_node *hlsl_new_string_constant(struct hlsl_ctx *ctx, const struct vkd3d_shader_location *loc) +{ + struct hlsl_constant_value value; + return hlsl_new_constant(ctx, hlsl_get_scalar_type(ctx, HLSL_TYPE_STRING), &value, loc); +}
This gives a compiler warning: ``` vkd3d/libs/vkd3d-shader/hlsl.c: In function ‘hlsl_new_string_constant’: vkd3d/libs/vkd3d-shader/hlsl.c:1236:12: warning: ‘value’ may be used uninitialized [-Wmaybe-uninitialized] 1236 | return hlsl_new_constant(ctx, hlsl_get_scalar_type(ctx, HLSL_TYPE_STRING), &value, loc); | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ vkd3d/libs/vkd3d-shader/hlsl.c:1208:22: note: by argument 3 of type ‘const struct hlsl_constant_value *’ to ‘hlsl_new_constant’ declared here 1208 | struct hlsl_ir_node *hlsl_new_constant(struct hlsl_ctx *ctx, struct hlsl_type *type, | ^~~~~~~~~~~~~~~~~ vkd3d/libs/vkd3d-shader/hlsl.c:1235:32: note: ‘value’ declared here 1235 | struct hlsl_constant_value value; ``` My guess is that you want to initialize value to just zeroes. ```c struct hlsl_constant_value value = {}; ``` -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/305#note_42202
After this commit, when running the tests with, say `make check -j12 `, some `tests/hlsl/attributes.shader_test` tests start passing, so we have to remove those `todo`s. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/305#note_42203
On Mon Aug 14 18:12:10 2023 +0000, Francisco Casas wrote:
This gives a compiler warning: ``` vkd3d/libs/vkd3d-shader/hlsl.c: In function ‘hlsl_new_string_constant’: vkd3d/libs/vkd3d-shader/hlsl.c:1236:12: warning: ‘value’ may be used uninitialized [-Wmaybe-uninitialized] 1236 | return hlsl_new_constant(ctx, hlsl_get_scalar_type(ctx, HLSL_TYPE_STRING), &value, loc); | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ vkd3d/libs/vkd3d-shader/hlsl.c:1208:22: note: by argument 3 of type ‘const struct hlsl_constant_value *’ to ‘hlsl_new_constant’ declared here 1208 | struct hlsl_ir_node *hlsl_new_constant(struct hlsl_ctx *ctx, struct hlsl_type *type, | ^~~~~~~~~~~~~~~~~ vkd3d/libs/vkd3d-shader/hlsl.c:1235:32: note: ‘value’ declared here 1235 | struct hlsl_constant_value value; ``` My guess is that you want to initialize value to just zeroes. ```c struct hlsl_constant_value value = {}; ``` Huh it doesn't give me that error on my machine. Good catch.
-- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/305#note_42262
On Mon Aug 14 18:06:17 2023 +0000, Zebediah Figura wrote:
Our current STRING pattern is probably wrong for C strings; it forbids any backslashes inside the string. The preprocessor has a pattern which is probably more correct. Adding an extra test or two here would not be a bad idea. For STRING I see this on line 57:
`STRING \"[^\"]*\"` Am I misreading? -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/305#note_42263
On Mon Aug 14 18:04:34 2023 +0000, Petrichor Park wrote:
Strings count as scalars. https://learn.microsoft.com/en-us/windows/win32/direct3dhlsl/dx-graphics-hls... MSDN documentation notwithstanding, they don't fall into the same category as everything else there (i.e., they're not a numeric type, and can't form vectors and matrices). Arguably we should rename HLSL_TYPE_LAST_SCALAR to HLSL_TYPE_LAST_NUMERIC, but then what we really should be doing is to split this enumeration into two separate enumerations. That's been on the list for a while, though, so for now I'd just leave the enumeration as-is.
-- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/305#note_42407
On Mon Aug 14 18:05:49 2023 +0000, Petrichor Park wrote:
I believe this is handled by the `type_no_void: TYPE_IDENTIFIER` in the yacc file, line 5644. There also aren't keywords for other inbuilt types (like `float`). The point is that in both HLSL and C, some words (i.e. keywords) aren't legal variable identifiers. In C this includes "float"; in HLSL it does not. This kind of change treats "string" as no longer a keyword. If that's actually the case, it needs tests to justify. If not, we can't handle it using TYPE_IDENTIFIER; we'll need to add a specific rule for it.
-- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/305#note_42408
On Mon Aug 14 19:21:05 2023 +0000, Petrichor Park wrote:
For STRING I see this on line 57: `STRING \"[^\"]*\"` Am I misreading? Right, but you'll notice that regular expression simply refuses to parse, say, "a\"b", or even "a\nb". For # line directives I think that's actually correct, but it's probably wrong for string literals.
-- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/305#note_42409
On Tue Aug 15 23:20:10 2023 +0000, Zebediah Figura wrote:
MSDN documentation notwithstanding, they don't fall into the same category as everything else there (i.e., they're not a numeric type, and can't form vectors and matrices). Arguably we should rename HLSL_TYPE_LAST_SCALAR to HLSL_TYPE_LAST_NUMERIC, but then what we really should be doing is to split this enumeration into two separate enumerations. That's been on the list for a while, though, so for now I'd just leave the enumeration as-is. I'd need to specially splice the STRING type into the place where the types are registered then ... is that OK?
-- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/305#note_42494
On Tue Aug 15 23:20:11 2023 +0000, Zebediah Figura wrote:
Right, but you'll notice that regular expression simply refuses to parse, say, "a\"b", or even "a\nb". For # line directives I think that's actually correct, but it's probably wrong for string literals. OH, I read `[^\"]` as escaping the inner quotation mark. Good catch.
-- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/305#note_42495
I'd need to specially splice the STRING type into the place where the types are registered then ... is that OK?
Yes, that seems like the right architecture. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/305#note_42561
On Mon Aug 14 18:06:29 2023 +0000, Zebediah Figura wrote:
This leaks $1. Does this ignore string value itself? We'll need to keep it for effects variables, or at least for annotations.
-- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/305#note_48773
On Mon Oct 16 09:46:08 2023 +0000, Nikolay Sivov wrote:
Does this ignore string value itself? We'll need to keep it for effects variables, or at least for annotations. It does, but that can be stored later, when we're going to use it.
-- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/305#note_48826
Any way we can get this worked on? I don't think it makes sense to have strings as scalars also because effects framework defines them as objects. And yes, we'll need to store the string itself somewhere. For practical purposes this should probably share some structure with how other initial values are stored. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/305#note_60826
participants (5)
-
Francisco Casas (＠fcasas) -
Nikolay Sivov (＠nsivov)
-
Petrichor Park
-
Petrichor Park (＠petrathekat)
-
Zebediah Figura (＠zfigura)