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@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, ... )
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.