This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by thomasanderson on Mar 22 2019, 4:17 PM.

Details

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.

Diff Detail

Repository
rL LLVM

Event Timeline

thomasanderson created this revision.Mar 22 2019, 4:17 PM

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?

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 accepted this revision.Mar 25 2019, 4:54 PM

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

This revision is now accepted and ready to land.Mar 25 2019, 4:54 PM

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

Add FIXME and fix MINGW

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

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)"

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
92 ↗(On Diff #192321)

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 added inline comments.Mar 26 2019, 12:45 PM
src/support/win32/locale_win32.cpp
92 ↗(On Diff #192321)

Oh, right. Sorry for the confusion and noise!

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

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptMar 26 2019, 12:52 PM

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

@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 added a comment.EditedMar 27 2019, 11:07 AM

Will D59572 fix the __libcpp_locale_guard performance issues?

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.

What does __stdio_common_vsprintf give you that vsnprintf doesn't?

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()

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

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

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?

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.

I see. Thanks for explaining!