[PATCH v2 vkd3d 1/4] vkd3d-shader: Handle preprocessor parsing errors.
Signed-off-by: Zebediah Figura <zfigura(a)codeweavers.com> --- v2: Avoid api.location.type, which is only available since Bison 3.4. libs/vkd3d-shader/preproc.h | 11 +++++++ libs/vkd3d-shader/preproc.l | 39 ++++++++++++++++++++++- libs/vkd3d-shader/preproc.y | 40 +++++++++++++++++++++++- libs/vkd3d-shader/vkd3d_shader_private.h | 2 ++ 4 files changed, 90 insertions(+), 2 deletions(-) diff --git a/libs/vkd3d-shader/preproc.h b/libs/vkd3d-shader/preproc.h index cbd93229..769b8c23 100644 --- a/libs/vkd3d-shader/preproc.h +++ b/libs/vkd3d-shader/preproc.h @@ -23,11 +23,22 @@ #include "vkd3d_shader_private.h" +struct preproc_location +{ + const char *filename; + unsigned int first_line, first_column; +}; + struct preproc_ctx { void *scanner; + struct vkd3d_shader_message_context *message_context; struct vkd3d_string_buffer buffer; + unsigned int line, column; + const char *source_name; + + bool error; }; #endif diff --git a/libs/vkd3d-shader/preproc.l b/libs/vkd3d-shader/preproc.l index 4d809b8f..116a7ed5 100644 --- a/libs/vkd3d-shader/preproc.l +++ b/libs/vkd3d-shader/preproc.l @@ -27,6 +27,10 @@ #define YY_DECL static int preproc_lexer_lex(YYSTYPE *yylval_param, YYLTYPE *yylloc_param, yyscan_t yyscanner) +static void update_location(struct preproc_ctx *ctx); + +#define YY_USER_ACTION update_location(yyget_extra(yyscanner)); + %} %option 8bit @@ -88,6 +92,28 @@ IDENTIFIER [A-Za-z_][A-Za-z0-9_]* %% +static void update_location(struct preproc_ctx *ctx) +{ + unsigned int i, leng = yyget_leng(ctx->scanner); + const char *text = yyget_text(ctx->scanner); + + /* We want to do this here, rather than before calling yylex(), because + * some tokens are skipped by the lexer. */ + + yyget_lloc(ctx->scanner)->first_line = ctx->line; + yyget_lloc(ctx->scanner)->first_column = ctx->column; + + for (i = 0; i < leng; ++i) + { + ++ctx->column; + if (text[i] == '\n') + { + ctx->column = 1; + ++ctx->line; + } + } +} + int yylex(YYSTYPE *lval, YYLTYPE *lloc, yyscan_t scanner) { struct preproc_ctx *ctx = yyget_extra(scanner); @@ -101,7 +127,7 @@ int yylex(YYSTYPE *lval, YYLTYPE *lloc, yyscan_t scanner) return 0; text = yyget_text(scanner); - TRACE("Parsing token %d, string %s.\n", token, debugstr_a(text)); + TRACE("Parsing token %d, line %d, string %s.\n", token, lloc->first_line, debugstr_a(text)); vkd3d_string_buffer_printf(&ctx->buffer, "%s ", text); } @@ -115,6 +141,10 @@ int preproc_lexer_parse(const struct vkd3d_shader_compile_info *compile_info, void *output_code; vkd3d_string_buffer_init(&ctx.buffer); + ctx.message_context = message_context; + ctx.source_name = compile_info->source_name ? compile_info->source_name : "<anonymous>"; + ctx.line = 1; + ctx.column = 1; yylex_init_extra(&ctx, &ctx.scanner); top_buffer = yy_scan_bytes(compile_info->source.code, compile_info->source.size, ctx.scanner); @@ -124,6 +154,13 @@ int preproc_lexer_parse(const struct vkd3d_shader_compile_info *compile_info, yy_delete_buffer(top_buffer, ctx.scanner); yylex_destroy(ctx.scanner); + if (ctx.error) + { + WARN("Failed to preprocess.\n"); + vkd3d_string_buffer_cleanup(&ctx.buffer); + return VKD3D_ERROR_INVALID_SHADER; + } + if (!(output_code = vkd3d_malloc(ctx.buffer.content_size))) { vkd3d_string_buffer_cleanup(&ctx.buffer); diff --git a/libs/vkd3d-shader/preproc.y b/libs/vkd3d-shader/preproc.y index 92448f24..186a0e1d 100644 --- a/libs/vkd3d-shader/preproc.y +++ b/libs/vkd3d-shader/preproc.y @@ -24,6 +24,8 @@ #include "vkd3d_shader_private.h" #include "preproc.h" +#define PREPROC_YYLTYPE struct preproc_location + } %code provides @@ -36,9 +38,45 @@ int preproc_yylex(PREPROC_YYSTYPE *yylval_param, PREPROC_YYLTYPE *yylloc_param, %code { +#define YYLLOC_DEFAULT(cur, rhs, n) \ + do \ + { \ + if (n) \ + { \ + (cur).filename = YYRHSLOC(rhs, 1).filename; \ + (cur).first_line = YYRHSLOC(rhs, 1).first_line; \ + (cur).first_column = YYRHSLOC(rhs, 1).first_column; \ + } \ + else \ + { \ + (cur).filename = YYRHSLOC(rhs, 0).filename; \ + (cur).first_line = YYRHSLOC(rhs, 0).first_line; \ + (cur).first_column = YYRHSLOC(rhs, 0).first_column; \ + } \ + } while (0) + +static void set_location(struct preproc_ctx *ctx, const struct preproc_location *loc) +{ + ctx->message_context->source_name = loc->filename; + ctx->message_context->line = loc->first_line; + ctx->message_context->column = loc->first_column; +} + +static void preproc_error(struct preproc_ctx *ctx, const struct preproc_location *loc, + enum vkd3d_shader_error error, const char *format, ...) +{ + va_list args; + + set_location(ctx, loc); + va_start(args, format); + vkd3d_shader_verror(ctx->message_context, error, format, args); + va_end(args); + ctx->error = true; +} + static void yyerror(const YYLTYPE *loc, void *scanner, struct preproc_ctx *ctx, const char *string) { - FIXME("Error reporting is not implemented.\n"); + preproc_error(ctx, loc, VKD3D_SHADER_ERROR_PP_INVALID_SYNTAX, "%s", string); } } diff --git a/libs/vkd3d-shader/vkd3d_shader_private.h b/libs/vkd3d-shader/vkd3d_shader_private.h index 83038384..5a022708 100644 --- a/libs/vkd3d-shader/vkd3d_shader_private.h +++ b/libs/vkd3d-shader/vkd3d_shader_private.h @@ -79,6 +79,8 @@ enum vkd3d_shader_error VKD3D_SHADER_ERROR_RS_INVALID_ROOT_PARAMETER_TYPE = 3002, VKD3D_SHADER_ERROR_RS_INVALID_DESCRIPTOR_RANGE_TYPE = 3003, VKD3D_SHADER_ERROR_RS_MIXED_DESCRIPTOR_RANGE_TYPES = 3004, + + VKD3D_SHADER_ERROR_PP_INVALID_SYNTAX = 4000, }; enum VKD3D_SHADER_INSTRUCTION_HANDLER -- 2.29.2
Signed-off-by: Zebediah Figura <zfigura(a)codeweavers.com> --- v2: Avoid another instance of YYEOF, which is only available since Bison 3.6. include/private/vkd3d_memory.h | 11 +++ libs/vkd3d-shader/preproc.h | 17 ++++ libs/vkd3d-shader/preproc.l | 108 +++++++++++++++++++++-- libs/vkd3d-shader/preproc.y | 102 ++++++++++++++++++++- libs/vkd3d-shader/vkd3d_shader_main.c | 15 ++++ libs/vkd3d-shader/vkd3d_shader_private.h | 6 ++ tests/hlsl_d3d12.c | 2 +- 7 files changed, 251 insertions(+), 10 deletions(-) diff --git a/include/private/vkd3d_memory.h b/include/private/vkd3d_memory.h index df93abf5..bd56d30a 100644 --- a/include/private/vkd3d_memory.h +++ b/include/private/vkd3d_memory.h @@ -22,6 +22,7 @@ #include <assert.h> #include <stdbool.h> #include <stdlib.h> +#include <string.h> #include "vkd3d_debug.h" @@ -54,6 +55,16 @@ static inline void vkd3d_free(void *ptr) free(ptr); } +static inline char *vkd3d_strdup(const char *string) +{ + size_t len = strlen(string) + 1; + char *ptr; + + if ((ptr = vkd3d_malloc(len))) + memcpy(ptr, string, len); + return ptr; +} + bool vkd3d_array_reserve(void **elements, size_t *capacity, size_t element_count, size_t element_size) DECLSPEC_HIDDEN; diff --git a/libs/vkd3d-shader/preproc.h b/libs/vkd3d-shader/preproc.h index 769b8c23..224ae5b2 100644 --- a/libs/vkd3d-shader/preproc.h +++ b/libs/vkd3d-shader/preproc.h @@ -29,6 +29,12 @@ struct preproc_location unsigned int first_line, first_column; }; +struct preproc_if_state +{ + /* Are we currently in a "true" block? */ + bool current_true; +}; + struct preproc_ctx { void *scanner; @@ -38,7 +44,18 @@ struct preproc_ctx unsigned int line, column; const char *source_name; + struct preproc_if_state *if_stack; + size_t if_count, if_stack_size; + + int current_directive; + + bool last_was_newline; + bool last_was_eof; + bool error; }; +void preproc_warning(struct preproc_ctx *ctx, const struct preproc_location *loc, + enum vkd3d_shader_error error, const char *format, ...) VKD3D_PRINTF_FUNC(4, 5) DECLSPEC_HIDDEN; + #endif diff --git a/libs/vkd3d-shader/preproc.l b/libs/vkd3d-shader/preproc.l index 116a7ed5..dd2b9c0a 100644 --- a/libs/vkd3d-shader/preproc.l +++ b/libs/vkd3d-shader/preproc.l @@ -50,6 +50,7 @@ static void update_location(struct preproc_ctx *ctx); %s C_COMMENT %s CXX_COMMENT +NEWLINE \r?\n WS [ \t] IDENTIFIER [A-Za-z_][A-Za-z0-9_]* @@ -57,10 +58,10 @@ IDENTIFIER [A-Za-z_][A-Za-z0-9_]* <INITIAL>"//" {yy_push_state(CXX_COMMENT, yyscanner);} <INITIAL>"/*" {yy_push_state(C_COMMENT, yyscanner);} -<CXX_COMMENT>\\\r?\n {} +<CXX_COMMENT>\\{NEWLINE} {} <CXX_COMMENT>\n { yy_pop_state(yyscanner); - return T_TEXT; + return T_NEWLINE; } <C_COMMENT>"*/" {yy_pop_state(yyscanner);} <C_COMMENT,CXX_COMMENT><<EOF>> {yy_pop_state(yyscanner);} @@ -68,13 +69,15 @@ IDENTIFIER [A-Za-z_][A-Za-z0-9_]* <INITIAL>{IDENTIFIER} {return T_TEXT;} + /* We have no use for floats, but shouldn't parse them as integers. */ + <INITIAL>[0-9]*\.[0-9]+([eE][+-]?[0-9]+)?[hHfF]? {return T_TEXT;} <INITIAL>[0-9]+\.([eE][+-]?[0-9]+)?[hHfF]? {return T_TEXT;} <INITIAL>[0-9]+([eE][+-]?[0-9]+)?[hHfF] {return T_TEXT;} <INITIAL>[0-9]+[eE][+-]?[0-9]+ {return T_TEXT;} -<INITIAL>0[xX][0-9a-fA-f]+[ul]{0,2} {return T_TEXT;} -<INITIAL>0[0-7]*[ul]{0,2} {return T_TEXT;} -<INITIAL>[1-9][0-9]*[ul]{0,2} {return T_TEXT;} +<INITIAL>0[xX][0-9a-fA-f]+[ul]{0,2} {return T_INTEGER;} +<INITIAL>0[0-7]*[ul]{0,2} {return T_INTEGER;} +<INITIAL>[1-9][0-9]*[ul]{0,2} {return T_INTEGER;} <INITIAL>"&&" {return T_TEXT;} <INITIAL>"||" {return T_TEXT;} @@ -87,6 +90,29 @@ IDENTIFIER [A-Za-z_][A-Za-z0-9_]* /* C strings (including escaped quotes). */ <INITIAL>\"([^"\\]|\\.)*\" {return T_TEXT;} +<INITIAL>#{WS}*{IDENTIFIER} { + struct preproc_ctx *ctx = yyget_extra(yyscanner); + const char *p; + + if (!ctx->last_was_newline) + return T_TEXT; + + for (p = yytext + 1; strchr(" \t", *p); ++p) + ; + + if (!strcmp(p, "endif")) + return T_ENDIF; + if (!strcmp(p, "if")) + return T_IF; + + preproc_warning(ctx, yyget_lloc(yyscanner), VKD3D_SHADER_WARNING_PP_UNKNOWN_DIRECTIVE, + "Ignoring unknown directive \"%s\".", yytext); + return T_TEXT; + } + +<INITIAL>\\{NEWLINE} {} +<INITIAL>{NEWLINE} {return T_NEWLINE;} + <INITIAL>{WS}+ {} <INITIAL>. {return T_TEXT;} @@ -114,6 +140,27 @@ static void update_location(struct preproc_ctx *ctx) } } +static bool preproc_is_writing(struct preproc_ctx *ctx) +{ + if (!ctx->if_count) + return true; + return ctx->if_stack[ctx->if_count - 1].current_true; +} + +static int return_token(int token, YYSTYPE *lval, const char *text) +{ + switch (token) + { + case T_INTEGER: + case T_TEXT: + if (!(lval->string = vkd3d_strdup(text))) + return 0; + break; + } + + return token; +} + int yylex(YYSTYPE *lval, YYLTYPE *lloc, yyscan_t scanner) { struct preproc_ctx *ctx = yyget_extra(scanner); @@ -123,11 +170,46 @@ int yylex(YYSTYPE *lval, YYLTYPE *lloc, yyscan_t scanner) const char *text; int token; - if (!(token = preproc_lexer_lex(lval, lloc, scanner))) + if (ctx->last_was_eof) return 0; - text = yyget_text(scanner); - TRACE("Parsing token %d, line %d, string %s.\n", token, lloc->first_line, debugstr_a(text)); + if (!(token = preproc_lexer_lex(lval, lloc, scanner))) + { + ctx->last_was_eof = true; + token = T_NEWLINE; + text = "\n"; + } + else + { + text = yyget_text(scanner); + } + + lloc->filename = ctx->source_name; + + if (ctx->last_was_newline) + { + switch (token) + { + case T_ENDIF: + case T_IF: + ctx->current_directive = token; + break; + + default: + ctx->current_directive = 0; + } + } + + ctx->last_was_newline = (token == T_NEWLINE); + + TRACE("Parsing token %d, line %d, in directive %d, string %s.\n", token, + lloc->first_line, ctx->current_directive, debugstr_a(text)); + + if (!ctx->current_directive && !preproc_is_writing(ctx)) + continue; + + if (ctx->current_directive) + return return_token(token, lval, text); vkd3d_string_buffer_printf(&ctx->buffer, "%s ", text); } @@ -148,12 +230,22 @@ int preproc_lexer_parse(const struct vkd3d_shader_compile_info *compile_info, yylex_init_extra(&ctx, &ctx.scanner); top_buffer = yy_scan_bytes(compile_info->source.code, compile_info->source.size, ctx.scanner); + ctx.last_was_newline = true; preproc_yyparse(ctx.scanner, &ctx); yy_delete_buffer(top_buffer, ctx.scanner); yylex_destroy(ctx.scanner); + if (ctx.if_count) + { + const struct preproc_location loc = {.filename = ctx.source_name}; + + preproc_warning(&ctx, &loc, VKD3D_SHADER_WARNING_PP_UNTERMINATED_IF, "Unterminated #if block."); + } + + vkd3d_free(ctx.if_stack); + if (ctx.error) { WARN("Failed to preprocess.\n"); diff --git a/libs/vkd3d-shader/preproc.y b/libs/vkd3d-shader/preproc.y index 186a0e1d..fda04a07 100644 --- a/libs/vkd3d-shader/preproc.y +++ b/libs/vkd3d-shader/preproc.y @@ -74,11 +74,71 @@ static void preproc_error(struct preproc_ctx *ctx, const struct preproc_location ctx->error = true; } +void preproc_warning(struct preproc_ctx *ctx, const struct preproc_location *loc, + enum vkd3d_shader_error error, const char *format, ...) +{ + va_list args; + + set_location(ctx, loc); + va_start(args, format); + vkd3d_shader_vwarning(ctx->message_context, error, format, args); + va_end(args); +} + static void yyerror(const YYLTYPE *loc, void *scanner, struct preproc_ctx *ctx, const char *string) { preproc_error(ctx, loc, VKD3D_SHADER_ERROR_PP_INVALID_SYNTAX, "%s", string); } +static bool preproc_was_writing(struct preproc_ctx *ctx) +{ + if (ctx->if_count < 2) + return true; + return ctx->if_stack[ctx->if_count - 2].current_true; +} + +static bool preproc_push_if(struct preproc_ctx *ctx, bool condition) +{ + struct preproc_if_state *state; + + if (!vkd3d_array_reserve((void **)&ctx->if_stack, &ctx->if_stack_size, ctx->if_count + 1, sizeof(*ctx->if_stack))) + return false; + state = &ctx->if_stack[ctx->if_count++]; + state->current_true = condition && preproc_was_writing(ctx); + return true; +} + +static int char_to_int(char c) +{ + if ('0' <= c && c <= '9') + return c - '0'; + if ('A' <= c && c <= 'F') + return c - 'A' + 10; + if ('a' <= c && c <= 'f') + return c - 'a' + 10; + return -1; +} + +static uint32_t preproc_parse_integer(const char *s) +{ + uint32_t base = 10, ret = 0; + int digit; + + if (s[0] == '0') + { + base = 8; + if (s[1] == 'x' || s[1] == 'X') + { + base = 16; + s += 2; + } + } + + while ((digit = char_to_int(*s++)) >= 0) + ret = ret * base + (uint32_t)digit; + return ret; +} + } %define api.prefix {preproc_yy} @@ -90,9 +150,49 @@ static void yyerror(const YYLTYPE *loc, void *scanner, struct preproc_ctx *ctx, %parse-param {void *scanner} %parse-param {struct preproc_ctx *ctx} -%token T_TEXT +%union +{ + char *string; + uint32_t integer; +} + +%token <string> T_INTEGER +%token <string> T_TEXT + +%token T_NEWLINE + +%token T_ENDIF "#endif" +%token T_IF "#if" + +%type <integer> expr %% shader_text : %empty + | shader_text directive + { + vkd3d_string_buffer_printf(&ctx->buffer, "\n"); + } + +directive + : T_IF expr T_NEWLINE + { + if (!preproc_push_if(ctx, !!$2)) + YYABORT; + } + | T_ENDIF T_NEWLINE + { + if (ctx->if_count) + --ctx->if_count; + else + preproc_warning(ctx, &@$, VKD3D_SHADER_WARNING_PP_INVALID_DIRECTIVE, + "Ignoring #endif without prior #if."); + } + +expr + : T_INTEGER + { + $$ = preproc_parse_integer($1); + vkd3d_free($1); + } diff --git a/libs/vkd3d-shader/vkd3d_shader_main.c b/libs/vkd3d-shader/vkd3d_shader_main.c index eed0316c..ad456133 100644 --- a/libs/vkd3d-shader/vkd3d_shader_main.c +++ b/libs/vkd3d-shader/vkd3d_shader_main.c @@ -148,6 +148,21 @@ bool vkd3d_shader_message_context_copy_messages(struct vkd3d_shader_message_cont return true; } +void vkd3d_shader_vwarning(struct vkd3d_shader_message_context *context, + enum vkd3d_shader_error error, const char *format, va_list args) +{ + if (context->log_level < VKD3D_SHADER_LOG_WARNING) + return; + + if (context->line) + vkd3d_string_buffer_printf(&context->messages, "%s:%u:%u: W%04u: ", + context->source_name, context->line, context->column, error); + else + vkd3d_string_buffer_printf(&context->messages, "%s: W%04u: ", context->source_name, error); + vkd3d_string_buffer_vprintf(&context->messages, format, args); + vkd3d_string_buffer_printf(&context->messages, "\n"); +} + void vkd3d_shader_verror(struct vkd3d_shader_message_context *context, enum vkd3d_shader_error error, const char *format, va_list args) { diff --git a/libs/vkd3d-shader/vkd3d_shader_private.h b/libs/vkd3d-shader/vkd3d_shader_private.h index 5a022708..01a74ede 100644 --- a/libs/vkd3d-shader/vkd3d_shader_private.h +++ b/libs/vkd3d-shader/vkd3d_shader_private.h @@ -81,6 +81,10 @@ enum vkd3d_shader_error VKD3D_SHADER_ERROR_RS_MIXED_DESCRIPTOR_RANGE_TYPES = 3004, VKD3D_SHADER_ERROR_PP_INVALID_SYNTAX = 4000, + + VKD3D_SHADER_WARNING_PP_INVALID_DIRECTIVE = 5001, + VKD3D_SHADER_WARNING_PP_UNKNOWN_DIRECTIVE = 5003, + VKD3D_SHADER_WARNING_PP_UNTERMINATED_IF = 5005, }; enum VKD3D_SHADER_INSTRUCTION_HANDLER @@ -867,6 +871,8 @@ void vkd3d_shader_error(struct vkd3d_shader_message_context *context, enum vkd3d const char *format, ...) VKD3D_PRINTF_FUNC(3, 4) DECLSPEC_HIDDEN; void vkd3d_shader_verror(struct vkd3d_shader_message_context *context, enum vkd3d_shader_error error, const char *format, va_list args) DECLSPEC_HIDDEN; +void vkd3d_shader_vwarning(struct vkd3d_shader_message_context *context, + enum vkd3d_shader_error error, const char *format, va_list args) DECLSPEC_HIDDEN; int shader_extract_from_dxbc(const void *dxbc, size_t dxbc_length, struct vkd3d_shader_message_context *message_context, struct vkd3d_shader_desc *desc) DECLSPEC_HIDDEN; diff --git a/tests/hlsl_d3d12.c b/tests/hlsl_d3d12.c index 61324fa9..77a7ea1a 100644 --- a/tests/hlsl_d3d12.c +++ b/tests/hlsl_d3d12.c @@ -414,7 +414,7 @@ static void test_preprocess(void) hr = D3DPreprocess(test_include_top, strlen(test_include_top), NULL, NULL, &test_include_fail, &blob, &errors); todo ok(hr == E_FAIL, "Got hr %#x.\n", hr); todo ok(blob == (ID3D10Blob *)0xdeadbeef, "Expected no compiled shader blob.\n"); - todo ok(!!errors, "Expected non-NULL error blob.\n"); + ok(!!errors, "Expected non-NULL error blob.\n"); if (errors) { if (vkd3d_test_state.debug_level) -- 2.29.2
On Fri, 11 Dec 2020 at 00:27, Zebediah Figura <zfigura(a)codeweavers.com> wrote:
diff --git a/libs/vkd3d-shader/vkd3d_shader_private.h b/libs/vkd3d-shader/vkd3d_shader_private.h index 5a022708..01a74ede 100644 --- a/libs/vkd3d-shader/vkd3d_shader_private.h +++ b/libs/vkd3d-shader/vkd3d_shader_private.h @@ -81,6 +81,10 @@ enum vkd3d_shader_error VKD3D_SHADER_ERROR_RS_MIXED_DESCRIPTOR_RANGE_TYPES = 3004,
VKD3D_SHADER_ERROR_PP_INVALID_SYNTAX = 4000, + + VKD3D_SHADER_WARNING_PP_INVALID_DIRECTIVE = 5001, + VKD3D_SHADER_WARNING_PP_UNKNOWN_DIRECTIVE = 5003, + VKD3D_SHADER_WARNING_PP_UNTERMINATED_IF = 5005, };
enum VKD3D_SHADER_INSTRUCTION_HANDLER @@ -867,6 +871,8 @@ void vkd3d_shader_error(struct vkd3d_shader_message_context *context, enum vkd3d const char *format, ...) VKD3D_PRINTF_FUNC(3, 4) DECLSPEC_HIDDEN; void vkd3d_shader_verror(struct vkd3d_shader_message_context *context, enum vkd3d_shader_error error, const char *format, va_list args) DECLSPEC_HIDDEN; +void vkd3d_shader_vwarning(struct vkd3d_shader_message_context *context, + enum vkd3d_shader_error error, const char *format, va_list args) DECLSPEC_HIDDEN;
Is reusing enum vkd3d_shader_error for warnings instead of introducing a separate enumeration here intentional?
On 12/11/20 12:11 PM, Henri Verbeet wrote:
On Fri, 11 Dec 2020 at 00:27, Zebediah Figura <zfigura(a)codeweavers.com> wrote:
diff --git a/libs/vkd3d-shader/vkd3d_shader_private.h b/libs/vkd3d-shader/vkd3d_shader_private.h index 5a022708..01a74ede 100644 --- a/libs/vkd3d-shader/vkd3d_shader_private.h +++ b/libs/vkd3d-shader/vkd3d_shader_private.h @@ -81,6 +81,10 @@ enum vkd3d_shader_error VKD3D_SHADER_ERROR_RS_MIXED_DESCRIPTOR_RANGE_TYPES = 3004,
VKD3D_SHADER_ERROR_PP_INVALID_SYNTAX = 4000, + + VKD3D_SHADER_WARNING_PP_INVALID_DIRECTIVE = 5001, + VKD3D_SHADER_WARNING_PP_UNKNOWN_DIRECTIVE = 5003, + VKD3D_SHADER_WARNING_PP_UNTERMINATED_IF = 5005, };
enum VKD3D_SHADER_INSTRUCTION_HANDLER @@ -867,6 +871,8 @@ void vkd3d_shader_error(struct vkd3d_shader_message_context *context, enum vkd3d const char *format, ...) VKD3D_PRINTF_FUNC(3, 4) DECLSPEC_HIDDEN; void vkd3d_shader_verror(struct vkd3d_shader_message_context *context, enum vkd3d_shader_error error, const char *format, va_list args) DECLSPEC_HIDDEN; +void vkd3d_shader_vwarning(struct vkd3d_shader_message_context *context, + enum vkd3d_shader_error error, const char *format, va_list args) DECLSPEC_HIDDEN;
Is reusing enum vkd3d_shader_error for warnings instead of introducing a separate enumeration here intentional?
It was intentional, though of course not necessary. I guess my reasoning was that there's no reason to let any numerical values coincide, and hence no reason to bother separating the enumerations...
On Sat, 12 Dec 2020 at 01:57, Zebediah Figura (she/her) <zfigura(a)codeweavers.com> wrote:
On 12/11/20 12:11 PM, Henri Verbeet wrote:
Is reusing enum vkd3d_shader_error for warnings instead of introducing a separate enumeration here intentional?
It was intentional, though of course not necessary. I guess my reasoning was that there's no reason to let any numerical values coincide, and hence no reason to bother separating the enumerations...
Fair enough, although it does look a bit odd to pass an enum vkd3d_shader_error to vkd3d_shader_warning(). We may want to consider starting the preprocessor warning range at e.g. 4200 or 4300 in that case though; if we were to add VKD3D_SHADER_LOG_WARNING and VKD3D_SHADER_LOG_INFO ranges for all the existing VKD3D_SHADER_LOG_ERROR ranges using the current allocation scheme, 4-digit error codes would already be insufficient. (And there are still some sources/targets we may potentially add; HLSL of course, but also GLSL, perhaps MSL, probably DXIL, D3D shader model 1-3 bytecode.)
On 12/14/20 9:26 AM, Henri Verbeet wrote:
On Sat, 12 Dec 2020 at 01:57, Zebediah Figura (she/her) <zfigura(a)codeweavers.com> wrote:
On 12/11/20 12:11 PM, Henri Verbeet wrote:
Is reusing enum vkd3d_shader_error for warnings instead of introducing a separate enumeration here intentional?
It was intentional, though of course not necessary. I guess my reasoning was that there's no reason to let any numerical values coincide, and hence no reason to bother separating the enumerations...
Fair enough, although it does look a bit odd to pass an enum vkd3d_shader_error to vkd3d_shader_warning(). We may want to consider starting the preprocessor warning range at e.g. 4200 or 4300 in that case though; if we were to add VKD3D_SHADER_LOG_WARNING and VKD3D_SHADER_LOG_INFO ranges for all the existing VKD3D_SHADER_LOG_ERROR ranges using the current allocation scheme, 4-digit error codes would already be insufficient. (And there are still some sources/targets we may potentially add; HLSL of course, but also GLSL, perhaps MSL, probably DXIL, D3D shader model 1-3 bytecode.)
I hadn't thought about keeping error codes four digits long; that makes sense to me. I'll adjust the numerical values.
Signed-off-by: Zebediah Figura <zfigura(a)codeweavers.com> --- libs/vkd3d-shader/preproc.h | 4 ++++ libs/vkd3d-shader/preproc.l | 3 +++ libs/vkd3d-shader/preproc.y | 25 +++++++++++++++++++++++++ 3 files changed, 32 insertions(+) diff --git a/libs/vkd3d-shader/preproc.h b/libs/vkd3d-shader/preproc.h index 224ae5b2..c05e3f6d 100644 --- a/libs/vkd3d-shader/preproc.h +++ b/libs/vkd3d-shader/preproc.h @@ -33,6 +33,10 @@ struct preproc_if_state { /* Are we currently in a "true" block? */ bool current_true; + /* Have we seen a "true" block in this #if..#endif yet? */ + bool seen_true; + /* Have we seen an #else yet? */ + bool seen_else; }; struct preproc_ctx diff --git a/libs/vkd3d-shader/preproc.l b/libs/vkd3d-shader/preproc.l index dd2b9c0a..fc3c5fa2 100644 --- a/libs/vkd3d-shader/preproc.l +++ b/libs/vkd3d-shader/preproc.l @@ -100,6 +100,8 @@ IDENTIFIER [A-Za-z_][A-Za-z0-9_]* for (p = yytext + 1; strchr(" \t", *p); ++p) ; + if (!strcmp(p, "else")) + return T_ELSE; if (!strcmp(p, "endif")) return T_ENDIF; if (!strcmp(p, "if")) @@ -190,6 +192,7 @@ int yylex(YYSTYPE *lval, YYLTYPE *lloc, yyscan_t scanner) { switch (token) { + case T_ELSE: case T_ENDIF: case T_IF: ctx->current_directive = token; diff --git a/libs/vkd3d-shader/preproc.y b/libs/vkd3d-shader/preproc.y index fda04a07..add39735 100644 --- a/libs/vkd3d-shader/preproc.y +++ b/libs/vkd3d-shader/preproc.y @@ -105,6 +105,8 @@ static bool preproc_push_if(struct preproc_ctx *ctx, bool condition) return false; state = &ctx->if_stack[ctx->if_count++]; state->current_true = condition && preproc_was_writing(ctx); + state->seen_true = condition; + state->seen_else = false; return true; } @@ -161,6 +163,7 @@ static uint32_t preproc_parse_integer(const char *s) %token T_NEWLINE +%token T_ELSE "#else" %token T_ENDIF "#endif" %token T_IF "#if" @@ -181,6 +184,28 @@ directive if (!preproc_push_if(ctx, !!$2)) YYABORT; } + | T_ELSE T_NEWLINE + { + if (ctx->if_count) + { + struct preproc_if_state *state = &ctx->if_stack[ctx->if_count - 1]; + + if (state->seen_else) + { + preproc_warning(ctx, &@$, VKD3D_SHADER_WARNING_PP_INVALID_DIRECTIVE, "Ignoring #else after #else."); + } + else + { + state->current_true = !state->seen_true && preproc_was_writing(ctx); + state->seen_else = true; + } + } + else + { + preproc_warning(ctx, &@$, VKD3D_SHADER_WARNING_PP_INVALID_DIRECTIVE, + "Ignoring #else without prior #if."); + } + } | T_ENDIF T_NEWLINE { if (ctx->if_count) -- 2.29.2
Signed-off-by: Zebediah Figura <zfigura(a)codeweavers.com> --- Makefile.am | 1 - libs/vkd3d-shader/preproc.l | 3 +++ libs/vkd3d-shader/preproc.y | 23 +++++++++++++++++++++++ 3 files changed, 26 insertions(+), 1 deletion(-) diff --git a/Makefile.am b/Makefile.am index 32c8777f..f6fbe416 100644 --- a/Makefile.am +++ b/Makefile.am @@ -241,7 +241,6 @@ XFAIL_TESTS = \ tests/hlsl-vector-indexing.shader_test \ tests/hlsl-vector-indexing-uniform.shader_test \ tests/math.shader_test \ - tests/preproc-if.shader_test \ tests/preproc-ifdef.shader_test \ tests/preproc-if-expr.shader_test \ tests/preproc-invalid.shader_test \ diff --git a/libs/vkd3d-shader/preproc.l b/libs/vkd3d-shader/preproc.l index fc3c5fa2..bf1ef5c0 100644 --- a/libs/vkd3d-shader/preproc.l +++ b/libs/vkd3d-shader/preproc.l @@ -100,6 +100,8 @@ IDENTIFIER [A-Za-z_][A-Za-z0-9_]* for (p = yytext + 1; strchr(" \t", *p); ++p) ; + if (!strcmp(p, "elif")) + return T_ELIF; if (!strcmp(p, "else")) return T_ELSE; if (!strcmp(p, "endif")) @@ -192,6 +194,7 @@ int yylex(YYSTYPE *lval, YYLTYPE *lloc, yyscan_t scanner) { switch (token) { + case T_ELIF: case T_ELSE: case T_ENDIF: case T_IF: diff --git a/libs/vkd3d-shader/preproc.y b/libs/vkd3d-shader/preproc.y index add39735..e0b3c4c1 100644 --- a/libs/vkd3d-shader/preproc.y +++ b/libs/vkd3d-shader/preproc.y @@ -163,6 +163,7 @@ static uint32_t preproc_parse_integer(const char *s) %token T_NEWLINE +%token T_ELIF "#elif" %token T_ELSE "#else" %token T_ENDIF "#endif" %token T_IF "#if" @@ -184,6 +185,28 @@ directive if (!preproc_push_if(ctx, !!$2)) YYABORT; } + | T_ELIF expr T_NEWLINE + { + if (ctx->if_count) + { + struct preproc_if_state *state = &ctx->if_stack[ctx->if_count - 1]; + + if (state->seen_else) + { + preproc_warning(ctx, &@$, VKD3D_SHADER_WARNING_PP_INVALID_DIRECTIVE, "Ignoring #elif after #else."); + } + else + { + state->current_true = $2 && !state->seen_true && preproc_was_writing(ctx); + state->seen_true = $2 || state->seen_true; + } + } + else + { + preproc_warning(ctx, &@$, VKD3D_SHADER_WARNING_PP_INVALID_DIRECTIVE, + "Ignoring #elif without prior #if."); + } + } | T_ELSE T_NEWLINE { if (ctx->if_count) -- 2.29.2
On Fri, 11 Dec 2020 at 00:27, Zebediah Figura <zfigura(a)codeweavers.com> wrote:
+struct preproc_location +{ + const char *filename; + unsigned int first_line, first_column; +}; + struct preproc_ctx { void *scanner;
+ struct vkd3d_shader_message_context *message_context; struct vkd3d_string_buffer buffer; + unsigned int line, column; + const char *source_name; + + bool error; };
There seems to be some duplication of location information between struct vkd3d_shader_message_context, struct preproc_ctx, and struct preproc_location. Is that intentional? If it is, would it make sense to introduce e.g. a struct vkd3d_shader_source_location instead of struct preproc_location, and then use that structure in both struct vkd3d_shader_message_context and struct preproc_ctx?
+#define YYLLOC_DEFAULT(cur, rhs, n) \ + do \ + { \ + if (n) \ + { \ + (cur).filename = YYRHSLOC(rhs, 1).filename; \ + (cur).first_line = YYRHSLOC(rhs, 1).first_line; \ + (cur).first_column = YYRHSLOC(rhs, 1).first_column; \ + } \ + else \ + { \ + (cur).filename = YYRHSLOC(rhs, 0).filename; \ + (cur).first_line = YYRHSLOC(rhs, 0).first_line; \ + (cur).first_column = YYRHSLOC(rhs, 0).first_column; \ + } \ + } while (0) + Generally speaking, we would try to avoid multi-line macros by doing something like the following where possible:
static inline void preproc_yylloc_default(struct preproc_location *cur, const struct preproc_location *rhs, int n) { if (n) { cur->filename = YYRHSLOC(rhs, 1).filename; cur->first_line = YYRHSLOC(rhs, 1).first_line; cur->first_column = YYRHSLOC(rhs, 1).first_column; } else { cur->filename = YYRHSLOC(rhs, 0).filename; cur->first_line = YYRHSLOC(rhs, 0).first_line; cur->first_column = YYRHSLOC(rhs, 0).first_column; } } #define YYLLOC_DEFAULT(cur, rhs, n) preproc_yylloc_default(&cur, rhs, n) In this particular case though, I think all that essentially reduces to: #define YYLLOC_DEFAULT(cur, rhs, n) (cur) = YYRHSLOC(rhs, !!n)
On 12/11/20 12:11 PM, Henri Verbeet wrote:
On Fri, 11 Dec 2020 at 00:27, Zebediah Figura <zfigura(a)codeweavers.com> wrote:
+struct preproc_location +{ + const char *filename; + unsigned int first_line, first_column; +}; + struct preproc_ctx { void *scanner;
+ struct vkd3d_shader_message_context *message_context; struct vkd3d_string_buffer buffer; + unsigned int line, column; + const char *source_name; + + bool error; };
There seems to be some duplication of location information between struct vkd3d_shader_message_context, struct preproc_ctx, and struct preproc_location. Is that intentional? If it is, would it make sense to introduce e.g. a struct vkd3d_shader_source_location instead of struct preproc_location, and then use that structure in both struct vkd3d_shader_message_context and struct preproc_ctx?
Wrt the vkd3d_shader_message_context part—the current structure definitely isn't a great fit for the preprocessor or HLSL parser (i.e. the parts that use yacc) because the location tracking means that we'll be setting a new location on every error call. (And of course most of the other parsers we have don't really have a meaningful concept of line/column locations...) But I wasn't sure enough to actually try to redesign those parts, and it seemed like something that could be deferred, so I just left it alone... Wrt preproc_location vs preproc_ctx—part of the rub here is that {line, column} is a bit different from {first_line, first_column}; i.e. if we were to add {last_line, last_column}, that wouldn't be a part of preproc_ctx.
+#define YYLLOC_DEFAULT(cur, rhs, n) \ + do \ + { \ + if (n) \ + { \ + (cur).filename = YYRHSLOC(rhs, 1).filename; \ + (cur).first_line = YYRHSLOC(rhs, 1).first_line; \ + (cur).first_column = YYRHSLOC(rhs, 1).first_column; \ + } \ + else \ + { \ + (cur).filename = YYRHSLOC(rhs, 0).filename; \ + (cur).first_line = YYRHSLOC(rhs, 0).first_line; \ + (cur).first_column = YYRHSLOC(rhs, 0).first_column; \ + } \ + } while (0) + Generally speaking, we would try to avoid multi-line macros by doing something like the following where possible:
static inline void preproc_yylloc_default(struct preproc_location *cur, const struct preproc_location *rhs, int n) { if (n) { cur->filename = YYRHSLOC(rhs, 1).filename; cur->first_line = YYRHSLOC(rhs, 1).first_line; cur->first_column = YYRHSLOC(rhs, 1).first_column; } else { cur->filename = YYRHSLOC(rhs, 0).filename; cur->first_line = YYRHSLOC(rhs, 0).first_line; cur->first_column = YYRHSLOC(rhs, 0).first_column; } }
#define YYLLOC_DEFAULT(cur, rhs, n) preproc_yylloc_default(&cur, rhs, n)
In this particular case though, I think all that essentially reduces to:
#define YYLLOC_DEFAULT(cur, rhs, n) (cur) = YYRHSLOC(rhs, !!n)
Excellent point, thanks.
On Sat, 12 Dec 2020 at 01:52, Zebediah Figura (she/her) <zfigura(a)codeweavers.com> wrote:
On 12/11/20 12:11 PM, Henri Verbeet wrote:
On Fri, 11 Dec 2020 at 00:27, Zebediah Figura <zfigura(a)codeweavers.com> wrote:
+struct preproc_location +{ + const char *filename; + unsigned int first_line, first_column; +}; + struct preproc_ctx { void *scanner;
+ struct vkd3d_shader_message_context *message_context; struct vkd3d_string_buffer buffer; + unsigned int line, column; + const char *source_name; + + bool error; };
There seems to be some duplication of location information between struct vkd3d_shader_message_context, struct preproc_ctx, and struct preproc_location. Is that intentional? If it is, would it make sense to introduce e.g. a struct vkd3d_shader_source_location instead of struct preproc_location, and then use that structure in both struct vkd3d_shader_message_context and struct preproc_ctx?
Wrt the vkd3d_shader_message_context part—the current structure definitely isn't a great fit for the preprocessor or HLSL parser (i.e. the parts that use yacc) because the location tracking means that we'll be setting a new location on every error call. (And of course most of the other parsers we have don't really have a meaningful concept of line/column locations...) But I wasn't sure enough to actually try to redesign those parts, and it seemed like something that could be deferred, so I just left it alone...
I probably haven't looked at this series in enough detail, so I may be missing something obvious, but is the issue that we're getting an explicit location in yyerror() that may not match the location stored in struct preproc_ctx? I think it would be fine to pass an explicit location to vkd3d_shader_error() and similar functions as well, and moving location tracking out of struct vkd3d_shader_message_context. That probably doesn't need to delay this series, but if that change is what we want, it seems easier to make it sooner rather than later. It does look like we'd want a struct vkd3d_shader_source_location instead of struct preproc_location in that case.
Wrt preproc_location vs preproc_ctx—part of the rub here is that {line, column} is a bit different from {first_line, first_column}; i.e. if we were to add {last_line, last_column}, that wouldn't be a part of preproc_ctx.
Hmm, right.
On 12/14/20 9:26 AM, Henri Verbeet wrote:
On Sat, 12 Dec 2020 at 01:52, Zebediah Figura (she/her) <zfigura(a)codeweavers.com> wrote:
On 12/11/20 12:11 PM, Henri Verbeet wrote:
On Fri, 11 Dec 2020 at 00:27, Zebediah Figura <zfigura(a)codeweavers.com> wrote:
+struct preproc_location +{ + const char *filename; + unsigned int first_line, first_column; +}; + struct preproc_ctx { void *scanner;
+ struct vkd3d_shader_message_context *message_context; struct vkd3d_string_buffer buffer; + unsigned int line, column; + const char *source_name; + + bool error; };
There seems to be some duplication of location information between struct vkd3d_shader_message_context, struct preproc_ctx, and struct preproc_location. Is that intentional? If it is, would it make sense to introduce e.g. a struct vkd3d_shader_source_location instead of struct preproc_location, and then use that structure in both struct vkd3d_shader_message_context and struct preproc_ctx?
Wrt the vkd3d_shader_message_context part—the current structure definitely isn't a great fit for the preprocessor or HLSL parser (i.e. the parts that use yacc) because the location tracking means that we'll be setting a new location on every error call. (And of course most of the other parsers we have don't really have a meaningful concept of line/column locations...) But I wasn't sure enough to actually try to redesign those parts, and it seemed like something that could be deferred, so I just left it alone...
I probably haven't looked at this series in enough detail, so I may be missing something obvious, but is the issue that we're getting an explicit location in yyerror() that may not match the location stored in struct preproc_ctx? I think it would be fine to pass an explicit location to vkd3d_shader_error() and similar functions as well, and moving location tracking out of struct vkd3d_shader_message_context.
yyerror() is one example where we need to preserve a location that's not the "current" one, but it actually ends up being most errors. In the case of the HLSL compiler, except for a scant few errors generated by the lexer, pretty much everything has a location attached that doesn't match the lexer's current location. This is partly because of lookahead (e.g. we want to complain about variable attributes on a function, and have the caret point toward the invalid attribute, but we don't know which one we've parsed yet), partly because bison makes no guarantees about where the lexer actually is when a given rule is parsed, and partly because we report some errors *after* having parsed the whole file [which we do by saving the locations of code that caused us to generate instructions and other IR objects]. In the case of the preprocessor, it's a bit odd, because most errors can be blamed on a malformed or misplaced directive [and so even including the column in the first place doesn't necessarily make a lot of sense]. It'd be arguably possible to always pass the current line and file name, except that it'd actually need to be the previous line and file name, since we've already parsed a newline at that point. But this is also sort of depending on bison internals that I don't think are guaranteed.
That probably doesn't need to delay this series, but if that change is what we want, it seems easier to make it sooner rather than later.
Well, I did add preproc_error() as a helper (and there are already similar helpers for the HLSL compiler) so that it wouldn't be too much of a problem. But it is of course nice to not punt the problem, and it's not too hard...
It does look like we'd want a struct vkd3d_shader_source_location instead of struct preproc_location in that case.
Wrt preproc_location vs preproc_ctx—part of the rub here is that {line, column} is a bit different from {first_line, first_column}; i.e. if we were to add {last_line, last_column}, that wouldn't be a part of preproc_ctx.
Hmm, right.
participants (3)
-
Henri Verbeet -
Zebediah Figura -
Zebediah Figura (she/her)