[PATCH] include: Use __cdecl calling convention for wine_dbg_log().

[Rémi Bernon]

On 2020-09-10 21:38, Matteo Bruni wrote:
> The practical consequence is that registers are saved / restored
> inside wine_dbg_log() instead of in the caller, which means not
> incurring in the overhead when debug channels are disabled.
> 
> Signed-off-by: Matteo Bruni <mbruni at codeweavers.com>
> ---
> This came up from hacking around yesterday with Paul and Rémi. I
> haven't tested the patch extensively but it seems to do the
> trick. wine_dbg_log() in practice is never inlined for me and that's
> the reason why this is enough. It's not possible to remove the inline
> keyword from the definition because that makes it trigger the "unused
> static function" warning all over Wine.
> 
> Worth mentioning that this patch makes sure that wine_dbg_log() has
> the same calling convention as __wine_dbg_header() and the other
> debug functions in ntdll.
> ---
>   include/wine/debug.h | 12 ++++++------
>   1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/include/wine/debug.h b/include/wine/debug.h
> index 912d61a90af0..462a43e22e20 100644
> --- a/include/wine/debug.h
> +++ b/include/wine/debug.h
> @@ -193,12 +193,12 @@ static inline int __wine_dbg_cdecl wine_dbg_printf( const char *format, ... )
>       return __wine_dbg_output( buffer );
>   }
>   
> -static int __wine_dbg_cdecl wine_dbg_log( enum __wine_debug_class cls,
> -                                          struct __wine_debug_channel *channel, const char *func,
> -                                          const char *format, ... ) __WINE_PRINTF_ATTR(4,5);
> -static inline int __wine_dbg_cdecl wine_dbg_log( enum __wine_debug_class cls,
> -                                                 struct __wine_debug_channel *channel,
> -                                                 const char *function, const char *format, ... )
> +static int __cdecl wine_dbg_log( enum __wine_debug_class cls,
> +                                 struct __wine_debug_channel *channel, const char *func,
> +                                 const char *format, ... ) __WINE_PRINTF_ATTR(4,5);
> +static inline int __cdecl wine_dbg_log( enum __wine_debug_class cls,
> +                                        struct __wine_debug_channel *channel,
> +                                        const char *function, const char *format, ... )
>   {
>       char buffer[1024];
>       __wine_dbg_va_list args;
> 

Well, if we intend to make these functions __cdecl, probably they all 
should be, in this header, including the inline helpers, as we've seen 
that any missing attribute can make the spilling leak to its callers.

Then, I didn't look very precisely either, but I believe there's 
actually a big issue that makes it not so simple after all (correct me 
if I'm wrong though, maybe it's all good):

If we set "ms_abi" attribute to these vararg functions, then their 
calling convention will be as such. Thus, we cannot use anymore the sysv 
va_list in there, and we have instead to use the __ms_va_list (defined 
to __builtin_ms_va_list in this case).

Now, doing so will break the vsnprintf call, when it goes to libc, as it 
expects a sysv va_list argument instead.

One possible way that I see would be instead to have a 
__wine_dbg_vsnprintf export in ntdll, that either forward the call to 
msvcrt vsnprintf (which already expects a __ms_va_list argument), or to 
an ntdll.so implementation.

The ntdll.so implementation will have to be here anyway, to support its 
own traces, as it cannot go back to msvcrt for that. It will have to 
handle the __ms_va_arg somehow, possibly translating it for libc (which 
actually may not be so hard, with snprintf and one argument at a time), 
or handle the formatting on its own.

The latter could actually make the whole thing worth the effort, if we 
consider that TRACE messages could benefit from a specific printf 
implementation with some extensions. For instances, it could print 
strings and wstrings at the same time, in a safe way, without needing 
the debugstr_a/w anymore. Same for guids for instance.
-- 
Rémi Bernon <rbernon at codeweavers.com>