
<!DOCTYPE html><html><head><meta http-equiv="Content-Type" content="text/html; charset=utf-8" /></head><body><div data-html-editor-font-wrapper="true" style="font-family: arial, sans-serif; font-size: 13px;"><br><br>April 28, 2022 4:22 PM, "Zebediah Figura" <<a target="_blank" rel="noopener noreferrer" href="mailto:zfigura@codeweavers.com">zfigura@codeweavers.com</a>> wrote:<br> <blockquote>On 4/28/22 14:45, Francisco Casas wrote:<br> <blockquote>diff --git a/libs/vkd3d-shader/hlsl.y b/libs/vkd3d-shader/hlsl.y<br>index 905dbfc5..e7fe74d8 100644<br>--- a/libs/vkd3d-shader/hlsl.y<br>+++ b/libs/vkd3d-shader/hlsl.y<br>@@ -1606,7 +1606,27 @@ static struct list *declare_vars(struct hlsl_ctx *ctx, struct hlsl_type<br>*basic_t<br>type = basic_type;<br>for (i = 0; i < v->arrays.count; ++i)<br>+ {<br>+ if (v->arrays.sizes[i] == HLSL_ARRAY_ELEMENTS_COUNT_IMPLICIT)<br>+ {<br>+ unsigned int size = initializer_size(&v->initializer);<br>+ unsigned int elem_components = hlsl_type_component_count(type);<br>+<br>+ assert(v->initializer.args_count);<br>+<br>+ v->arrays.sizes[i] = (size + elem_components - 1)/elem_components;<br>+<br>+ if (size % elem_components != 0)<br>+ {<br>+ hlsl_error(ctx, &v->loc, VKD3D_SHADER_ERROR_HLSL_WRONG_PARAMETER_COUNT,<br>+ "Cannot initialize implicit array with %u components, expected a multiple of %u.",<br>+ size, elem_components);<br>+ free_parse_initializer(&v->initializer);<br>+ v->initializer.args_count = 0;<br>+ }</blockquote><br>This doesn't seem like it'll do the right thing for implicit sizes on an inner array, especially<br>considering that the right thing is probably to fail compilation.<br><br>It also won't do the right thing for initializers without braces (viz. also fail compilation).<blockquote>These cases are checked in the parse rules introduced in this patch.</blockquote><br><br>Oh, I see, I wasn't actually correctly reading the parser rule. That makes sense.<br><br>On the other hand, handling IMPLICIT inside of the loop, without any checks, was one of the things<br>that made me think "we're not handling inner arrays correctly", so arguably something deserves<br>changing here :-)</blockquote><br>Well, the structure of the parsing rule ensures that only the most external array could have<br>IMPLICIT side. This is related to the<br>assert(i == v->arrays.count - 1);<br>suggested by Giovanni.<br> <blockquote>I don't see any handling for braceless initializers in this patch, though; am I missing something?</blockquote><br>The error is added to the "variable_def:" rule.<br>But then again, maybe it is not so clear that the parse rule ensures that only the most external array<br>can have IMPLICIT size. I could add the same assertion here too.<br><br>By the way, I am not sure if you changed your stance on checking the implicit size arrays at the parse<br>level. I would prefer to keep it there... if I find a way of also handling these unbounded resource<br>arrays nicely.<br> <blockquote><blockquote>I think we need tests for all of these corner cases, and also a test for missing initializers.</blockquote> <blockquote>Okay, adding them.<br>+ }<br>type = hlsl_new_array_type(ctx, type, v->arrays.sizes[i]);<br>+ }<br>vkd3d_free(v->arrays.sizes);<br>if (type->type != HLSL_CLASS_MATRIX)<br>@@ -2464,6 +2484,7 @@ static bool add_method_call(struct hlsl_ctx *ctx, struct list *instrs, struct<br>hl<br>%token <name> TYPE_IDENTIFIER<br>%type <arrays> arrays<br>+%type <arrays> implicit_arrays<br>%type <assign_op> assign_op<br>@@ -3108,7 +3129,7 @@ variables_def:<br>}<br>variable_decl:<br>- any_identifier arrays colon_attribute<br>+ any_identifier implicit_arrays colon_attribute<br>{<br>$$ = hlsl_alloc(ctx, sizeof(*$$));<br>$$->loc = @1;<br>@@ -3137,6 +3158,15 @@ state_block:<br>variable_def:<br>variable_decl<br>+ {<br>+ if ($$->arrays.sizes && $$->arrays.sizes[$$->arrays.count - 1] ==<br>HLSL_ARRAY_ELEMENTS_COUNT_IMPLICIT)<br>+ {<br>+ hlsl_error(ctx, &@1, VKD3D_SHADER_ERROR_HLSL_MISSING_INITIALIZER,<br>+ "Implicit array requires initializer.");<br>+ free_parse_variable_def($$);<br>+ YYABORT;<br>+ }<br>+ }</blockquote><br>This won't work for unbounded resource arrays, which can be declared with an empty pair of brackets<br>(and no initializer).<blockquote>I see, I didn't knew about those.<br>It also aborts compilation somewhat unnecessarily.</blockquote> <blockquote>AFAIK (except for unbounded resource arrays) the native compiler doesn't allow implicit size arrays<br>without initializer, so aborting seemed logical.</blockquote><br><br>Failing compilation is fine, but in general I think we want to avoid aborting if we can help it. We<br>should only abort if we really can't continue parsing, e.g. if we encounter a syntax error. That<br>way, if there are multiple errors, the (HLSL) programmer can deal with them all at once.<br> <blockquote>(As a side note, perhaps we should use zero instead of UINT_MAX. Unbounded resources in shader</blockquote><br>model 5.1 are encoded as zero in the reflection data, whereas declaring a resource array as e.g.<br>"Texture2D t[0xffffffff]" yields 0xffffffff instead, although the shader bytecode is identical.)<br><br>As a special extra, this code is apparently valid, and I think deserves to be a test case:<br><br>struct apple<br>{<br>Texture2D t[];<br>};<blockquote>Hmm, this is interesting. Maybe it is intended for input semantics?<br>I will investigate.</blockquote><br><br>Not for input semantics, but rather for bound resources. Shader model 5.0 allows declaring arrays<br>of textures, and shader model 5.1 (and Direct3D 12) allows declaring "unbounded" arrays, as e.g.<br>"Texture2D t[]" or "Texture2D t[0]".<br><br>Normally textures aren't declared as part of a struct, but I was curious to see if it was legal,<br>and it turns out it is. (Although the reflection type information that's generated isn't quite<br>correct.)</blockquote><br>Hmm, I will see where I can add a FIXME for those cases for now then.</div></body></html>