From: Zebediah Figura Subject: Re: [PATCH] include: Use __cdecl calling convention for wine_dbg_log(). Message-Id: Date: Sat, 19 Sep 2020 10:39:39 -0500 In-Reply-To: <8832f565-e0ee-763d-f712-7dccd83f6c4b@codeweavers.com> References: <20200910193811.420427-1-mbruni@codeweavers.com> <0af3be11-847d-0548-02d3-bf4eaa363480@codeweavers.com> <8832f565-e0ee-763d-f712-7dccd83f6c4b@codeweavers.com> 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 >>> --- >>> 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. > Cheers, > -----BEGIN PGP SIGNATURE----- iQEzBAEBCAAdFiEENTo75Twe9rbPART3DZ01igeheEAFAl9mJjsACgkQDZ01igeh eEAGmQgAopYeBtlKc9dcoqHTk0viHJXUYqARQJ+Jc470Yx2WLCfUA17/MXpoBGrh MamBMXAhw/rpstj9p6mnCxA+PLxuUjo4n1nsd+g6YADzq1fQ2DfosPa8h6PaQGWv xYhYdBnhGSR85Yyzg+Kp8OTsdWf68Cn/oYGAE/ScnCgGORwoDumOsvy5zBIl0a2i i8Wwib94UV6llAjRgAc8diQ/cE3LrDHyuHlbcf3uwLfA1Rl1rG1+ydYHskti3AbC foKfiTVkE3UrLQ+XRBvX4ZTIHMSng15/yeD3GwSP7A/1NLguuhNKTbLsoBAE0Cj2 BAxs8fWa8MnN2TtciF148uBVxs+stA== =iE3a -----END PGP SIGNATURE-----