[PATCH 0/6] MR570: widl: Cleanup in preparation for sharing lexer and parser with some midl.exe program.
In order to write conformance tests for widl we'll need to run midl.exe from the SDK against some tests IDL, and do the same with our own IDL parser implementation. This is going to be easier using some midl.exe program, which will import widl lexer and parser. This MR is some cleanup in preparation for this, trying to reduce the amount of global state in widl, and make it possible to share only selected parts with midl.exe. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/570
From: R��mi Bernon <rbernon(a)codeweavers.com> --- tools/widl/parser.l | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) diff --git a/tools/widl/parser.l b/tools/widl/parser.l index 9286a494d4d..1e602a0ac98 100644 --- a/tools/widl/parser.l +++ b/tools/widl/parser.l @@ -20,6 +20,7 @@ %option stack %option noinput nounput noyy_top_state +%option noyywrap %option 8bit never-interactive prefix="parser_" nl \r?\n @@ -232,13 +233,6 @@ SAFEARRAY{ws}*/\( return tSAFEARRAY; } %% -#ifndef parser_wrap -int parser_wrap(void) -{ - return 1; -} -#endif - struct keyword { const char *kw; int token; -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/570
From: R��mi Bernon <rbernon(a)codeweavers.com> --- tools/widl/parser.h | 2 -- tools/widl/parser.l | 42 ++++++++++++++++++++++++------------------ tools/widl/parser.y | 11 +++++++++-- 3 files changed, 33 insertions(+), 22 deletions(-) diff --git a/tools/widl/parser.h b/tools/widl/parser.h index bc0c50792e1..47bcf4bc5db 100644 --- a/tools/widl/parser.h +++ b/tools/widl/parser.h @@ -28,8 +28,6 @@ extern char *parser_text; extern int parser_debug; extern int yy_flex_debug; -int parser_lex(void); - extern int import_stack_ptr; int do_import(char *fname); void abort_import(void); diff --git a/tools/widl/parser.l b/tools/widl/parser.l index 1e602a0ac98..1493e2d6ad7 100644 --- a/tools/widl/parser.l +++ b/tools/widl/parser.l @@ -18,6 +18,7 @@ * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301, USA */ +%option bison-bridge %option stack %option noinput nounput noyy_top_state %option noyywrap @@ -59,6 +60,11 @@ double [0-9]+\.[0-9]+([eE][+-]?[0-9]+)* #include "parser.h" #include "wpp_private.h" +#define YYerror PARSER_error +#define YYSTYPE PARSER_STYPE +#define YYUNDEF PARSER_UNDEF +#define yyerror parser_error + #include "parser.tab.h" static void addcchar(char c); @@ -68,8 +74,8 @@ static char *cbuffer; static int cbufidx; static int cbufalloc = 0; -static int kw_token(const char *kw); -static int attr_token(const char *kw); +static int kw_token(const char *kw, YYSTYPE *yylval); +static int attr_token(const char *kw, YYSTYPE *yylval); static void switch_to_acf(void); @@ -162,24 +168,24 @@ struct uuid *parse_uuid(const char *u) } yy_pop_state(); } -<PP_PRAGMA>[^\n]* parser_lval.str = xstrdup(yytext); yy_pop_state(); return aPRAGMA; +<PP_PRAGMA>[^\n]* yylval->str = xstrdup(yytext); yy_pop_state(); return aPRAGMA; <INITIAL>^{ws}*midl_pragma{ws}+warning return tPRAGMA_WARNING; <INITIAL,ATTR>\" yy_push_state(QUOTE); cbufidx = 0; <QUOTE>\" { yy_pop_state(); - parser_lval.str = get_buffered_cstring(); + yylval->str = get_buffered_cstring(); return aSTRING; } <INITIAL,ATTR>L\" yy_push_state(WSTRQUOTE); cbufidx = 0; <WSTRQUOTE>\" { yy_pop_state(); - parser_lval.str = get_buffered_cstring(); + yylval->str = get_buffered_cstring(); return aWSTRING; } <INITIAL,ATTR>\' yy_push_state(SQUOTE); cbufidx = 0; <SQUOTE>\' { yy_pop_state(); - parser_lval.str = get_buffered_cstring(); + yylval->str = get_buffered_cstring(); return aSQSTRING; } <QUOTE,WSTRQUOTE,SQUOTE>\\\\ | @@ -189,25 +195,25 @@ struct uuid *parse_uuid(const char *u) <QUOTE,WSTRQUOTE,SQUOTE>. addcchar(yytext[0]); <INITIAL,ATTR>\[ yy_push_state(ATTR); return '['; <ATTR>\] yy_pop_state(); return ']'; -<ATTR>{cident} return attr_token(yytext); +<ATTR>{cident} return attr_token(yytext, yylval); <ATTR>{uuid} { - parser_lval.uuid = parse_uuid(yytext); + yylval->uuid = parse_uuid(yytext); return aUUID; } <INITIAL,ATTR>{hex} { - parser_lval.num = xstrtoul(yytext, NULL, 0); + yylval->num = xstrtoul(yytext, NULL, 0); return aHEXNUM; } <INITIAL,ATTR>{int} { - parser_lval.num = xstrtoul(yytext, NULL, 0); + yylval->num = xstrtoul(yytext, NULL, 0); return aNUM; } <INITIAL>{double} { - parser_lval.dbl = strtod(yytext, NULL); + yylval->dbl = strtod(yytext, NULL); return aDOUBLE; } SAFEARRAY{ws}*/\( return tSAFEARRAY; -{cident} return kw_token(yytext); +{cident} return kw_token(yytext, yylval); <INITIAL,ATTR>\n line_number++; <INITIAL,ATTR>{ws} <INITIAL,ATTR>\<\< return SHL; @@ -452,30 +458,30 @@ static int kw_cmp_func(const void *s1, const void *s2) return strcmp(KWP(s1)->kw, KWP(s2)->kw); } -static int kw_token(const char *kw) +static int kw_token(const char *kw, YYSTYPE *yylval) { struct keyword key, *kwp; key.kw = kw; kwp = bsearch(&key, keywords, NKEYWORDS, sizeof(keywords[0]), kw_cmp_func); if (kwp && (!kwp->winrt_only || winrt_mode)) { - parser_lval.str = xstrdup(kwp->kw); + yylval->str = xstrdup(kwp->kw); return kwp->token; } - parser_lval.str = xstrdup(kw); + yylval->str = xstrdup(kw); return is_type(kw) ? aKNOWNTYPE : aIDENTIFIER; } -static int attr_token(const char *kw) +static int attr_token(const char *kw, YYSTYPE *yylval) { struct keyword key, *kwp; key.kw = kw; kwp = bsearch(&key, attr_keywords, sizeof(attr_keywords)/sizeof(attr_keywords[0]), sizeof(attr_keywords[0]), kw_cmp_func); if (kwp && (!kwp->winrt_only || winrt_mode)) { - parser_lval.str = xstrdup(kwp->kw); + yylval->str = xstrdup(kwp->kw); return kwp->token; } - return kw_token(kw); + return kw_token(kw, yylval); } static void addcchar(char c) diff --git a/tools/widl/parser.y b/tools/widl/parser.y index 4000f37032c..6d8332a2423 100644 --- a/tools/widl/parser.y +++ b/tools/widl/parser.y @@ -121,7 +121,16 @@ static typelib_t *current_typelib; %} +%code provides +{ + +int parser_lex( PARSER_STYPE *yylval ); + +} + %define api.prefix {parser_} +%define api.pure full +%define parse.error verbose %union { attr_t *attr; @@ -347,8 +356,6 @@ static typelib_t *current_typelib; %right '!' '~' CAST PPTR POS NEG ADDRESSOF tSIZEOF %left '.' MEMBERPTR '[' ']' -%define parse.error verbose - %% input: gbl_statements m_acf { $1 = append_parameterized_type_stmts($1); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/570
From: R��mi Bernon <rbernon(a)codeweavers.com> --- tools/widl/client.c | 29 ++++++++++++----------------- tools/widl/header.c | 42 +++++++++++++++++------------------------- tools/widl/parser.h | 8 +++++++- tools/widl/parser.y | 12 +++--------- tools/widl/proxy.c | 39 +++++++++++++++++++-------------------- tools/widl/register.c | 18 +++++++++--------- tools/widl/server.c | 38 ++++++++++++++++---------------------- tools/widl/utils.c | 2 +- tools/widl/utils.h | 2 +- tools/widl/widl.c | 41 ++++++++++++++++++++++++++--------------- tools/widl/widl.h | 18 +++++++++--------- 11 files changed, 120 insertions(+), 129 deletions(-) diff --git a/tools/widl/client.c b/tools/widl/client.c index 704be91cb78..d15fe8cd535 100644 --- a/tools/widl/client.c +++ b/tools/widl/client.c @@ -488,17 +488,15 @@ static void write_implicithandledecl(type_t *iface) } } - -static void init_client(void) +static void init_client( const struct idl_ctx *ctx ) { if (client) return; - if (!(client = fopen(client_name, "w"))) - error("Could not open %s for output\n", client_name); + if (!(client = fopen( client_name, "w" ))) error( "Could not open %s for output\n", client_name ); - print_client("/*** Autogenerated by WIDL %s from %s - Do not edit ***/\n", PACKAGE_VERSION, input_name); - print_client("#include <string.h>\n"); + print_client( "/*** Autogenerated by WIDL %s from %s - Do not edit ***/\n", PACKAGE_VERSION, ctx->input ); + print_client( "#include <string.h>\n" ); print_client( "\n"); - print_client("#include \"%s\"\n", header_name); + print_client( "#include \"%s\"\n", header_name ); print_client( "\n"); print_client( "#ifndef DECLSPEC_HIDDEN\n"); print_client( "#define DECLSPEC_HIDDEN\n"); @@ -611,17 +609,14 @@ static void write_client_routines(const statement_list_t *stmts) write_typeformatstring(client, stmts, need_stub); } -void write_client(const statement_list_t *stmts) +void write_client( const struct idl_ctx *ctx ) { - if (!do_client) - return; - if (do_everything && !need_stub_files(stmts)) - return; + if (!do_client) return; + if (do_everything && !need_stub_files( ctx->statements )) return; - init_client(); - if (!client) - return; + init_client( ctx ); + if (!client) return; - write_client_routines( stmts ); - fclose(client); + write_client_routines( ctx->statements ); + fclose( client ); } diff --git a/tools/widl/header.c b/tools/widl/header.c index 8b9c64e1e38..5ba1f57a8c7 100644 --- a/tools/widl/header.c +++ b/tools/widl/header.c @@ -1495,24 +1495,20 @@ static void write_local_stubs_stmts(FILE *local_stubs, const statement_list_t *s } } -void write_local_stubs(const statement_list_t *stmts) +void write_local_stubs( const struct idl_ctx *ctx ) { - FILE *local_stubs; + FILE *local_stubs; - if (!local_stubs_name) return; + if (!local_stubs_name) return; + if (!(local_stubs = fopen( local_stubs_name, "w" ))) error( "Could not open %s for output\n", local_stubs_name ); - local_stubs = fopen(local_stubs_name, "w"); - if (!local_stubs) { - error("Could not open %s for output\n", local_stubs_name); - return; - } - fprintf(local_stubs, "/* call_as/local stubs for %s */\n\n", input_name); - fprintf(local_stubs, "#include <objbase.h>\n"); - fprintf(local_stubs, "#include \"%s\"\n\n", header_name); + fprintf( local_stubs, "/* call_as/local stubs for %s */\n\n", ctx->input ); + fprintf( local_stubs, "#include <objbase.h>\n" ); + fprintf( local_stubs, "#include \"%s\"\n\n", header_name ); - write_local_stubs_stmts(local_stubs, stmts); + write_local_stubs_stmts( local_stubs, ctx->statements ); - fclose(local_stubs); + fclose(local_stubs); } static void write_function_proto(FILE *header, const type_t *iface, const var_t *fun, const char *prefix) @@ -2125,17 +2121,13 @@ static void write_header_stmts(FILE *header, const statement_list_t *stmts, cons } } -void write_header(const statement_list_t *stmts) +void write_header( const struct idl_ctx *ctx ) { FILE *header; if (!do_header) return; - - if(!(header = fopen(header_name, "w"))) { - error("Could not open %s for output\n", header_name); - return; - } - fprintf(header, "/*** Autogenerated by WIDL %s from %s - Do not edit ***/\n\n", PACKAGE_VERSION, input_name); + if (!(header = fopen( header_name, "w" ))) error( "Could not open %s for output\n", header_name ); + fprintf( header, "/*** Autogenerated by WIDL %s from %s - Do not edit ***/\n\n", PACKAGE_VERSION, ctx->input ); fprintf(header, "#ifdef _WIN32\n"); fprintf(header, "#ifndef __REQUIRED_RPCNDR_H_VERSION__\n"); @@ -2143,7 +2135,7 @@ void write_header(const statement_list_t *stmts) fprintf(header, "#endif\n"); fprintf(header, "#include <rpc.h>\n" ); fprintf(header, "#include <rpcndr.h>\n" ); - if (!for_each_serializable(stmts, NULL, serializable_exists)) + if (!for_each_serializable(ctx->statements, NULL, serializable_exists)) fprintf(header, "#include <midles.h>\n" ); fprintf(header, "#endif\n\n"); @@ -2156,18 +2148,18 @@ void write_header(const statement_list_t *stmts) fprintf(header, "#define __%s__\n\n", header_token); fprintf(header, "/* Forward declarations */\n\n"); - write_forward_decls(header, stmts); + write_forward_decls(header, ctx->statements); fprintf(header, "/* Headers for imported files */\n\n"); - write_imports(header, stmts); + write_imports(header, ctx->statements); fprintf(header, "\n"); start_cplusplus_guard(header); - write_header_stmts(header, stmts, NULL, FALSE); + write_header_stmts(header, ctx->statements, NULL, FALSE); fprintf(header, "/* Begin additional prototypes for all interfaces */\n"); fprintf(header, "\n"); - for_each_serializable(stmts, header, write_serialize_function_decl); + for_each_serializable(ctx->statements, header, write_serialize_function_decl); write_user_types(header); write_generic_handle_routines(header); write_context_handle_rundowns(header); diff --git a/tools/widl/parser.h b/tools/widl/parser.h index 47bcf4bc5db..5f6b8c091e8 100644 --- a/tools/widl/parser.h +++ b/tools/widl/parser.h @@ -21,7 +21,13 @@ #ifndef __WIDL_PARSER_H #define __WIDL_PARSER_H -int parser_parse(void); +struct idl_ctx +{ + const char *input; + statement_list_t *statements; +}; + +int parser_parse( struct idl_ctx *ctx ); extern FILE *parser_in; extern char *parser_text; diff --git a/tools/widl/parser.y b/tools/widl/parser.y index 6d8332a2423..044279f9906 100644 --- a/tools/widl/parser.y +++ b/tools/widl/parser.y @@ -124,6 +124,7 @@ static typelib_t *current_typelib; %code provides { +void parser_error( struct idl_ctx *ctx, const char *message ) __attribute__((noreturn)); int parser_lex( PARSER_STYPE *yylval ); } @@ -131,6 +132,7 @@ int parser_lex( PARSER_STYPE *yylval ); %define api.prefix {parser_} %define api.pure full %define parse.error verbose +%parse-param {struct idl_ctx *ctx} %union { attr_t *attr; @@ -361,15 +363,7 @@ int parser_lex( PARSER_STYPE *yylval ); input: gbl_statements m_acf { $1 = append_parameterized_type_stmts($1); check_statements($1, FALSE); check_all_user_types($1); - write_header($1); - write_id_data($1); - write_proxies($1); - write_client($1); - write_server($1); - write_regscript($1); - write_typelib_regscript($1); - write_dlldata($1); - write_local_stubs($1); + ctx->statements = $1; } ; diff --git a/tools/widl/proxy.c b/tools/widl/proxy.c index a80aa5d77fe..d878c2152ec 100644 --- a/tools/widl/proxy.c +++ b/tools/widl/proxy.c @@ -76,20 +76,19 @@ static void write_stubdesc(int expr_eval_routines) print_proxy( "\n"); } -static void init_proxy(const statement_list_t *stmts) +static void init_proxy( const struct idl_ctx *ctx ) { - if (proxy) return; - if(!(proxy = fopen(proxy_name, "w"))) - error("Could not open %s for output\n", proxy_name); - print_proxy( "/*** Autogenerated by WIDL %s from %s - Do not edit ***/\n", PACKAGE_VERSION, input_name); - print_proxy( "\n"); - print_proxy( "#define __midl_proxy\n"); - print_proxy( "#include \"objbase.h\"\n"); - print_proxy( "\n"); - print_proxy( "#ifndef DECLSPEC_HIDDEN\n"); - print_proxy( "#define DECLSPEC_HIDDEN\n"); - print_proxy( "#endif\n"); - print_proxy( "\n"); + if (proxy) return; + if (!(proxy = fopen( proxy_name, "w" ))) error( "Could not open %s for output\n", proxy_name ); + print_proxy( "/*** Autogenerated by WIDL %s from %s - Do not edit ***/\n", PACKAGE_VERSION, ctx->input ); + print_proxy( "\n"); + print_proxy( "#define __midl_proxy\n"); + print_proxy( "#include \"objbase.h\"\n"); + print_proxy( "\n"); + print_proxy( "#ifndef DECLSPEC_HIDDEN\n"); + print_proxy( "#define DECLSPEC_HIDDEN\n"); + print_proxy( "#endif\n"); + print_proxy( "\n"); } static void clear_output_vars( const var_list_t *args ) @@ -1042,14 +1041,14 @@ static void write_proxy_routines(const statement_list_t *stmts) fprintf(proxy, "};\n"); } -void write_proxies(const statement_list_t *stmts) +void write_proxies( const struct idl_ctx *ctx ) { - if (!do_proxies) return; - if (do_everything && !need_proxy_file(stmts)) return; + if (!do_proxies) return; + if (do_everything && !need_proxy_file( ctx->statements )) return; - init_proxy(stmts); - if(!proxy) return; + init_proxy( ctx ); + if (!proxy) return; - write_proxy_routines( stmts ); - fclose(proxy); + write_proxy_routines( ctx->statements ); + fclose( proxy ); } diff --git a/tools/widl/register.c b/tools/widl/register.c index 6c00dfaf5a2..39239cef556 100644 --- a/tools/widl/register.c +++ b/tools/widl/register.c @@ -240,12 +240,12 @@ static void write_progids( const statement_list_t *stmts ) } } -void write_regscript( const statement_list_t *stmts ) +void write_regscript( const struct idl_ctx *ctx ) { const type_t *ps_factory; if (!do_regscript) return; - if (do_everything && !need_proxy_file( stmts )) return; + if (do_everything && !need_proxy_file( ctx->statements )) return; init_output_buffer(); @@ -261,7 +261,7 @@ void write_regscript( const statement_list_t *stmts ) put_str( indent++, "{\n" ); put_str( indent, "NoRemove ActivatableClassId\n" ); put_str( indent++, "{\n" ); - write_runtimeclasses_registry( stmts ); + write_runtimeclasses_registry( ctx->statements ); put_str( --indent, "}\n" ); put_str( --indent, "}\n" ); put_str( --indent, "}\n" ); @@ -275,16 +275,16 @@ void write_regscript( const statement_list_t *stmts ) put_str( indent, "NoRemove Interface\n" ); put_str( indent++, "{\n" ); - ps_factory = find_ps_factory( stmts ); - if (ps_factory) write_interfaces( stmts, ps_factory ); + ps_factory = find_ps_factory( ctx->statements ); + if (ps_factory) write_interfaces( ctx->statements, ps_factory ); put_str( --indent, "}\n" ); put_str( indent, "NoRemove CLSID\n" ); put_str( indent++, "{\n" ); - write_coclasses( stmts, NULL ); + write_coclasses( ctx->statements, NULL ); put_str( --indent, "}\n" ); - write_progids( stmts ); + write_progids( ctx->statements ); put_str( --indent, "}\n" ); } @@ -304,13 +304,13 @@ void write_regscript( const statement_list_t *stmts ) } } -void write_typelib_regscript( const statement_list_t *stmts ) +void write_typelib_regscript( const struct idl_ctx *ctx ) { const statement_t *stmt; unsigned int count = 0; if (!do_typelib) return; - if (stmts) LIST_FOR_EACH_ENTRY( stmt, stmts, const statement_t, entry ) + if (ctx->statements) LIST_FOR_EACH_ENTRY( stmt, ctx->statements, const statement_t, entry ) { if (stmt->type != STMT_LIBRARY) continue; if (count && !strendswith( typelib_name, ".res" )) diff --git a/tools/widl/server.c b/tools/widl/server.c index 1d8ab41185d..0c2c573d6a2 100644 --- a/tools/widl/server.c +++ b/tools/widl/server.c @@ -441,19 +441,16 @@ static void write_serverinterfacedecl(type_t *iface) fprintf(server, "\n"); } - -static void init_server(void) +static void init_server( const struct idl_ctx *ctx ) { - if (server) - return; - if (!(server = fopen(server_name, "w"))) - error("Could not open %s for output\n", server_name); - - print_server("/*** Autogenerated by WIDL %s from %s - Do not edit ***/\n", PACKAGE_VERSION, input_name); - print_server("#include <string.h>\n"); - fprintf(server, "\n"); - print_server("#include \"%s\"\n", header_name); - print_server("\n"); + if (server) return; + if (!(server = fopen( server_name, "w" ))) error( "Could not open %s for output\n", server_name ); + + print_server( "/*** Autogenerated by WIDL %s from %s - Do not edit ***/\n", PACKAGE_VERSION, ctx->input ); + print_server( "#include <string.h>\n" ); + fprintf( server, "\n" ); + print_server( "#include \"%s\"\n", header_name ); + print_server( "\n" ); print_server( "#ifndef DECLSPEC_HIDDEN\n"); print_server( "#define DECLSPEC_HIDDEN\n"); print_server( "#endif\n"); @@ -537,17 +534,14 @@ static void write_server_routines(const statement_list_t *stmts) write_typeformatstring(server, stmts, need_stub); } -void write_server(const statement_list_t *stmts) +void write_server( const struct idl_ctx *ctx ) { - if (!do_server) - return; - if (do_everything && !need_stub_files(stmts)) - return; + if (!do_server) return; + if (do_everything && !need_stub_files( ctx->statements )) return; - init_server(); - if (!server) - return; + init_server( ctx ); + if (!server) return; - write_server_routines( stmts ); - fclose(server); + write_server_routines( ctx->statements ); + fclose( server ); } diff --git a/tools/widl/utils.c b/tools/widl/utils.c index aad40f6b087..93ff5f35e8a 100644 --- a/tools/widl/utils.c +++ b/tools/widl/utils.c @@ -76,7 +76,7 @@ void error_loc(const char *s, ...) } /* yyerror: yacc assumes this is not newline terminated. */ -void parser_error(const char *s) +void parser_error( struct idl_ctx *ctx, const char *s ) { error_loc("%s\n", s); } diff --git a/tools/widl/utils.h b/tools/widl/utils.h index f042f0e064c..0d7fe306fe5 100644 --- a/tools/widl/utils.h +++ b/tools/widl/utils.h @@ -22,8 +22,8 @@ #define __WIDL_UTILS_H #include "widltypes.h" +#include "parser.h" -void parser_error(const char *s) __attribute__((noreturn)); int parser_warning(const char *s, ...) __attribute__((format (printf, 1, 2))); void error_loc(const char *s, ...) __attribute__((format (printf, 1, 2))) __attribute__((noreturn)); void error(const char *s, ...) __attribute__((format (printf, 1, 2))) __attribute__((noreturn)); diff --git a/tools/widl/widl.c b/tools/widl/widl.c index 7b2276b0a41..84cb30ac7a4 100644 --- a/tools/widl/widl.c +++ b/tools/widl/widl.c @@ -38,6 +38,7 @@ #include "widl.h" #include "utils.h" #include "parser.h" +#include "parser.tab.h" #include "wpp_private.h" #include "header.h" @@ -327,16 +328,15 @@ static char *eat_space(char *s) return s; } -void write_dlldata(const statement_list_t *stmts) +void write_dlldata( const struct idl_ctx *ctx ) { struct strarray filenames = empty_strarray; int define_proxy_delegation = 0; FILE *dlldata; - if (!do_dlldata || !need_proxy_file(stmts)) - return; + if (!do_dlldata || !need_proxy_file( ctx->statements )) return; - define_proxy_delegation = need_proxy_delegation(stmts); + define_proxy_delegation = need_proxy_delegation( ctx->statements ); dlldata = fopen(dlldata_name, "r"); if (dlldata) { @@ -429,7 +429,7 @@ static void write_id_data_stmts(const statement_list_t *stmts) } } -void write_id_data(const statement_list_t *stmts) +static void write_id_data( const struct idl_ctx *ctx ) { if (!do_idfile) return; @@ -470,7 +470,7 @@ void write_id_data(const statement_list_t *stmts) fprintf(idfile, "#endif\n\n"); start_cplusplus_guard(idfile); - write_id_data_stmts(stmts); + write_id_data_stmts( ctx->statements ); fprintf(idfile, "\n"); end_cplusplus_guard(idfile); @@ -715,6 +715,7 @@ int open_typelib( const char *name ) int main(int argc,char *argv[]) { + struct idl_ctx ctx = {0}; int i; int ret = 0; struct strarray files; @@ -805,7 +806,7 @@ int main(int argc,char *argv[]) return 1; } else - input_idl_name = input_name = xstrdup(files.str[0]); + ctx.input = input_idl_name = input_name = xstrdup(files.str[0]); } else { fprintf(stderr, "%s", usage); @@ -826,25 +827,25 @@ int main(int argc,char *argv[]) (debuglevel & DEBUGLEVEL_PPMSG) != 0 ); if (!header_name) - header_name = replace_extension( get_basename(input_name), ".idl", ".h" ); + header_name = replace_extension( get_basename( ctx.input ), ".idl", ".h" ); if (!typelib_name && do_typelib) - typelib_name = replace_extension( get_basename(input_name), ".idl", ".tlb" ); + typelib_name = replace_extension( get_basename( ctx.input ), ".idl", ".tlb" ); if (!proxy_name && do_proxies) - proxy_name = replace_extension( get_basename(input_name), ".idl", "_p.c" ); + proxy_name = replace_extension( get_basename( ctx.input ), ".idl", "_p.c" ); if (!client_name && do_client) - client_name = replace_extension( get_basename(input_name), ".idl", "_c.c" ); + client_name = replace_extension( get_basename( ctx.input ), ".idl", "_c.c" ); if (!server_name && do_server) - server_name = replace_extension( get_basename(input_name), ".idl", "_s.c" ); + server_name = replace_extension( get_basename( ctx.input ), ".idl", "_s.c" ); if (!regscript_name && do_regscript) - regscript_name = replace_extension( get_basename(input_name), ".idl", "_r.rgs" ); + regscript_name = replace_extension( get_basename( ctx.input ), ".idl", "_r.rgs" ); if (!idfile_name && do_idfile) - idfile_name = replace_extension( get_basename(input_name), ".idl", "_i.c" ); + idfile_name = replace_extension( get_basename( ctx.input ), ".idl", "_i.c" ); if (do_proxies) proxy_token = dup_basename_token(proxy_name,"_p.c"); if (do_client) client_token = dup_basename_token(client_name,"_c.c"); @@ -895,7 +896,7 @@ int main(int argc,char *argv[]) header_token = make_token(header_name); init_types(); - ret = parser_parse(); + ret = parser_parse( &ctx ); fclose(parser_in); @@ -903,6 +904,16 @@ int main(int argc,char *argv[]) exit(1); } + write_header( &ctx ); + write_id_data( &ctx ); + write_proxies( &ctx ); + write_client( &ctx ); + write_server( &ctx ); + write_regscript( &ctx ); + write_typelib_regscript( &ctx ); + write_dlldata( &ctx ); + write_local_stubs( &ctx ); + /* Everything has been done successfully, don't delete any files. */ set_everything(FALSE); local_stubs_name = NULL; diff --git a/tools/widl/widl.h b/tools/widl/widl.h index c63882108a8..3576f028d19 100644 --- a/tools/widl/widl.h +++ b/tools/widl/widl.h @@ -84,16 +84,16 @@ enum stub_mode extern enum stub_mode get_stub_mode(void); extern int open_typelib( const char *name ); -extern void write_header(const statement_list_t *stmts); -extern void write_id_data(const statement_list_t *stmts); -extern void write_proxies(const statement_list_t *stmts); -extern void write_client(const statement_list_t *stmts); -extern void write_server(const statement_list_t *stmts); -extern void write_regscript(const statement_list_t *stmts); -extern void write_typelib_regscript(const statement_list_t *stmts); +struct idl_ctx; +extern void write_header( const struct idl_ctx *ctx ); +extern void write_proxies( const struct idl_ctx *ctx ); +extern void write_client( const struct idl_ctx *ctx ); +extern void write_server( const struct idl_ctx *ctx ); +extern void write_regscript( const struct idl_ctx *ctx ); +extern void write_typelib_regscript( const struct idl_ctx *ctx ); extern void output_typelib_regscript( const typelib_t *typelib ); -extern void write_local_stubs(const statement_list_t *stmts); -extern void write_dlldata(const statement_list_t *stmts); +extern void write_local_stubs( const struct idl_ctx *ctx ); +extern void write_dlldata( const struct idl_ctx *ctx ); extern void start_cplusplus_guard(FILE *fp); extern void end_cplusplus_guard(FILE *fp); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/570
From: R��mi Bernon <rbernon(a)codeweavers.com> --- tools/widl/parser.l | 27 +++++++++++++-------------- 1 file changed, 13 insertions(+), 14 deletions(-) diff --git a/tools/widl/parser.l b/tools/widl/parser.l index 1493e2d6ad7..de8cae93b2f 100644 --- a/tools/widl/parser.l +++ b/tools/widl/parser.l @@ -90,6 +90,13 @@ struct { } import_stack[MAX_IMPORT_DEPTH]; int import_stack_ptr = 0; +struct import +{ + const char *name; + struct list entry; +}; +static struct list imports = LIST_INIT( imports ); + /* converts an integer in string form to an unsigned long and prints an error * on overflow */ static unsigned int xstrtoul(const char *nptr, char **endptr, int base) @@ -519,28 +526,20 @@ void pop_import(void) import_stack_ptr--; } -struct imports { - char *name; - struct imports *next; -} *first_import; - int do_import(char *fname) { FILE *f; char *path, *name; - struct imports *import; + struct import *import; int ptr = import_stack_ptr; int ret, fd; - import = first_import; - while (import && strcmp(import->name, fname)) - import = import->next; - if (import) return 0; /* already imported */ + LIST_FOR_EACH_ENTRY( import, &imports, struct import, entry ) + if (!strcmp( import->name, fname )) return 0; /* already imported */ - import = xmalloc(sizeof(struct imports)); - import->name = xstrdup(fname); - import->next = first_import; - first_import = import; + import = xmalloc( sizeof(struct import) ); + import->name = xstrdup( fname ); + list_add_tail( &imports, &import->entry ); /* don't search for a file name with a path in the include directories, * for compatibility with MIDL */ -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/570
From: R��mi Bernon <rbernon(a)codeweavers.com> --- tools/widl/parser.h | 2 +- tools/widl/parser.l | 32 ++++++++++++++++++++------------ tools/widl/parser.y | 15 ++++----------- 3 files changed, 25 insertions(+), 24 deletions(-) diff --git a/tools/widl/parser.h b/tools/widl/parser.h index 5f6b8c091e8..270db49189b 100644 --- a/tools/widl/parser.h +++ b/tools/widl/parser.h @@ -35,7 +35,7 @@ extern int parser_debug; extern int yy_flex_debug; extern int import_stack_ptr; -int do_import(char *fname); +void push_import(char *fname); void abort_import(void); void pop_import(void); diff --git a/tools/widl/parser.l b/tools/widl/parser.l index de8cae93b2f..6fd1b85ee19 100644 --- a/tools/widl/parser.l +++ b/tools/widl/parser.l @@ -513,7 +513,7 @@ void pop_import(void) { int ptr = import_stack_ptr-1; - fclose(yyin); + if (yyin) fclose( yyin ); yy_delete_buffer( YY_CURRENT_BUFFER ); yy_switch_to_buffer( import_stack[ptr].state ); if (temp_name) { @@ -526,7 +526,7 @@ void pop_import(void) import_stack_ptr--; } -int do_import(char *fname) +void push_import(char *fname) { FILE *f; char *path, *name; @@ -534,8 +534,25 @@ int do_import(char *fname) int ptr = import_stack_ptr; int ret, fd; + if (import_stack_ptr == MAX_IMPORT_DEPTH) + error_loc("Exceeded max import depth\n"); + + import_stack[ptr].state = YY_CURRENT_BUFFER; + import_stack[ptr].temp_name = temp_name; + import_stack[ptr].input_name = input_name; + import_stack[ptr].line_number = line_number; + import_stack_ptr++; + temp_name = NULL; + LIST_FOR_EACH_ENTRY( import, &imports, struct import, entry ) - if (!strcmp( import->name, fname )) return 0; /* already imported */ + { + if (!strcmp( import->name, fname )) + { + /* already imported, scan an empty buffer for <<EOF>> */ + yy_scan_string( "" ); + return; + } + } import = xmalloc( sizeof(struct import) ); import->name = xstrdup( fname ); @@ -548,13 +565,6 @@ int do_import(char *fname) else if (!(path = wpp_find_include( fname, input_name ))) error_loc("Unable to open include file %s\n", fname); - if (import_stack_ptr == MAX_IMPORT_DEPTH) - error_loc("Exceeded max import depth\n"); - - import_stack[ptr].temp_name = temp_name; - import_stack[ptr].input_name = input_name; - import_stack[ptr].line_number = line_number; - import_stack_ptr++; input_name = path; line_number = 1; @@ -570,9 +580,7 @@ int do_import(char *fname) if((f = fopen(temp_name, "r")) == NULL) error_loc("Unable to open %s\n", temp_name); - import_stack[ptr].state = YY_CURRENT_BUFFER; yy_switch_to_buffer(yy_create_buffer(f, YY_BUF_SIZE)); - return 1; } void abort_import(void) diff --git a/tools/widl/parser.y b/tools/widl/parser.y index 044279f9906..9227fd633ef 100644 --- a/tools/widl/parser.y +++ b/tools/widl/parser.y @@ -333,7 +333,7 @@ int parser_lex( PARSER_STYPE *yylval ); %type <str> libraryhdr callconv cppquote importlib import %type <str> typename m_typename %type <uuid> uuid_string -%type <import> import_start +%type <str> import_start %type <typelib> library_start librarydef %type <statement> statement typedef pragma_warning %type <stmt_list> gbl_statements imp_statements int_statements @@ -486,17 +486,10 @@ typedecl: cppquote: tCPPQUOTE '(' aSTRING ')' { $$ = $3; } ; -import_start: tIMPORT aSTRING ';' { $$ = xmalloc(sizeof(struct _import_t)); - $$->name = $2; - $$->import_performed = do_import($2); - if (!$$->import_performed) yychar = aEOF; - } - ; -import: import_start imp_statements aEOF { $$ = $1->name; - if ($1->import_performed) pop_import(); - free($1); - } +import_start: tIMPORT aSTRING ';' { $$ = $2; push_import($2); } + ; +import: import_start imp_statements aEOF { pop_import(); } ; importlib: tIMPORTLIB '(' aSTRING ')' -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/570
From: R��mi Bernon <rbernon(a)codeweavers.com> --- tools/widl/parser.h | 4 +- tools/widl/parser.l | 91 ++++++++++++++++++++++++--------------------- tools/widl/widl.c | 1 + 3 files changed, 50 insertions(+), 46 deletions(-) diff --git a/tools/widl/parser.h b/tools/widl/parser.h index 270db49189b..0c661b55e85 100644 --- a/tools/widl/parser.h +++ b/tools/widl/parser.h @@ -34,13 +34,11 @@ extern char *parser_text; extern int parser_debug; extern int yy_flex_debug; -extern int import_stack_ptr; +extern int parse_only; void push_import(char *fname); void abort_import(void); void pop_import(void); -#define parse_only import_stack_ptr - int is_type(const char *name); int do_warning(const char *toggle, warning_list_t *wnum); diff --git a/tools/widl/parser.l b/tools/widl/parser.l index 6fd1b85ee19..8016bdc1fcc 100644 --- a/tools/widl/parser.l +++ b/tools/widl/parser.l @@ -81,14 +81,16 @@ static void switch_to_acf(void); static warning_list_t *disabled_warnings = NULL; -#define MAX_IMPORT_DEPTH 20 -struct { - YY_BUFFER_STATE state; - char *input_name; - int line_number; - char *temp_name; -} import_stack[MAX_IMPORT_DEPTH]; -int import_stack_ptr = 0; +struct import_state +{ + YY_BUFFER_STATE buffer; + char *input_name; + int line_number; + char *temp_name; + struct list entry; +}; +static struct list import_stack = LIST_INIT( import_stack ); +int parse_only = 0; struct import { @@ -160,7 +162,8 @@ struct uuid *parse_uuid(const char *u) } <PP_PRAGMA>midl_echo[^\n]* yyless(9); yy_pop_state(); return tCPPQUOTE; <PP_PRAGMA>winrt[^\n]* { - if(import_stack_ptr) { + if (!list_empty( &import_stack )) + { if(!winrt_mode) error_loc("winrt IDL file imported in non-winrt mode\n"); }else { @@ -235,7 +238,7 @@ SAFEARRAY{ws}*/\( return tSAFEARRAY; <INITIAL,ATTR>\.\.\. return ELLIPSIS; <INITIAL,ATTR>. return yytext[0]; <<EOF>> { - if (import_stack_ptr) + if (!list_empty( &import_stack )) return aEOF; if (acf_name) { @@ -511,37 +514,44 @@ static char *get_buffered_cstring(void) void pop_import(void) { - int ptr = import_stack_ptr-1; - - if (yyin) fclose( yyin ); - yy_delete_buffer( YY_CURRENT_BUFFER ); - yy_switch_to_buffer( import_stack[ptr].state ); - if (temp_name) { - unlink(temp_name); - free(temp_name); - } - temp_name = import_stack[ptr].temp_name; - input_name = import_stack[ptr].input_name; - line_number = import_stack[ptr].line_number; - import_stack_ptr--; + struct list *entry = list_head( &import_stack ); + struct import_state *state; + + assert(entry); + + state = LIST_ENTRY( entry, struct import_state, entry ); + list_remove( &state->entry ); + parse_only = !list_empty( &import_stack ); + + if (yyin) fclose( yyin ); + yy_delete_buffer( YY_CURRENT_BUFFER ); + if (temp_name) unlink( temp_name ); + free( temp_name ); + + temp_name = state->temp_name; + input_name = state->input_name; + line_number = state->line_number; + yy_switch_to_buffer( state->buffer ); + free( state ); } void push_import(char *fname) { + struct import_state *state; FILE *f; char *path, *name; struct import *import; - int ptr = import_stack_ptr; int ret, fd; - if (import_stack_ptr == MAX_IMPORT_DEPTH) - error_loc("Exceeded max import depth\n"); + state = xmalloc( sizeof(struct import_state )); + list_add_head( &import_stack, &state->entry ); + parse_only = !list_empty( &import_stack ); - import_stack[ptr].state = YY_CURRENT_BUFFER; - import_stack[ptr].temp_name = temp_name; - import_stack[ptr].input_name = input_name; - import_stack[ptr].line_number = line_number; - import_stack_ptr++; + state->buffer = YY_CURRENT_BUFFER; + state->temp_name = temp_name; + state->input_name = input_name; + state->line_number = line_number; + input_name = NULL; temp_name = NULL; LIST_FOR_EACH_ENTRY( import, &imports, struct import, entry ) @@ -585,29 +595,25 @@ void push_import(char *fname) void abort_import(void) { - int ptr; - - for (ptr=0; ptr<import_stack_ptr; ptr++) - unlink(import_stack[ptr].temp_name); + while (!list_empty( &import_stack )) pop_import(); } static void switch_to_acf(void) { - int ptr = import_stack_ptr; int ret, fd; - char *name; FILE *f; - assert(import_stack_ptr == 0); + if (yyin) fclose( yyin ); + yy_delete_buffer( YY_CURRENT_BUFFER ); + if (temp_name) unlink( temp_name ); + free( temp_name ); input_name = acf_name; acf_name = NULL; line_number = 1; - fd = make_temp_file( "widl-acf", NULL, &name ); - temp_name = name; - if (!(f = fdopen(fd, "wt"))) - error("Could not open fd %s for writing\n", name); + fd = make_temp_file( "widl-acf", NULL, &temp_name ); + if (!(f = fdopen( fd, "wt" ))) error( "Could not open fd %s for writing\n", temp_name ); ret = wpp_parse(input_name, f); fclose(f); @@ -616,7 +622,6 @@ static void switch_to_acf(void) if((f = fopen(temp_name, "r")) == NULL) error_loc("Unable to open %s\n", temp_name); - import_stack[ptr].state = YY_CURRENT_BUFFER; yy_switch_to_buffer(yy_create_buffer(f, YY_BUF_SIZE)); } diff --git a/tools/widl/widl.c b/tools/widl/widl.c index 84cb30ac7a4..c339fc30e98 100644 --- a/tools/widl/widl.c +++ b/tools/widl/widl.c @@ -898,6 +898,7 @@ int main(int argc,char *argv[]) init_types(); ret = parser_parse( &ctx ); + abort_import(); fclose(parser_in); if(ret) { -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/570
On Mon Aug 1 16:34:00 2022 +0000, **** wrote:
Marvin replied on the mailing list: ``` Hi, It looks like your patch introduced the new failures shown below. Please investigate and fix them before resubmitting your patch. If they are not new, fixing them anyway would help a lot. Otherwise please ask for the known failures list to be updated. The full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=120320 Your paranoid android. === debian11 (build log) === tools/widl/parser.tab.h:322:5: error: conflicting types for ���parser_parse��� ../wine/tools/widl/widl.c:899:9: error: too many arguments to function ���parser_parse��� Task: The win32 Wine build failed === debian11 (build log) === tools/widl/parser.tab.h:322:5: error: conflicting types for ���parser_parse��� ../wine/tools/widl/widl.c:899:9: error: too many arguments to function ���parser_parse��� Task: The wow64 Wine build failed ``` I'm not sure what's wrong here, I'm trying to do the same as the hlsl parser, but it looks like that it didn't regenerate parser.tab.h, or that some bison version difference make it not use the `%parse-param` directive?
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/570#note_5434
While cleanup seems nice, I'm not sure I follow why separating parser and lexer would help testing. I think that testing widl itself is more interesting and it's as simple as adding an interesting IDL file to a regular Wine test. My understanding is that you'd like an easy way to validate test data against midl and you'd like to write midl.exe launcher for that. But that part could be just skipped on Wine, assuming that an equivalent thing happened during the process of building the test itself. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/570#note_5441
On Mon Aug 1 18:01:43 2022 +0000, Jacek Caban wrote:
While cleanup seems nice, I'm not sure I follow why separating parser and lexer would help testing. I think that testing widl itself is more interesting and it's as simple as adding an interesting IDL file to a regular Wine test. My understanding is that you'd like an easy way to validate test data against midl and you'd like to write midl.exe launcher for that. But that part could be just skipped on Wine, assuming that an equivalent thing happened during the process of building the test itself. I think that in order for the parser to be more robust, it would be nice to test invalid IDL code too.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/570#note_5445
On Mon Aug 1 18:01:43 2022 +0000, R��mi Bernon wrote:
I think that in order for the parser to be more robust, it would be nice to test invalid IDL code too. And imho winetest-like tests makes it easier to see what gets implemented and what is still not implemented.
Yes, we can consider that the set of IDL code used by Wine is already a good enough test case and any severe breakage will break the build first, but if the build isn't broken already, it's still hard to tell the effect of some widl change before it gets used. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/570#note_5447
On Mon Aug 1 18:13:46 2022 +0000, R��mi Bernon wrote:
And imho winetest-like tests makes it easier to see what gets implemented and what is still not implemented. Yes, we can consider that the set of IDL code used by Wine is already a good enough test case and any severe breakage will break the build first, but if the build isn't broken already, it's still hard to tell the effect of some widl change before it gets used. Maybe we could have a special "test" mode in widl (with #widl pragma or a command line option), which we could use to instruct widl to "expect failure" (and revert return code, probably silence or redirect output). The same could be used to trigger complete build (including proxy, stubs, typelibs, iids, depending on what IDL provides), but redirecting it all to /dev/null.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/570#note_5449
On Mon Aug 1 18:45:08 2022 +0000, Jacek Caban wrote:
Maybe we could have a special "test" mode in widl (with #widl pragma or a command line option), which we could use to instruct widl to "expect failure" (and revert return code, probably silence or redirect output). The same could be used to trigger complete build (including proxy, stubs, typelibs, iids, depending on what IDL provides), but redirecting it all to /dev/null. Well, I think it sounds very much an ad-hoc solution to solve a problem for which we have all the necessary tools already with winetest.
I also believe that having a builtin midl.exe, command-line compatible with native midl.exe, may be interesting for some use cases. I intend to make it as much compatible as the basic tests will require it to be, but it could be later extended. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/570#note_5455
On Mon Aug 1 20:12:23 2022 +0000, R��mi Bernon wrote:
Well, I think it sounds very much an ad-hoc solution to solve a problem for which we have all the necessary tools already with winetest. I also believe that having a builtin midl.exe, command-line compatible with native midl.exe, may be interesting for some use cases. I intend to make it as much compatible as the basic tests will require it to be, but it could be later extended. What interesting use cases do you mean? Note that one may just build widl as PE if it's just for sake of running widl on Windows. So far you only mentioned being able to test parser failures.
I think that it would be generally more interesting to make widl itself more compatible with midl. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/570#note_5457
On Mon Aug 1 20:33:16 2022 +0000, Jacek Caban wrote:
What interesting use cases do you mean? Note that one may just build widl as PE if it's just for sake of running widl on Windows. So far you only mentioned being able to test parser failures. I think that it would be generally more interesting to make widl itself more compatible with midl. Well for instance having an open-source, drop-in replacement alternative to midl.exe.
I think that it would be generally more interesting to make widl itself more compatible with midl.
Yes, I agree, though then it'd possibly break backward compatibility. I'm not sure we want that as widl is used publicly elsewhere. Having a builtin midl.exe (and possibly a Unix version of it) would be a better way to get a command-line compatible tool, improving widl at the same time for things that do not break backward compatibility, but eventually deprecating it once the other is in good shape. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/570#note_6724
On Tue Aug 23 09:27:43 2022 +0000, R��mi Bernon wrote:
Well for instance having an open-source, drop-in replacement alternative to midl.exe.
I think that it would be generally more interesting to make widl itself more compatible with midl. Yes, I agree, though then it'd possibly break backward compatibility. I'm not sure we want that as widl is used publicly elsewhere. Having a builtin midl.exe (and possibly a Unix version of it) would be a better way to get a command-line compatible tool, improving widl at the same time for things that do not break backward compatibility, but eventually deprecating it once the other is in good shape. widl can already be used as a drop-in midl replacement in some cases, for example when building Firefox (and we recently started using it for Gecko). It works because I fixed compatibility in required aspects, see commits like 1db0ad9798f, 511b50e7bf, fc761cb9352d. I don't see why we can't support more options like that. If we run into a backward compatibility problem, we may always add a way to be more explicit about midl/widl compatibility mode. Do you have some specific problems in mind?
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/570#note_6734
On Tue Aug 23 11:06:56 2022 +0000, Jacek Caban wrote:
widl can already be used as a drop-in midl replacement in some cases, for example when building Firefox (and we recently started using it for Gecko). It works because I fixed compatibility in required aspects, see commits like 1db0ad9798f, 511b50e7bf, fc761cb9352d. I don't see why we can't support more options like that. If we run into a backward compatibility problem, we may always add a way to be more explicit about midl/widl compatibility mode. Do you have some specific problems in mind? I don't have anything specific in mind, just forward thinking about it as it doesn't look like widl was meant to be command-line compatible (it has some specific options and different names for some that are in midl).
In any case, I don't mind trying to make widl more compatible, then I don't think it makes much difference for the matter of testing it. For that, I think it makes sense to have a builtin exe version of widl, named either midl.exe or widl.exe, because that's how all the Wine conformance tests are done. The widl tests are conformance tests in the exact same way as the other Wine tests are: we want to compare native behavior with our own implementation. I don't think it makes any sense to have another testing mechanism just for widl. Then, the question becomes how do we build both a tool used for building Wine, and a PE version of it for testing purposes. I have a simple implementation for that, sharing mostly everything from tools/widl to program/midl with a PARENTSRC and a few small changes to the code. Maybe that's better than trying to share less first, and incrementally add features to the builtin? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/570#note_6738
On Tue Aug 23 11:19:29 2022 +0000, R��mi Bernon wrote:
I don't have anything specific in mind, just forward thinking about it as it doesn't look like widl was meant to be command-line compatible (it has some specific options and different names for some that are in midl). In any case, I don't mind trying to make widl more compatible, then I don't think it makes much difference for the matter of testing it. For that, I think it makes sense to have a builtin exe version of widl, named either midl.exe or widl.exe, because that's how all the Wine conformance tests are done. The widl tests are conformance tests in the exact same way as the other Wine tests are: we want to compare native behavior with our own implementation. I don't think it makes any sense to have another testing mechanism just for widl. Then, the question becomes how do we build both a tool used for building Wine, and a PE version of it for testing purposes. I have a simple implementation for that, sharing mostly everything from tools/widl to program/midl with a PARENTSRC and a few small changes to the code. Maybe that's better than trying to share less first, and incrementally add features to the builtin? Let me rephrase my previous question: what value does all that bring? Is there anything other than testing parser failures?
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/570#note_6741
On Tue Aug 23 11:29:18 2022 +0000, Jacek Caban wrote:
Let me rephrase my previous question: what value does all that bring? Is there anything other than testing parser failures? Well it could also test generated files for compatibility. It's a bit more tricky because it's probably not worth checking that it's 100% identical but it could probably be done with a bit of post processing.
In any case I think that testing parser successes and failures is already enough value to justify it. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/570#note_6742
On Tue Aug 23 11:32:46 2022 +0000, R��mi Bernon wrote:
Well it could also test generated files for compatibility. It's a bit more tricky because it's probably not worth checking that it's 100% identical but it could probably be done with a bit of post processing. In any case I think that testing parser successes and failures is already enough value to justify it. A nice way to test generated files is to use it somehow. For header files, it's easy to achieve by writing a C file that #includes it, and maybe uses it in some interesting ways. For typelibs, it requires loading and analyzing or using them. We already have that in oleaut32 tests. The only thing we don't have is easy validating that data against midl, but, at least for typelibs, it could be done by extending existing tests.
For testing parser success it's as easy as adding the interesting bits to some IDL file in tests, it's not a new ability that we'd gain from PE midl.exe. That leaves only parser failures and I don't find it a good enough justification, especially when there are other ways to achieve that. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/570#note_6758
On Tue Aug 23 12:04:26 2022 +0000, Jacek Caban wrote:
A nice way to test generated files is to use it somehow. For header files, it's easy to achieve by writing a C file that #includes it, and maybe uses it in some interesting ways. For typelibs, it requires loading and analyzing or using them. We already have that in oleaut32 tests. The only thing we don't have is easy validating that data against midl, but, at least for typelibs, it could be done by extending existing tests. For testing parser success it's as easy as adding the interesting bits to some IDL file in tests, it's not a new ability that we'd gain from PE midl.exe. That leaves only parser failures and I don't find it a good enough justification, especially when there are other ways to achieve that. It's not because including the C file works in a test or when used to build Wine that it's compatible with the one midl would generate. We compile all the tests with widl, so we only test that these files are usable in our tests, and only to the extend of how the tests and Wine code are using them.
We can and probably already have differences that makes midl generated headers incompatible with widl generated ones. We also don't test anything about the C++ generated code, which has been and may still be broken, and I don't think we will add C++ compilation support just for that. I honestly find it quite hard to have to argue for adding more tests in Wine, especially to an area that has current none, and to which contributing has been historically very hard in large part because of the difficulty to foresee the effect of any change. If that's how it is I won't insist, but I'm also probably not going to be very eager to work on widl either. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/570#note_6759
This merge request was closed by R��mi Bernon. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/570
On Tue Aug 23 12:26:19 2022 +0000, R��mi Bernon wrote:
It's not because including the C file works in a test or when used to build Wine that it's compatible with the one midl would generate. We compile all the tests with widl, so we only test that these files are usable in our tests, and only to the extend of how the tests and Wine code are using them. We can and probably already have differences that makes midl generated headers incompatible with widl generated ones. We also don't test anything about the C++ generated code, which has been and may still be broken, and I don't think we will add C++ compilation support just for that. I honestly find it quite hard to have to argue for adding more tests in Wine, especially to an area that has current none, and to which contributing has been historically very hard in large part because of the difficulty to foresee the effect of any change. If that's how it is I won't insist, but I'm also probably not going to be very eager to work on widl either. If you want to test generated files, the right way is for someone to install the PSDK on Windows, generate files with midl, and use that as test sources, similar to the existing generated.c files.
Adding a new midl.exe just for the purpose of running tests doesn't make much sense, especially since the Windows VMs don't have the PSDK so the tests will never run there. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/570#note_6760
On Tue Aug 23 12:48:03 2022 +0000, Alexandre Julliard wrote:
If you want to test generated files, the right way is for someone to install the PSDK on Windows, generate files with midl, and use that as test sources, similar to the existing generated.c files. Adding a new midl.exe just for the purpose of running tests doesn't make much sense, especially since the Windows VMs don't have the PSDK so the tests will never run there. It was indeed my intention to depend on the PSDK being installed. Wouldn't it be possible?
I don't understand the reasoning here, unless there's some legal matters. Having the files generated manually is a workaround, but it's cumbersome and I think it'd better if we could run the tools while the tests are run, eventually saving and committing the results if needed. It also makes sure we keep the commands to generate them committed and tested, avoiding bit rot and being unable to regenerate them later. This is basically the same as we do elsewhere, for mscoree tests for instance, where we run csc, or for multimedia tests where some are using native tools to generate sample media files to workaround issues with open-source encoders (it's not always done but imho it's superior to using external tools and manual steps to regenerate the files). -- https://gitlab.winehq.org/wine/wine/-/merge_requests/570#note_6761
On Tue Aug 23 13:18:51 2022 +0000, R��mi Bernon wrote:
It was indeed my intention to depend on the PSDK being installed. Wouldn't it be possible? I don't understand the reasoning here, unless there's some legal matters. Having the files generated manually is a workaround, but it's cumbersome and I think it'd better if we could run the tools while the tests are run, eventually saving and committing the results if needed. It also makes sure we keep the commands to generate them committed and tested, avoiding bit rot and being unable to regenerate them later. This is basically the same as we do elsewhere, for mscoree tests for instance, where we run csc, or for multimedia tests where some are using native tools to generate sample media files to workaround issues with open-source encoders (it's not always done but imho it's superior to using external tools and manual steps to regenerate the files). It's also not only about parser issues. For IDL 3.0 we need to test stuff like computed IIDs and ABI name generation.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/570#note_6762
If you want to test generated files, the right way is for someone to install the PSDK on Windows, generate files with midl, and use that as test sources, similar to the existing generated.c files.
FWIW there's several checks failing in several of these files when I try to build them with the latest MSVC version https://gcc.godbolt.org has (v19.32). Arguably some of these failures are caused by the addition of new struct members, but there's also some type alignment mismatch, some unknown types and some misnamed fields. I think it's still a sign that not routinely testing these things and using manually generated files makes it not very reliable and tends to rot. Then it indeed means that the testbot needs to have the PSDK and build tools installed on some VMs, updated from time to time (and it raises a question to how to handle SDK versions, but at least one official version would be good), and that the tests should probably be changed to compile them with MSVC during the test. I think that having all this would also make public header changes validation easier. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/570#note_6769
participants (4)
-
Alexandre Julliard (＠julliard) -
Bernhard Kölbl
-
Jacek Caban (＠jacek)
-
Rémi Bernon