[libcxx] [Windows] Store the lconv struct returned from localeconv in locale_t

[mstorsjo]

Summary

This fixes using non-default locales, which currently can crash when e.g. formatting numbers.

Within the localeconv_l function, the per-thread locale is temporarily changed with __libcpp_locale_guard, then localeconv() is called, returning an lconv * struct pointer.

When localeconv_l returns, the __libcpp_locale_guard dtor restores the per-thread locale back to the original. This invalidates the contents of the earlier returned lconv struct, and all C strings that are pointed to within it are also invalidated.

Thus, to have an actually working localeconv_l function, the function needs to allocate some sort of storage for the returned contents, that stays valid for as long as the caller needs to use the returned struct.

Extend the libcxx/win32 specific locale_t class with storage for a deep copy of a lconv struct, and change localeconv_l to take a reference to the locale_t, to allow it to store the returned lconv struct there.

This works fine for libcxx itself, but wouldn't necessarily be right for a caller that uses libcxx's localeconv_l function.

[rnk]

Exciting. =/ So, successive calls to localeconv_l would invalidate the result from the last call. I could live with that if it was possible to make it private to libcxx.

[mstorsjo]

In D69505#1726163, @rnk wrote:

Exciting. =/ So, successive calls to localeconv_l would invalidate the result from the last call.

It's even worse than that. If you call localeconv_l with a locale other than the currently set thread specific locale, __libcpp_locale_guard will temporarily set the one we want to inspect, then receive the lconv * return pointer, and then reset the thread specific locale to what it was before the function was called. When the thread specific locale is reset on return, the lconv * pointer we're returning is invalidated.

So currently, localeconv_l never works, unless it's used on the currently set locale.

I could live with that if it was possible to make it private to libcxx.

Any suggestions on how to achieve that? Everything in include/locale, include/__locale and include/support/win32/locale_win32.h ends up included by callers. This current patch changes the ABI of the locale_t class on windows, but is that part of the API/ABI that users of libcxx are expected to use, or is it just a helper that accidentally ends up visible? (The locale_t class ends up defined without any enclosing namespace whatsoever, at the moment.)

One way could be to add some wrapper around the localeconv_l calls in src/locale.cpp. Ideally a wrapper that still returns lconv * for compatibility with everything else, but which can be made to allocate temp storage on the stack for windows. But I struggle to see a neat way of doing it.

One way that does come to mind is something preprocessor based, like this:

#ifdef _WIN32
#define GET_LCONV_PTR(var, loc) \
  lconv_storage lcs##var; \
  { \
    __libcpp_locale_guard __current(loc); \
    lcs.store(localeconv()); \
  } \
  lconv *var = lcs##var.getPtr()
#else
#define GET_LCONV_PTR(var, loc) \
  lconv *var = localeconv_l(loc)
#endif

[mstorsjo]

Ping @rnk

[rnk]

I don't have any great ideas, honestly. It seems like if we want to make non-default locales work well with libc++ on Windows, we're going to have to do more work to reimplement locales or find some new entry points in ucrtbase.dll that take the locale as a parameter instead of as a global.

In the meantime, this seems fine as stopgap fix.

[mstorsjo]

Thanks for looking into this again!

Do you have any input/opinion on tests for this (as @mclow.lists requested for D69554 as well)?

[mstorsjo]

FWIW, I looked into tests, and this seems to fix at least 11 failing tests in libc++ on windows.

@EricWF / @mclow.lists - what do you think of it?

[mstorsjo]

In D69505#1810841, @mstorsjo wrote:

FWIW, I looked into tests, and this seems to fix at least 11 failing tests in libc++ on windows.

@EricWF / @mclow.lists - what do you think of it?

Ping @EricWF

[ldionne]

Is it easy to add a test for this behavior? Otherwise, LGTM.

[mstorsjo]

In D69505#1843162, @ldionne wrote:

Is it easy to add a test for this behavior? Otherwise, LGTM.

This is already covered by the existing tests - this pach fixes around 10 of the currently failing tests.

@mclow.lists - did you want to dig further into this, or can I go ahead with it?

[mstorsjo]

@hans - It would be nice to have this i 10.x, after letting it sit in master for a few days.

[hans]

In D69505#1847889, @mstorsjo wrote:

@hans - It would be nice to have this i 10.x, after letting it sit in master for a few days.

Cherry-picked in ca6b341bd5d7159d9e398eef1a787b649c5bc888.

libcxx/include/support/win32/locale_win32.h

 #define LC_MESSAGES_MASK _M_MESSAGES
 #define LC_ALL_MASK (  LC_COLLATE_MASK \
                      | LC_CTYPE_MASK \
                      | LC_MESSAGES_MASK \
                      | LC_MONETARY_MASK \
                      | LC_NUMERIC_MASK \
                      | LC_TIME_MASK )
 
+class __lconv_storage {
+public:
+    __lconv_storage(const lconv *__lc_input) {
+        __lc = *__lc_input;
+
+        __decimal_point = __lc_input->decimal_point;
+        __thousands_sep = __lc_input->thousands_sep;
+        __grouping = __lc_input->grouping;
+        __int_curr_symbol = __lc_input->int_curr_symbol;
+        __currency_symbol = __lc_input->currency_symbol;
+        __mon_decimal_point = __lc_input->mon_decimal_point;
+        __mon_thousands_sep = __lc_input->mon_thousands_sep;
+        __mon_grouping = __lc_input->mon_grouping;
+        __positive_sign = __lc_input->positive_sign;
+        __negative_sign = __lc_input->negative_sign;
+
+        __lc.decimal_point = const_cast<char *>(__decimal_point.c_str());
+        __lc.thousands_sep = const_cast<char *>(__thousands_sep.c_str());
+        __lc.grouping = const_cast<char *>(__grouping.c_str());
+        __lc.int_curr_symbol = const_cast<char *>(__int_curr_symbol.c_str());
+        __lc.currency_symbol = const_cast<char *>(__currency_symbol.c_str());
+        __lc.mon_decimal_point = const_cast<char *>(__mon_decimal_point.c_str());
+        __lc.mon_thousands_sep = const_cast<char *>(__mon_thousands_sep.c_str());
+        __lc.mon_grouping = const_cast<char *>(__mon_grouping.c_str());
+        __lc.positive_sign = const_cast<char *>(__positive_sign.c_str());
+        __lc.negative_sign = const_cast<char *>(__negative_sign.c_str());
+    }
+
+    lconv *__get() {
+        return &__lc;
+    }
+private:
+    lconv __lc;
+    std::string __decimal_point;
+    std::string __thousands_sep;
+    std::string __grouping;
+    std::string __int_curr_symbol;
+    std::string __currency_symbol;
+    std::string __mon_decimal_point;
+    std::string __mon_thousands_sep;
+    std::string __mon_grouping;
+    std::string __positive_sign;
+    std::string __negative_sign;
+};
+
 class locale_t {
 public:
     locale_t()
-        : __locale(nullptr), __locale_str(nullptr) {}
+        : __locale(nullptr), __locale_str(nullptr), __lc(nullptr) {}
     locale_t(std::nullptr_t)
-        : __locale(nullptr), __locale_str(nullptr) {}
+        : __locale(nullptr), __locale_str(nullptr), __lc(nullptr) {}
     locale_t(_locale_t __xlocale, const char* __xlocale_str)
-        : __locale(__xlocale), __locale_str(__xlocale_str) {}
+        : __locale(__xlocale), __locale_str(__xlocale_str), __lc(nullptr) {}
+    locale_t(const locale_t &__l)
+        : __locale(__l.__locale), __locale_str(__l.__locale_str), __lc(nullptr) {}
+
+    ~locale_t() {
+        delete __lc;
+    }
+
+    locale_t &operator =(const locale_t &__l) {
+        __locale = __l.__locale;
+        __locale_str = __l.__locale_str;
+        // __lc not copied
+        return *this;
+    }
 
     friend bool operator==(const locale_t& __left, const locale_t& __right) {
         return __left.__locale == __right.__locale;
     }
 
     friend bool operator==(const locale_t& __left, int __right) {
         return __left.__locale == nullptr && __right == 0;
     }
operator bool() const {
         return __locale != nullptr;
     }
 
     const char* __get_locale() const { return __locale_str; }
 
     operator _locale_t() const {
         return __locale;
     }
+
+    lconv *__store_lconv(const lconv *__input_lc) {
+        delete __lc;
+        __lc = new __lconv_storage(__input_lc);
+        return __lc->__get();
+    }
 private:
     _locale_t __locale;
     const char* __locale_str;
+    __lconv_storage *__lc = nullptr;
 };
 
 // Locale management functions
 #define freelocale _free_locale
 // FIXME: base currently unused. Needs manual work to construct the new locale
 locale_t newlocale( int mask, const char * locale, locale_t base );
 // uselocale can't be implemented on Windows because Windows allows partial modification
 // of thread-local locale and so _get_current_locale() returns a copy while uselocale does
 // not create any copies.
 // We can still implement raii even without uselocale though.
 
 
-lconv *localeconv_l( locale_t loc );
+lconv *localeconv_l( locale_t &loc );
 size_t mbrlen_l( const char *__restrict s, size_t n,
                  mbstate_t *__restrict ps, locale_t loc);
 size_t mbsrtowcs_l( wchar_t *__restrict dst, const char **__restrict src,
                     size_t len, mbstate_t *__restrict ps, locale_t loc );
 size_t wcrtomb_l( char *__restrict s, wchar_t wc, mbstate_t *__restrict ps,
                   locale_t loc);
 size_t mbrtowc_l( wchar_t *__restrict pwc, const char *__restrict s,
                   size_t n, mbstate_t *__restrict ps, locale_t loc);

libcxx/src/support/win32/locale_win32.cpp

 #if defined(_LIBCPP_MSVCRT)
   return ___mb_cur_max_l_func(__l);
 #else
   __libcpp_locale_guard __current(__l);
   return MB_CUR_MAX;
 #endif
 }
 
-
-lconv *localeconv_l( locale_t loc )
+lconv *localeconv_l( locale_t &loc )
 {
     __libcpp_locale_guard __current(loc);
-    return localeconv();
+    lconv *lc = localeconv();
+    if (!lc)
+        return lc;
+    return loc.__store_lconv(lc);
 }
 size_t mbrlen_l( const char *__restrict s, size_t n,
                  mbstate_t *__restrict ps, locale_t loc )
 {
     __libcpp_locale_guard __current(loc);
     return mbrlen( s, n, ps );
 }
 size_t mbsrtowcs_l( wchar_t *__restrict dst, const char **__restrict src,