[libc++] Fix return value of snprintf_l() on Windows when buffer is too small

[thomasanderson]

Summary

When the output buffer is too small to contain the output, vsnprintf()
fills the buffer and returns the number of characters that would have
been written if the buffer was sufficiently large.

_vnsprintf_s() on the other hand fills the buffer and returns -1 when this
happens. We want the former behavior, but we also want to be able to
pass in a locale to prevent having to call setlocale().

__stdio_common_vsprintf() is the only function general enough to get
the behavior we want.

[EricWF]

I'm never comfortable using other STL's implementation details. When a user does that to libc++, we slap their hand.
And MSVC docs explicitly ask us not to use that here:
https://docs.microsoft.com/en-us/cpp/c-runtime-library/internal-crt-globals-and-functions?view=vs-2017

Is there any other paths forward?

[thomasanderson]

In D59727#1440354, @EricWF wrote:

I'm never comfortable using other STL's implementation details. When a user does that to libc++, we slap their hand.
And MSVC docs explicitly ask us not to use that here:
https://docs.microsoft.com/en-us/cpp/c-runtime-library/internal-crt-globals-and-functions?view=vs-2017

Is there any other paths forward?

There's a messy solution that doesn't rely on internals. We could use _vsnprintf_s_l() and if the return value is -1 and errno is ERANGE, then we know the buffer is too small. Then we would have to fall back on the old slow codepath to get the buffer size.

[EricWF]

OK, but can you open a bug that we need to find a better solution in the long run?

[mstorsjo]

FWIW, this will break mingw builds that use the old msvcrt.dll. Since a couple years, mingw-w64 also supports the UCRT, but this is not the default configuration upstream yet (while it is in my own builds).

[thomasanderson]

Add FIXME and fix MINGW

[thomasanderson]

In D59727#1442500, @EricWF wrote:

OK, but can you open a bug that we need to find a better solution in the long run?

Done: https://bugs.llvm.org/show_bug.cgi?id=41244
Also added a FIXME in code

In D59727#1442687, @mstorsjo wrote:

FWIW, this will break mingw builds that use the old msvcrt.dll. Since a couple years, mingw-w64 also supports the UCRT, but this is not the default configuration upstream yet (while it is in my own builds).

Latest patch adds "!defined(MINGW32)"

[smeenai]

In D59727#1442687, @mstorsjo wrote:

FWIW, this will break mingw builds that use the old msvcrt.dll. Since a couple years, mingw-w64 also supports the UCRT, but this is not the default configuration upstream yet (while it is in my own builds).

This is guarded by _LIBCPP_MSVCRT, which shouldn't be defined for MinGW.

src/support/win32/locale_win32.cpp

96–98

From __config:

#  if defined(_MSC_VER) && !defined(__MINGW32__)
#    define _LIBCPP_MSVCRT // Using Microsoft's C Runtime library
#  endif

So checking for !defined(__MINGW32__) here is redundant.

[mstorsjo]

src/support/win32/locale_win32.cpp

96–98

Oh, right. Sorry for the confusion and noise!

[thomasanderson]

Removed !defined(__MINGW32__) in the latest patch. Thanks @smeenai and @mstorsjo ! Going to land with that latest change.

[zoecarver]

@thomasanderson vsnprintf uses __stdio_common_vsprintf on MSVC. Is there a performance hit when only vsnprintf is used?

[thomasanderson]

In D59727#1444732, @zoecarver wrote:

@thomasanderson vsnprintf uses __stdio_common_vsprintf on MSVC. Is there a performance hit when only vsnprintf is used?

vsnprintf doesn't have a performance hit, but it doesn't allow setting the locale. The __libcpp_locale_guard is the thing that's slow that we're trying to avoid.

[zoecarver]

Will D59572 fix the __libcpp_locale_guard performance issues?

[thomasanderson]

In D59727#1444886, @zoecarver wrote:

Will D59572 fix the __libcpp_locale_guard performance issues?

It improves the issue, but does not fix it. The slowness is caused by setlocale(), and that change reduces the number of calls to setlocale() from 10 to 2.

[zoecarver]

What does __stdio_common_vsprintf give you that vsnprintf doesn't?

[thomasanderson]

In D59727#1444917, @zoecarver wrote:

What does __stdio_common_vsprintf give you that vsnprintf doesn't?

It also takes a locale as an argument so we can avoid calling setlocale()

[zoecarver]

Thanks for explaining. Would it work to call setlocale() then add just use vsnprintf?

[thomasanderson]

In D59727#1444922, @zoecarver wrote:

Thanks for explaining. Would it work to call setlocale() then add just use vsnprintf?

Yes, in fact that's what the code path in the #else block does

[zoecarver]

I mean would it be possible to call setlocale directly so it wouldn't have to deal with __libcpp_locale_guard. Also, do you know if that is equally as fast as calling __stdio_common_vsprintf?

[thomasanderson]

In D59727#1444955, @zoecarver wrote:

I mean would it be possible to call setlocale directly so it wouldn't have to deal with __libcpp_locale_guard. Also, do you know if that is equally as fast as calling __stdio_common_vsprintf?

Have a look at the constructor/destructor for __libcpp_locale_guard. It's entire purpose in life is to call setlocale(), which is the thing that's slow. Inlining the setlocale() here wouldn't improve speed.

[zoecarver]

I see. Thanks for explaining!

src/support/win32/locale_win32.cpp

 int snprintf_l(char *ret, size_t n, locale_t loc, const char *format, ...)
 {
 #if !defined(_LIBCPP_MSVCRT)
     __libcpp_locale_guard __current(loc);
 #endif
     va_list ap;
     va_start( ap, format );
 #if defined(_LIBCPP_MSVCRT)
-    int result = _vsnprintf_l( ret, n, format, loc, ap );
+    int result = __stdio_common_vsprintf(
+        _CRT_INTERNAL_LOCAL_PRINTF_OPTIONS | _CRT_INTERNAL_PRINTF_STANDARD_SNPRINTF_BEHAVIOR,
+        ret, n, format, loc, ap);
 #else
     int result = vsnprintf( ret, n, format, ap );
 #endif
     va_end(ap);
     return result;
 }
 
 int asprintf_l( char **ret, locale_t loc, const char *format, ... )