[libc++] Use runtime rather then compile-time glibc version check

[phosek]

Summary

glibc supports versioning, so it's possible to build against older
version and run against newer version. This is sometimes relied on
in practice, e.g. in Fuchsia build we build against older sysroot
(equivalent to Ubuntu Trusty) to cover the broadest possible range
of host systems, but that doesn't necessarily match the system that
binary is going to run on which may have newer version, in which case
the compile test used in curr_symbol is going to fail. Using runtime
check is more reliable. This is a follow up to D56702 which addressed
one instance, this patch addresses all of the remaining ones.

[ldionne]

I think the correct way of handling this would be to define a Lit feature that expresses the fact that we're running on a newer system but we compiled on an older system. Like the reverse of what we do for back-deployment on macOS. I know this isn't what we did in D56702, but that strikes me as the wrong approach. It seems to me like this is something we want to tackle at the test driver level, not inside the test per-se. Am I missing something?

[phosek]

In D88188#2291289, @ldionne wrote:

I think the correct way of handling this would be to define a Lit feature that expresses the fact that we're running on a newer system but we compiled on an older system. Like the reverse of what we do for back-deployment on macOS. I know this isn't what we did in D56702, but that strikes me as the wrong approach. It seems to me like this is something we want to tackle at the test driver level, not inside the test per-se. Am I missing something?

We could do it in lit but I'm not sure if it's going to be simpler or cleaner. We would need to find out the location of glibc which can vary across distributions and then parse the output of libc.so.6 --version which is more work than what we're doing in glibc_version_less_than. If the test is being executed on a remote system, this needs to be done over remote protocol which may complicate things even more. Then we need to pass that version to each test where the logic is likely going to be very similar what I've done in this patch.

[phosek]

In D88188#2291752, @phosek wrote:

In D88188#2291289, @ldionne wrote:

I think the correct way of handling this would be to define a Lit feature that expresses the fact that we're running on a newer system but we compiled on an older system. Like the reverse of what we do for back-deployment on macOS. I know this isn't what we did in D56702, but that strikes me as the wrong approach. It seems to me like this is something we want to tackle at the test driver level, not inside the test per-se. Am I missing something?

We could do it in lit but I'm not sure if it's going to be simpler or cleaner. We would need to find out the location of glibc which can vary across distributions and then parse the output of libc.so.6 --version which is more work than what we're doing in glibc_version_less_than. If the test is being executed on a remote system, this needs to be done over remote protocol which may complicate things even more. Then we need to pass that version to each test where the logic is likely going to be very similar what I've done in this patch.

I checked with @mcgrathr and pointed out that we could use getconf GNU_LIBC_VERSION to get glibc version which is a command-line equivalent of confstr invocation used by glibc_version_less_than. I still think that the current implementation is simpler, but I can try to rewrite it in lit if you prefer.

[phosek]

In D88188#2293911, @phosek wrote:

In D88188#2291752, @phosek wrote:

In D88188#2291289, @ldionne wrote:

I think the correct way of handling this would be to define a Lit feature that expresses the fact that we're running on a newer system but we compiled on an older system. Like the reverse of what we do for back-deployment on macOS. I know this isn't what we did in D56702, but that strikes me as the wrong approach. It seems to me like this is something we want to tackle at the test driver level, not inside the test per-se. Am I missing something?

We could do it in lit but I'm not sure if it's going to be simpler or cleaner. We would need to find out the location of glibc which can vary across distributions and then parse the output of libc.so.6 --version which is more work than what we're doing in glibc_version_less_than. If the test is being executed on a remote system, this needs to be done over remote protocol which may complicate things even more. Then we need to pass that version to each test where the logic is likely going to be very similar what I've done in this patch.

I checked with @mcgrathr and pointed out that we could use getconf GNU_LIBC_VERSION to get glibc version which is a command-line equivalent of confstr invocation used by glibc_version_less_than. I still think that the current implementation is simpler, but I can try to rewrite it in lit if you prefer.

@ldionne do you have any preference?

[ldionne]

I guess I don't really like this sort of mangling of the test suite, however looking at other locale tests, it seems like we already do a lot of it. Furthermore, my lit suggestion would basically allow XFAILing the tests, which isn't what you're after. So I guess this is okay.

How far back do you support glibc? It would be nice to have a clear bound on this.

[phosek]

In D88188#2301902, @ldionne wrote:

I guess I don't really like this sort of mangling of the test suite, however looking at other locale tests, it seems like we already do a lot of it. Furthermore, my lit suggestion would basically allow XFAILing the tests, which isn't what you're after. So I guess this is okay.

Thanks!

How far back do you support glibc? It would be nice to have a clear bound on this.

2.17 which was shipped with Ubuntu 14.04, this patch is sufficient so I don't think we should need further changes.

[ldionne]

This broke the tests on MacOS: https://buildkite.com/llvm-project/libcxx-ci/builds/74#5d6d5ad0-27cc-4de5-b6d7-1b7b04d49464

libcxx/test/std/localization/locale.categories/category.monetary/locale.moneypunct.byname/thousands_sep.pass.cpp

129

On non-glibc, this changed the wsep from L' ' to L'\u00A0' if I read this correctly.

[phosek]

libcxx/test/std/localization/locale.categories/category.monetary/locale.moneypunct.byname/thousands_sep.pass.cpp

129

Thanks for catching this, addressed in rG4424d2428aebd3537d3f5a3625acb65edfb5d11f.

libcxx/test/std/localization/locale.categories/category.monetary/locale.money.get/locale.money.get.members/get_long_double_fr_FR.pass.cpp

explicit my_facetw(std::size_t refs = 0)
         : Fw(refs) {}
 };
 
 
 // GLIBC 2.27 and newer use U2027 (narrow non-breaking space) as a thousands sep.
 // this function converts the spaces in string inputs to that character if need
 // be.
 static std::wstring convert_thousands_sep(std::wstring const& in) {
-#ifndef TEST_GLIBC_PREREQ
-#define TEST_GLIBC_PREREQ(x, y) 0
-#endif
-#if TEST_GLIBC_PREREQ(2,27)
+#if defined(_CS_GNU_LIBC_VERSION)
+  if (glibc_version_less_than("2.27"))
+    return in;
   std::wstring out;
   unsigned I = 0;
   bool seen_decimal = false;
   for (; I < in.size(); ++I) {
     if (seen_decimal || in[I] != L' ') {
       seen_decimal |= in[I] == L',';
       out.push_back(in[I]);
       continue;

libcxx/test/std/localization/locale.categories/category.monetary/locale.money.put/locale.money.put.members/put_long_double_fr_FR.pass.cpp

explicit my_facetw(std::size_t refs = 0)
         : Fw(refs) {}
 };
 
 
 // GLIBC 2.27 and newer use U2027 (narrow non-breaking space) as a thousands sep.
 // this function converts the spaces in string inputs to that character if need
 // be.
 static std::wstring convert_thousands_sep(std::wstring const& in) {
-#ifndef TEST_GLIBC_PREREQ
-#define TEST_GLIBC_PREREQ(x, y) 0
-#endif
-#if TEST_GLIBC_PREREQ(2,27)
+#if defined(_CS_GNU_LIBC_VERSION)
+  if (glibc_version_less_than("2.27"))
+    return in;
   std::wstring out;
   unsigned I = 0;
   bool seen_num_start = false;
   bool seen_decimal = false;
   for (; I < in.size(); ++I) {
     seen_decimal |= in[I] == L',';
     seen_num_start |= in[I] == '-' || std::iswdigit(in[I]);
     if (seen_decimal || !seen_num_start || in[I] != L' ') {

libcxx/test/std/localization/locale.categories/category.monetary/locale.moneypunct.byname/curr_symbol.pass.cpp

 class Fwt
     : public std::moneypunct_byname<wchar_t, true>
 {
 public:
     explicit Fwt(const std::string& nm, std::size_t refs = 0)
         : std::moneypunct_byname<wchar_t, true>(nm, refs) {}
 };
 
-#if defined(_CS_GNU_LIBC_VERSION)
-static bool glibc_version_less_than(char const* version) {
-    std::string test_version = std::string("glibc ") + version;
-
-    size_t n = confstr(_CS_GNU_LIBC_VERSION, nullptr, (size_t)0);
-    char *current_version = new char[n];
-    confstr(_CS_GNU_LIBC_VERSION, current_version, n);
-
-    bool result = strverscmp(current_version, test_version.c_str()) < 0;
-    delete[] current_version;
-    return result;
-}
-#endif
-
 int main(int, char**)
 {
     {
         Fnf f("C", 1);
         assert(f.curr_symbol() == std::string());
     }
     {
         Fnt f("C", 1);

libcxx/test/std/localization/locale.categories/category.monetary/locale.moneypunct.byname/decimal_point.pass.cpp

int main(int, char**)
         assert(f.decimal_point() == L',');
     }
     {
         Fwt f(LOCALE_fr_FR_UTF_8, 1);
         assert(f.decimal_point() == L',');
     }
 // GLIBC 2.23 uses '.' as the decimal point while other C libraries use ','
 // GLIBC 2.27 corrects this
-#ifndef TEST_GLIBC_PREREQ
-#define TEST_GLIBC_PREREQ(x, y) 0
-#endif
-#if !defined(TEST_HAS_GLIBC) || TEST_GLIBC_PREREQ(2, 27)
+#if defined(_CS_GNU_LIBC_VERSION)
+    const char sep = glibc_version_less_than("2.27") ? '.' : ',';
+    const wchar_t wsep = glibc_version_less_than("2.27") ? L'.' : L',';
+#else
     const char sep = ',';
     const wchar_t wsep = L',';
-#else
-    const char sep = '.';
-    const wchar_t wsep = L'.';
 #endif
     {
         Fnf f(LOCALE_ru_RU_UTF_8, 1);
         assert(f.decimal_point() == sep);
     }
     {
         Fnt f(LOCALE_ru_RU_UTF_8, 1);
         assert(f.decimal_point() == sep);

libcxx/test/std/localization/locale.categories/category.monetary/locale.moneypunct.byname/thousands_sep.pass.cpp

int main(int, char**)
         Fnf f(LOCALE_fr_FR_UTF_8, 1);
         assert(f.thousands_sep() == ' ');
     }
     {
         Fnt f(LOCALE_fr_FR_UTF_8, 1);
         assert(f.thousands_sep() == ' ');
     }
 // The below tests work around GLIBC's use of U202F as mon_thousands_sep.
-#ifndef TEST_GLIBC_PREREQ
-#define TEST_GLIBC_PREREQ(x, y) 0
-#endif
-#if defined(TEST_HAS_GLIBC) && TEST_GLIBC_PREREQ(2, 27)
-    const wchar_t fr_sep = L'\u202F';
+#if defined(_CS_GNU_LIBC_VERSION)
+    const wchar_t fr_sep = glibc_version_less_than("2.27") ? L' ' : L'\u202F';
 #else
     const wchar_t fr_sep = L' ';
 #endif
     {
         Fwf f(LOCALE_fr_FR_UTF_8, 1);
         assert(f.thousands_sep() == fr_sep);
     }
     {
         Fwt f(LOCALE_fr_FR_UTF_8, 1);
         assert(f.thousands_sep() == fr_sep);
     }
 // The below tests work around GLIBC's use of U00A0 as mon_thousands_sep
 // and U002E as mon_decimal_point.
 // TODO: Fix thousands_sep for 'char'.
 // related to https://gcc.gnu.org/bugzilla/show_bug.cgi?id=16006
-#ifndef TEST_HAS_GLIBC
+#if defined(_CS_GNU_LIBC_VERSION)
     const char sep = ' ';
-    const wchar_t wsep = L' ';
-#elif TEST_GLIBC_PREREQ(2, 27)
     // FIXME libc++ specifically works around \u00A0 by translating it into
     // a regular space.
-    const char sep = ' ';
-    const wchar_t wsep = L'\u202F';
+    const wchar_t wsep = glibc_version_less_than("2.27") ? L'\u00A0' : L'\u202F';
 #else
+    const char sep = ' ';
     // FIXME libc++ specifically works around \u00A0 by translating it into
     // a regular space.
-    const char sep = ' ';
     const wchar_t wsep = L'\u00A0';
 #endif
     {
         Fnf f(LOCALE_ru_RU_UTF_8, 1);
         assert(f.thousands_sep() == sep);
     }
     {
         Fnt f(LOCALE_ru_RU_UTF_8, 1);

libcxx/test/std/localization/locale.categories/facet.numpunct/locale.numpunct.byname/thousands_sep.pass.cpp

int main(int, char**)
         {
             typedef wchar_t C;
             const std::numpunct<C>& np = std::use_facet<std::numpunct<C> >(l);
             assert(np.thousands_sep() == L',');
         }
     }
     {
         std::locale l(LOCALE_fr_FR_UTF_8);
-#if defined(TEST_HAS_GLIBC)
-        const char sep = ' ';
 // The below tests work around GLIBC's use of U202F as LC_NUMERIC thousands_sep.
-# if TEST_GLIBC_PREREQ(2, 27)
-        const wchar_t wsep = L'\u202f';
-# else
-        const wchar_t wsep = L' ';
-# endif
+#if defined(_CS_GNU_LIBC_VERSION)
+        const char sep = ' ';
+        const wchar_t wsep = glibc_version_less_than("2.27") ? L' ' : L'\u202f';
 #else
         const char sep = ',';
         const wchar_t wsep = L',';
 #endif
         {
             typedef char C;
             const std::numpunct<C>& np = std::use_facet<std::numpunct<C> >(l);
             if (np.thousands_sep() != sep) {

libcxx/test/support/platform_support.h

 {
     return std::wstring_convert<std::codecvt_utf8_utf16<wchar_t> >().from_bytes(
         get_temp_file_name());
 }
 #endif // _LIBCPP_HAS_OPEN_WITH_WCHAR
 
 #endif // __CloudABI__
 
+#if defined(_CS_GNU_LIBC_VERSION)
+inline bool glibc_version_less_than(char const* version) {
+  std::string test_version = std::string("glibc ") + version;
+
+  size_t n = confstr(_CS_GNU_LIBC_VERSION, nullptr, (size_t)0);
+  char *current_version = new char[n];
+  confstr(_CS_GNU_LIBC_VERSION, current_version, n);
+
+  bool result = strverscmp(current_version, test_version.c_str()) < 0;
+  delete[] current_version;
+  return result;
+}
+#endif // _CS_GNU_LIBC_VERSION
+
 #endif // PLATFORM_SUPPORT_H