Page MenuHomePhabricator

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

Authored by phosek on Jan 14 2019, 8:35 PM.

Details

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.

Diff Detail

Repository
rCXX libc++

Event Timeline

phosek created this revision.Jan 14 2019, 8:35 PM
phosek updated this revision to Diff 182205.Jan 16 2019, 7:01 PM
ldionne added inline comments.Jan 22 2019, 9:08 AM
libcxx/test/std/localization/locale.categories/category.monetary/locale.moneypunct.byname/curr_symbol.pass.cpp
119–123 ↗(On Diff #182205)

This logic could be defined in a function like:

#if defined(_CS_GNU_LIBC_VERSION)
bool glibc_version_less_than(char const* version) {
    std::string test_version = "glibc "; test_version += 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

Then, we could use it as:

#if defined(_CS_GNU_LIBC_VERSION)
    // GLIBC <= 2.23 uses currency_symbol="<U0440><U0443><U0431>"
    // GLIBC >= 2.24 uses currency_symbol="<U20BD>"
    // See also: http://www.fileformat.info/info/unicode/char/20bd/index.htm
    if (!glibc_version_less_than("2.24")) {
        assert(f.curr_symbol() == " \u20BD");
    } else {
        assert(f.curr_symbol() == " \xD1\x80\xD1\x83\xD0\xB1");
    }
#else
    assert(f.curr_symbol() == " \xD1\x80\xD1\x83\xD0\xB1");
#endif

We may or may not want to lift this function to a header common to the tests, but not defining the logic inline seems like a gain in readability. WDYT?

phosek updated this revision to Diff 183163.Jan 23 2019, 1:10 PM
phosek marked an inline comment as done.
phosek updated this revision to Diff 183164.Jan 23 2019, 1:14 PM
ldionne accepted this revision.Jan 28 2019, 10:23 AM
This revision is now accepted and ready to land.Jan 28 2019, 10:23 AM
This revision was automatically updated to reflect the committed changes.