Re: [Mingw-w64-public] [PATCH] ucrtbase: Provide most common *printf functions as non-inline functions

[Martin S. <ma...@ma...>]

On Thu, 16 Nov 2017, Martin Storsjö wrote:

> On Thu, 16 Nov 2017, Jacek Caban wrote:
>
>> On 16.11.2017 14:59, Martin Storsjö wrote:
>>       On Thu, 16 Nov 2017, Jacek Caban wrote:
>>
>>             Hi Martin,
>>
>>             The patch looks generally good to me. I'm fine with
>>             committing it.
>>
>>             As a side note, I expect this patch to also fix
>>             winpthread (built with
>>             toolchain defaulting default msvcrt.dll) linking
>>             problems that I saw when
>>             tried your patches.
>>
>
>
>>
>>             The first argument with compatibility flags seems
>>             inconsistent between
>>             implementations. Is there a reason for that? It's
>>             fine for the patch, you
>>             preserve headers behaviour, but it may be nice to
>>             revisit that at some
>>             point.
>> 
>>
>>       Yes, they're subtly different - almost (but not quite) what they
>>       were in their inline form in the headers.
>> 
>>
>>       In general we want these functions to behave according to
>>       standards, but e.g. for nonstandard functions like _snprintf, I
>>       think it's better to preserve the behaviour of how this function
>>       used to behave (i.e not writing any null termination) than to
>>       make it identical to normal snprintf.
>> 
>> 
>> That doesn't sound right. According to [1] those underscored functions
>> behave the same as not underscored on MSVC. They are also documented to be
>> preferred, according to Microsoft. That said, a code written against
>> ucrtbase according to documentation will use underscored versions and 
> expect
>> then to work the same as non-underscored. Such applications would consider
>> (correctly, I believe) mingw-w64 behaviour as broken.
>
> Oh, I didn't know that. I have a faint memory of testing snprintf vs 
> _snprintf on a modern MSVC, but I'll have to recheck.
>
>> In general, I think, it goes down to not introducing incompatibilities with
>> MSVC unless we have a reason to.
>> 
>> 
>>
>>       I did change _vsnprintf a little though, when making it
>>       non-inline. In the header form, I had _vsnprintf also behaving
>>       like _snprintf, in the legacy mode.
>>
>>       When we make (v)asprintf non-inline, they end up calling the
>>       implementation of (v)asprintf from libmingwex, so there's no
>>       point in trying to provide a version in libucrtbase.a.
>>
>>       The libmingwex implementation of (v)asprintf calls _vsnprintf.
>>       When testing things with wine (which I do a lot), this actually
>>       didn't work when I had the _vsnprintf implementation of use
>>       UCRTBASE_PRINTF_LEGACY_VSPRINTF_NULL_TERMINATION, I had to
>>       change it to UCRTBASE_PRINTF_STANDARD_SNPRINTF_BEHAVIOUR. With
>>       the real ucrtbase on real windows,
>>       UCRTBASE_PRINTF_LEGACY_VSPRINTF_NULL_TERMINATION works here, so
>>       it seems like it's a missed cornercase in the implementation of
>>       these flags in wine (where I only have myself to blame).
>>
>>       I guess I should change it to consistently use
>>       UCRTBASE_PRINTF_LEGACY_VSPRINTF_NULL_TERMINATION in _vsnprintf
>>       to match _snprintf and fix wine instead.
>> 
>> 
>> Oh, please, don't ever try to work around Wine bugs like that. Wine
>> ucrtbase.dll is not yet mature, but I expect things will get better over
>> time. Right now there are probably tons of subtle differences there.
>
> Yup - I was surprised to find this inconsistency in this case though, as I 
> implemented and wrote (IMO) pretty thorough tests for these flags to 
> __stdio_common_*printf, but apparently I missed some case.

Pushed, with _vsnprintf retaining the legacy form (which matches what MSVC 
does). And fwiw, I've identified the incorrect corner case in wine.

// Martin