Re: [PATCH 1/3] winebuild: Added support for entry points generated in runtime.

[Sebastian Lackner]

On 19.01.2016 12:20, Jacek Caban wrote:
> Signed-off-by: Jacek Caban
>
> tools/winebuild/build.h          |  1 +
>  tools/winebuild/parser.c         |  1 +
>  tools/winebuild/spec32.c         | 59
> ++++++++++++++++++++++++++++++++++++++++
>  tools/winebuild/winebuild.man.in |  3 ++
>  4 files changed, 64 insertions(+)
>
> 0001-winebuild-Added-support-for-entry-points-generated-in.diff
>
> diff --git a/tools/winebuild/build.h b/tools/winebuild/build.h
> index e32e7a9..2d5a162 100644
> --- a/tools/winebuild/build.h
> +++ b/tools/winebuild/build.h
> @@ -171,6 +171,7 @@ struct strarray
>  #define FLAG_REGISTER  0x10  /* use register calling convention */
>  #define FLAG_PRIVATE   0x20  /* function is private (cannot be imported) */
>  #define FLAG_ORDINAL   0x40  /* function should be imported by ordinal */
> +#define FLAG_JIT       0x80  /* function is generated at runtime */
>
> #define FLAG_FORWARD   0x100  /* function is a forwarded name */
>  #define FLAG_EXT_LINK  0x200  /* function links to an external symbol */
> diff --git a/tools/winebuild/parser.c b/tools/winebuild/parser.c
> index 1d7b84e..c42fceb 100644
> --- a/tools/winebuild/parser.c
> +++ b/tools/winebuild/parser.c
> @@ -69,6 +69,7 @@ static const char * const FlagNames[] =
>      "register",    /* FLAG_REGISTER */
>      "private",     /* FLAG_PRIVATE */
>      "ordinal",     /* FLAG_ORDINAL */
> - "jit",         /* FLAG_JIT */
> NULL
>
> };
>
> diff --git a/tools/winebuild/spec32.c b/tools/winebuild/spec32.c
> index cb56abe..ebce6b4 100644
> --- a/tools/winebuild/spec32.c
> +++ b/tools/winebuild/spec32.c
> @@ -270,6 +270,60 @@ static void output_relay_debug( DLLSPEC *spec )
>      }
>  }
>
> +static void output_jit_descs( DLLSPEC *spec )
> +{
> - int i, emited_descs = 0, jit_cnt = 0;

No native english speaker here, but shouldn't it be "emitted" ?
> - char **jit_entries, *sym_name;
> - ORDDEF *odp;
> -
> - if (target_cpu != CPU_x86)
> -
>    return;
> -
> - for (odp = spec->entry_points; odp < spec->entry_points + spec->nb_entry_points; odp++)
> -
>    if (odp->flags & FLAG_JIT)
> -
>        jit_cnt++;

This code should probably ignore spec entries with FLAG_FORWARD.
Generating thunks wouldn't make sense then. Also, winebuild should
probably complain when non-stdcall functions use -jit.
> - if (!jit_cnt)
> -
>    return;
> -
> - jit_entries = xmalloc( jit_cnt*sizeof(*jit_entries) );
> -
> - output( "\t.data\n" );
> - output( "%s\n", asm_globl("__wine_jit_descs") );
> -
> - for (odp = spec->entry_points; odp < spec->entry_points + spec->nb_entry_points; odp++) {

Minor issue, but coding style in winebuild usually has "{" on the next line.
There are a couple more minor style issues in the next lines, like missing space inbetween of "(...)" and arguments.
> -
>    if (!(odp->flags & FLAG_JIT))
> -
>        continue;
> -
> -
>    for (i=0; i < emited_descs; i++) {
> -
>        if (!strcmp(odp->link_name, jit_entries[i]))
> -
>            break;
> -
>    }
> -
>    if (i < emited_descs)
> -
>        continue; /* already emited */
> -
>    jit_entries[emited_descs] = odp->link_name;
> -
> -
>    output( "\t%s %s\n", get_asm_ptr_keyword(), asm_name(odp->link_name) );
> -
>    output( "\t.long %u\n", get_args_size(odp) );
> -
>    emited_descs++;
> - }
> -
> - output( "\t%s 0\n", get_asm_ptr_keyword() );
> - output( "\t.long 0\n" );

You should use .previous here to restore the previous section.
> -
> - output("\t.text\n" );
> - output( "%s\n", asm_globl("__wine_jit_entries") );
> -
> - for (i=0; i < emited_descs; i++) {
> -
>    sym_name = strmake( "__jit_%s", jit_entries[i] );
> -
>    output( "%s\n", asm_globl(sym_name) );
> -
>    free( sym_name );
> -
> -
>    output( "\t.long 0\n\t.long 0\n\t.long 0\n\t.long 0\n" );

You might want to to emit .cfi_startproc and .cgi_endproc CFI directives here,
to ensure debuggers and unwinding code can split it into separate functions.
Also, you might want to use output_function_size().
> - }

Same as above, you should add a ".restore" here.
> -
> - free(jit_entries);
>
> +}
> -
>
> /*******************************************************************
> -
>     output_exports
> -
>
> @@ -332,6 +386,10 @@ void output_exports( DLLSPEC *spec )
>                  output( "\t%s %s_%s\n",
>                           get_asm_ptr_keyword(), asm_name("__wine_spec_ext_link"), odp->link_name );
>              }
> -
>        else if (target_cpu == CPU_x86 && (odp->flags & FLAG_JIT))
> -
>        {
> -
>            output( "\t%s %s_%s\n", get_asm_ptr_keyword(), asm_name("__jit"), odp->link_name );
> -
>        }
>        else
>        {
>            output( "\t%s %s\n", get_asm_ptr_keyword(), asm_name(odp->link_name) );
>
> @@ -622,6 +680,7 @@ void BuildSpec32File( DLLSPEC *spec )
>      output_stubs( spec );
>      output_exports( spec );
>      output_imports( spec );
> - output_jit_descs( spec );
> if (is_undefined( "__wine_call_from_regs" )) output_asm_relays();
> output_resources( spec );
> output_gnu_stack_note();
>
> diff --git a/tools/winebuild/winebuild.man.in b/tools/winebuild/winebuild.man.in
> index 2063738..2905bac 100644
> --- a/tools/winebuild/winebuild.man.in
> +++ b/tools/winebuild/winebuild.man.in
> @@ -299,6 +299,9 @@ accessed through GetProcAddress.
>  The entry point will be imported by ordinal instead of by name. The
>  name is still exported.
>  .TP
> +.B -jit
> +The function body will be generated in runtime. For internal Wine usage only.
> +.TP
>  .BI -arch= cpu\fR[\fB,\fIcpu\fR]
>  The entry point is only available on the specified CPU
>  architecture(s). The names \fBwin32\fR and \fBwin64\fR match all