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

[Rémi Bernon]

On 2020-09-19 17:39, Zebediah Figura wrote:
> On 9/18/20 11:52 AM, Rémi Bernon wrote:
>> On 2020-09-10 22:30, Rémi Bernon wrote:
>>> 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.
>>
>>
>> Hi!
>>
>>
>> I actually pushed the idea a little bit further and implemented it, in
>> the attached patches, to illustrate a bit and because I think it could
>> possibly still be interesting.
>>
>>
>> The SSE register spilling savings are actually a little bit
>> underwhelming. As we have seen, any ms_abi function that calls a
>> sysv_abi function (or a non specified, in an ELF builtin) will spill the
>> XMM registers to the stack and restore them after the call.
>>
>> Marking the debug functions ms_abi mostly helps with the leaf functions
>> that do not call anything else, but for instance in wined3d, there's a
>> lot of unspecified helpers that will cause the same spilling to happen.
>> We will have to specify ms_abi on all of them to solve the issue.
>>
>> Then, making all functions ms_abi pushes the spilling down the call
>> chain, and if there's a system function to call in the end, it will have
>> to spill the registers anyway. If that function is called much more
>> often than the Wine entry point, then it may also not be ideal to force
>> the register spilling there, and having it on the entry point would be
>> preferable.
>>
>> Anyway, making debug functions ms_abi should not hurt in any case, they
>> are not supposed to be called often, they never go back from sysv_abi to
>> ms_abi, and keeping their own induced spilling internally is better
>> regardless of what the surrounding code does.
>>
>>
>> Now, what I find the most useful, is what this implementation allows us
>> to do with debug TRACE messages. As it makes every debug formatting go
>> through ntdll.so, I think it has at least two interesting properties:
>>
>> 1) We can control the formatting, and as I did in the patch, extend the
>> format specifiers as we like, and get rid of most of the static inline
>> debug helpers (I don't know if that's a good thing or not, but I
>> personally like to be able to do so).
>>
>> 2) The actual value formatting is done consistently by glibc. There's no
>> value formatting difference between PE code and ELF code TRACE messages.
>> And thanks to 1) we can still support MSVC-specific format specifiers,
>> we just need to convert them to the glibc equivalents.
>>
>>
> 
> This is something I considered a while back, when I was having problems
> with %l truncating 64-bit LONG_PTR values. The problem with it is that
> if we introduce custom changes to the format string, we lose the ability
> to use __attribute__((format(printf))), which I think is pretty valuable.
> 

I wasn't suggesting to change the basic types formatting, only support 
the Microsoft extensions, which I believe are checked for some (most) of 
them by the attribute.

Any customization we make should at least make sure it doesn't break the 
format checker. That's why I implemented the %p specializations with 
this suffixed syntax. We won't have validation other than "being a 
pointer", but that's maybe enough. For wine_dbgstr_longlong, it could be 
%p(ull), and take the ULONGLONG address instead.
-- 
Rémi Bernon <rbernon at codeweavers.com>