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 <jacek(a)codeweavers.com>
> ---
>  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