* [PATCH 1/2] libstdc++: Robustify long double std::to_chars testcase [PR98384] @ 2021-02-22 21:55 Patrick Palka 2021-02-22 21:55 ` [PATCH 2/2] libstdc++: Fix endianness issue with IBM long double [PR98384] Patrick Palka 2021-02-23 16:30 ` [PATCH 1/2] libstdc++: Robustify long double std::to_chars testcase [PR98384] Patrick Palka 0 siblings, 2 replies; 7+ messages in thread From: Patrick Palka @ 2021-02-22 21:55 UTC (permalink / raw) To: gcc-patches; +Cc: libstdc++, Patrick Palka This makes the hexadecimal section of the long double std::to_chars testcase more robust by avoiding false-negative FAILs due to printf using a different leading hex digit than us, and by additionally verifying the correctness of the hexadecimal form via round-tripping through std::from_chars. Tested on x86, x86_64, powerpc64be, powerpc64le and aarch64. Does this look OK for trunk? libstdc++-v3/ChangeLog: PR libstdc++/98384 * testsuite/20_util/to_chars/long_double.cc: Include <optional>. (test01): Simplify verifying the nearby values by using a 2-iteration loop and a dedicated output buffer to check that the nearby values are different. Factor out the printf-based verification into a local function, and check that the leading hex digits agree before comparing with the output of printf. Also verify the output by round-tripping it through from_chars. --- .../testsuite/20_util/to_chars/long_double.cc | 73 ++++++++++++------- 1 file changed, 47 insertions(+), 26 deletions(-) diff --git a/libstdc++-v3/testsuite/20_util/to_chars/long_double.cc b/libstdc++-v3/testsuite/20_util/to_chars/long_double.cc index 4f72cb65400..34cc03034cc 100644 --- a/libstdc++-v3/testsuite/20_util/to_chars/long_double.cc +++ b/libstdc++-v3/testsuite/20_util/to_chars/long_double.cc @@ -26,6 +26,7 @@ #include <cmath> #include <cstring> #include <iterator> +#include <optional> #include <limits> #include <testsuite_hooks.h> @@ -50,6 +51,38 @@ namespace detail void test01() { + // Verifies correctness of the hexadecimal form [BEGIN,END) for VALUE by + // round-tripping it through from_chars (if available). + auto verify_via_from_chars = [] (char *begin, char *end, long double value) { +#if __cpp_lib_to_chars >= 201611L || _GLIBCXX_HAVE_USELOCALE + long double roundtrip; + auto result = from_chars(begin, end, roundtrip, chars_format::hex); + VERIFY( result.ec == errc{} ); + VERIFY( result.ptr == end ); + VERIFY( roundtrip == value ); +#endif + }; + + // Verifies correctness of the null-terminated hexadecimal form at BEGIN + // for VALUE and PRECISION by comparing it with the output of printf's %La + // conversion specifier. + auto verify_via_printf = [] (char *begin, long double value, + optional<int> precision = nullopt) { + char printf_buffer[1024] = {}; + if (precision.has_value()) + sprintf(printf_buffer, "%.*La", precision.value(), value); + else + sprintf(printf_buffer, "%La", value); + + // Only compare with the output of printf if the leading hex digits agree. + // If the leading hex digit of our form doesn't agree with that of printf, + // then the two forms may still be equivalent (e.g. 1.1p+0 vs 8.8p-3), so we + // don't want a FAIL in this case. But if the leading hex digits do agree, + // then we do expect the two forms to be the same. + if (printf_buffer[strlen("0x")] == begin[0]) + VERIFY( !strcmp(begin, printf_buffer+strlen("0x")) ); + }; + const long double hex_testcases[] = { detail::nextdownl(numeric_limits<long double>::max()), detail::nextupl(numeric_limits<long double>::min()), @@ -92,38 +125,27 @@ test01() if (testcase == 0.0L || isinf(testcase)) continue; - char to_chars_buffer[1024], printf_buffer[1024]; - memset(to_chars_buffer, '\0', sizeof(to_chars_buffer)); - memset(printf_buffer, '\0', sizeof(printf_buffer)); - + char to_chars_buffer[1024] = {}; auto result = to_chars(begin(to_chars_buffer), end(to_chars_buffer), testcase, chars_format::hex); VERIFY( result.ec == errc{} ); *result.ptr = '\0'; - sprintf(printf_buffer, "%La", testcase); - VERIFY( !strcmp(to_chars_buffer, printf_buffer+strlen("0x")) ); + verify_via_from_chars(begin(to_chars_buffer), result.ptr, testcase); + verify_via_printf(to_chars_buffer, testcase); + // Verify the nearby values, and also check they have a different + // shortest form. + for (long double nearby + : { detail::nextdownl(testcase), detail::nextupl(testcase) }) { - // Verify that the nearby values have a different shortest form. - testcase = detail::nextdownl(testcase); - result = to_chars(begin(to_chars_buffer), end(to_chars_buffer), - testcase, chars_format::hex); - VERIFY( result.ec == errc{} ); - *result.ptr = '\0'; - VERIFY( strcmp(to_chars_buffer, printf_buffer+strlen("0x")) != 0); - sprintf(printf_buffer, "%La", testcase); - VERIFY( !strcmp(to_chars_buffer, printf_buffer+strlen("0x")) ); - - testcase = detail::nextupl(detail::nextupl(testcase)); - result = to_chars(begin(to_chars_buffer), end(to_chars_buffer), - testcase, chars_format::hex); + char nearby_buffer[1024] = {}; + result = to_chars(begin(nearby_buffer), end(nearby_buffer), + nearby, chars_format::hex); VERIFY( result.ec == errc{} ); *result.ptr = '\0'; - VERIFY( strcmp(to_chars_buffer, printf_buffer+strlen("0x")) != 0); - sprintf(printf_buffer, "%La", testcase); - VERIFY( !strcmp(to_chars_buffer, printf_buffer+strlen("0x")) ); - - testcase = detail::nextdownl(testcase); + VERIFY( strcmp(nearby_buffer, to_chars_buffer) != 0); + verify_via_from_chars(begin(nearby_buffer), result.ptr, nearby); + verify_via_printf(nearby_buffer, nearby); } for (int precision = -1; precision < 50; precision++) @@ -132,8 +154,7 @@ test01() testcase, chars_format::hex, precision); VERIFY( result.ec == errc{} ); *result.ptr = '\0'; - sprintf(printf_buffer, "%.*La", precision, testcase); - VERIFY( !strcmp(to_chars_buffer, printf_buffer+strlen("0x")) ); + verify_via_printf(to_chars_buffer, testcase, precision); } } } -- 2.30.1.559.g2283e0e9af ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH 2/2] libstdc++: Fix endianness issue with IBM long double [PR98384] 2021-02-22 21:55 [PATCH 1/2] libstdc++: Robustify long double std::to_chars testcase [PR98384] Patrick Palka @ 2021-02-22 21:55 ` Patrick Palka 2021-02-22 22:22 ` Jonathan Wakely 2021-02-23 16:30 ` [PATCH 1/2] libstdc++: Robustify long double std::to_chars testcase [PR98384] Patrick Palka 1 sibling, 1 reply; 7+ messages in thread From: Patrick Palka @ 2021-02-22 21:55 UTC (permalink / raw) To: gcc-patches; +Cc: libstdc++, Patrick Palka The logic in std::to_chars for extracting the high- and low-order parts of a IBM long double value does the right thing on powerpc64le, but not on powerpc64be. This patch makes the extraction endian-agnostic, which fixes the execution FAIL of to_chars/long_double.cc on powerpc64be. Tested on powerpc64le and powerpc64be. Does this look OK for trunk? libstdc++-v3/ChangeLog: PR libstdc++/98384 * src/c++17/floating_to_chars.cc (get_ieee_repr): Extract the high- and low-order parts from an IBM long double value in an endian-agnostic way. --- libstdc++-v3/src/c++17/floating_to_chars.cc | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/libstdc++-v3/src/c++17/floating_to_chars.cc b/libstdc++-v3/src/c++17/floating_to_chars.cc index a50548acae7..4b2f85c1c1a 100644 --- a/libstdc++-v3/src/c++17/floating_to_chars.cc +++ b/libstdc++-v3/src/c++17/floating_to_chars.cc @@ -395,11 +395,11 @@ namespace // of the high part, and we merge the mantissa of the high part with the // mantissa (and the implicit leading bit) of the low part. using uint_t = unsigned __int128; - uint_t value_bits = 0; - memcpy(&value_bits, &value, sizeof(value_bits)); + uint64_t value_bits[2] = {}; + memcpy(value_bits, &value, sizeof(value_bits)); - const uint64_t value_hi = value_bits; - const uint64_t value_lo = value_bits >> 64; + const uint64_t value_hi = value_bits[0]; + const uint64_t value_lo = value_bits[1]; uint64_t mantissa_hi = value_hi & ((1ull << 52) - 1); unsigned exponent_hi = (value_hi >> 52) & ((1ull << 11) - 1); -- 2.30.1.559.g2283e0e9af ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 2/2] libstdc++: Fix endianness issue with IBM long double [PR98384] 2021-02-22 21:55 ` [PATCH 2/2] libstdc++: Fix endianness issue with IBM long double [PR98384] Patrick Palka @ 2021-02-22 22:22 ` Jonathan Wakely 0 siblings, 0 replies; 7+ messages in thread From: Jonathan Wakely @ 2021-02-22 22:22 UTC (permalink / raw) To: Patrick Palka; +Cc: gcc-patches, libstdc++ On Mon, 22 Feb 2021, 22:13 Patrick Palka via Libstdc++, < libstdc++@gcc.gnu.org> wrote: > The logic in std::to_chars for extracting the high- and low-order parts > of a IBM long double value does the right thing on powerpc64le, but not > on powerpc64be. This patch makes the extraction endian-agnostic, which > fixes the execution FAIL of to_chars/long_double.cc on powerpc64be. > > Tested on powerpc64le and powerpc64be. Does this look OK for trunk? > I was going to suggest __builtin_unpack_longdouble but actually since you want the bits this is better. You'd need another step to get the bits from each double. OK for trunk. ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] libstdc++: Robustify long double std::to_chars testcase [PR98384] 2021-02-22 21:55 [PATCH 1/2] libstdc++: Robustify long double std::to_chars testcase [PR98384] Patrick Palka 2021-02-22 21:55 ` [PATCH 2/2] libstdc++: Fix endianness issue with IBM long double [PR98384] Patrick Palka @ 2021-02-23 16:30 ` Patrick Palka 2021-02-24 16:44 ` Jonathan Wakely 1 sibling, 1 reply; 7+ messages in thread From: Patrick Palka @ 2021-02-23 16:30 UTC (permalink / raw) To: Patrick Palka; +Cc: gcc-patches, libstdc++ On Mon, 22 Feb 2021, Patrick Palka wrote: > This makes the hexadecimal section of the long double std::to_chars > testcase more robust by avoiding false-negative FAILs due to printf > using a different leading hex digit than us, and by additionally > verifying the correctness of the hexadecimal form via round-tripping > through std::from_chars. > > Tested on x86, x86_64, powerpc64be, powerpc64le and aarch64. Does this > look OK for trunk? The commit message could explain the issue better, so here's v2 with a more detailed commit message. -- >8 -- Subject: [PATCH] libstdc++: Robustify long double std::to_chars testcase [PR98384] The long double std::to_chars testcase currently verifies the correctness of its output by comparing it to that of printf, so if there's a mismatch between to_chars and printf, the test FAILs. This works well for the scientific, fixed and general formatting modes, because the corresponding printf conversion specifiers (%e, %f and %g) are rigidly specified. But this doesn't work so well for the hex formatting mode because the corresponding printf conversion specifier %a is more flexibly specified. For instance, the hexadecimal forms 0x1p+0, 0x2p-1, 0x4p-2 and 0x8p-3 are all equivalent and valid outputs of the %a specifier for the number 1. The apparent freedom here is the choice of leading hex digit -- the standard just requires that the leading hex digit is nonzero for normalized numbers. Currently, our hexadecimal formatting implementation uses 0/1/2 as the leading hex digit for floating point types that have an implicit leading mantissa bit which in practice means all supported floating point types except x86 long double. The latter type has a 64 bit mantissa with an explicit leading mantissa bit, and for this type our implementation uses the most significant four bits of the mantissa as leading hex digit. This seems to be consistent with most printf implementations, but not all, as PR98384 illustrates. In order to avoid false-positive FAILs due to arbitrary disagreement between to_chars and printf about the choice of leading hex digit, this patch makes the testcase's verification via printf conditional on the leading hex digits first agreeing. An additional verification step is also added: round-tripping the output of to_chars through from_chars should yield the original value. Tested on x86, x86_64, powerpc64be, powerpc64le and aarch64. Does this look OK for trunk? libstdc++-v3/ChangeLog: PR libstdc++/98384 * testsuite/20_util/to_chars/long_double.cc: Include <optional>. (test01): Simplify verifying the nearby values by using a 2-iteration loop and a dedicated output buffer to check that the nearby values are different. Factor out the printf-based verification into a local function, and check that the leading hex digits agree before comparing with the output of printf. Also verify the output by round-tripping it through from_chars. --- .../testsuite/20_util/to_chars/long_double.cc | 73 ++++++++++++------- 1 file changed, 47 insertions(+), 26 deletions(-) diff --git a/libstdc++-v3/testsuite/20_util/to_chars/long_double.cc b/libstdc++-v3/testsuite/20_util/to_chars/long_double.cc index 4f72cb65400..da847ae5401 100644 --- a/libstdc++-v3/testsuite/20_util/to_chars/long_double.cc +++ b/libstdc++-v3/testsuite/20_util/to_chars/long_double.cc @@ -26,6 +26,7 @@ #include <cmath> #include <cstring> #include <iterator> +#include <optional> #include <limits> #include <testsuite_hooks.h> @@ -50,6 +51,38 @@ namespace detail void test01() { + // Verifies correctness of the hexadecimal form [BEGIN,END) for VALUE by + // round-tripping it through from_chars (if available). + auto verify_via_from_chars = [] (char *begin, char *end, long double value) { +#if __cpp_lib_to_chars >= 201611L || _GLIBCXX_HAVE_USELOCALE + long double roundtrip; + auto result = from_chars(begin, end, roundtrip, chars_format::hex); + VERIFY( result.ec == errc{} ); + VERIFY( result.ptr == end ); + VERIFY( roundtrip == value ); +#endif + }; + + // Verifies correctness of the null-terminated hexadecimal form at BEGIN + // for VALUE and PRECISION by comparing it with the output of printf's %La + // conversion specifier. + auto verify_via_printf = [] (char *begin, long double value, + optional<int> precision = nullopt) { + char printf_buffer[1024] = {}; + if (precision.has_value()) + sprintf(printf_buffer, "%.*La", precision.value(), value); + else + sprintf(printf_buffer, "%La", value); + + // Only compare with the output of printf if the leading hex digits agree. + // If the leading hex digit of our form doesn't agree with that of printf, + // then the two forms may still be equivalent (e.g. 1.1p+0 vs 8.8p-3). But + // if the leading hex digits do agree, then we do expect the two forms to be + // the same. + if (printf_buffer[strlen("0x")] == begin[0]) + VERIFY( !strcmp(begin, printf_buffer+strlen("0x")) ); + }; + const long double hex_testcases[] = { detail::nextdownl(numeric_limits<long double>::max()), detail::nextupl(numeric_limits<long double>::min()), @@ -92,38 +125,27 @@ test01() if (testcase == 0.0L || isinf(testcase)) continue; - char to_chars_buffer[1024], printf_buffer[1024]; - memset(to_chars_buffer, '\0', sizeof(to_chars_buffer)); - memset(printf_buffer, '\0', sizeof(printf_buffer)); - + char to_chars_buffer[1024] = {}; auto result = to_chars(begin(to_chars_buffer), end(to_chars_buffer), testcase, chars_format::hex); VERIFY( result.ec == errc{} ); *result.ptr = '\0'; - sprintf(printf_buffer, "%La", testcase); - VERIFY( !strcmp(to_chars_buffer, printf_buffer+strlen("0x")) ); + verify_via_from_chars(begin(to_chars_buffer), result.ptr, testcase); + verify_via_printf(to_chars_buffer, testcase); + // Verify the nearby values, and also check they have a different + // shortest form. + for (long double nearby + : { detail::nextdownl(testcase), detail::nextupl(testcase) }) { - // Verify that the nearby values have a different shortest form. - testcase = detail::nextdownl(testcase); - result = to_chars(begin(to_chars_buffer), end(to_chars_buffer), - testcase, chars_format::hex); - VERIFY( result.ec == errc{} ); - *result.ptr = '\0'; - VERIFY( strcmp(to_chars_buffer, printf_buffer+strlen("0x")) != 0); - sprintf(printf_buffer, "%La", testcase); - VERIFY( !strcmp(to_chars_buffer, printf_buffer+strlen("0x")) ); - - testcase = detail::nextupl(detail::nextupl(testcase)); - result = to_chars(begin(to_chars_buffer), end(to_chars_buffer), - testcase, chars_format::hex); + char nearby_buffer[1024] = {}; + result = to_chars(begin(nearby_buffer), end(nearby_buffer), + nearby, chars_format::hex); VERIFY( result.ec == errc{} ); *result.ptr = '\0'; - VERIFY( strcmp(to_chars_buffer, printf_buffer+strlen("0x")) != 0); - sprintf(printf_buffer, "%La", testcase); - VERIFY( !strcmp(to_chars_buffer, printf_buffer+strlen("0x")) ); - - testcase = detail::nextdownl(testcase); + VERIFY( strcmp(nearby_buffer, to_chars_buffer) != 0); + verify_via_from_chars(begin(nearby_buffer), result.ptr, nearby); + verify_via_printf(nearby_buffer, nearby); } for (int precision = -1; precision < 50; precision++) @@ -132,8 +154,7 @@ test01() testcase, chars_format::hex, precision); VERIFY( result.ec == errc{} ); *result.ptr = '\0'; - sprintf(printf_buffer, "%.*La", precision, testcase); - VERIFY( !strcmp(to_chars_buffer, printf_buffer+strlen("0x")) ); + verify_via_printf(to_chars_buffer, testcase, precision); } } } -- 2.30.1.602.g966e671106 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] libstdc++: Robustify long double std::to_chars testcase [PR98384] 2021-02-23 16:30 ` [PATCH 1/2] libstdc++: Robustify long double std::to_chars testcase [PR98384] Patrick Palka @ 2021-02-24 16:44 ` Jonathan Wakely 2021-02-24 17:04 ` Patrick Palka 0 siblings, 1 reply; 7+ messages in thread From: Jonathan Wakely @ 2021-02-24 16:44 UTC (permalink / raw) To: Patrick Palka; +Cc: libstdc++, gcc-patches On 23/02/21 11:30 -0500, Patrick Palka via Libstdc++ wrote: >On Mon, 22 Feb 2021, Patrick Palka wrote: > >> This makes the hexadecimal section of the long double std::to_chars >> testcase more robust by avoiding false-negative FAILs due to printf >> using a different leading hex digit than us, and by additionally >> verifying the correctness of the hexadecimal form via round-tripping >> through std::from_chars. >> >> Tested on x86, x86_64, powerpc64be, powerpc64le and aarch64. Does this >> look OK for trunk? > >The commit message could explain the issue better, so here's v2 with a >more detailed commit message. > >-- >8 -- > >Subject: [PATCH] libstdc++: Robustify long double std::to_chars testcase > [PR98384] > >The long double std::to_chars testcase currently verifies the >correctness of its output by comparing it to that of printf, so if >there's a mismatch between to_chars and printf, the test FAILs. This >works well for the scientific, fixed and general formatting modes, >because the corresponding printf conversion specifiers (%e, %f and %g) >are rigidly specified. > >But this doesn't work so well for the hex formatting mode because the >corresponding printf conversion specifier %a is more flexibly specified. >For instance, the hexadecimal forms 0x1p+0, 0x2p-1, 0x4p-2 and 0x8p-3 >are all equivalent and valid outputs of the %a specifier for the number >1. The apparent freedom here is the choice of leading hex digit -- the >standard just requires that the leading hex digit is nonzero for >normalized numbers. > >Currently, our hexadecimal formatting implementation uses 0/1/2 as the >leading hex digit for floating point types that have an implicit leading >mantissa bit which in practice means all supported floating point types >except x86 long double. The latter type has a 64 bit mantissa with an >explicit leading mantissa bit, and for this type our implementation uses >the most significant four bits of the mantissa as leading hex digit. >This seems to be consistent with most printf implementations, but not >all, as PR98384 illustrates. > >In order to avoid false-positive FAILs due to arbitrary disagreement >between to_chars and printf about the choice of leading hex digit, this >patch makes the testcase's verification via printf conditional on the >leading hex digits first agreeing. An additional verification step is >also added: round-tripping the output of to_chars through from_chars >should yield the original value. > >Tested on x86, x86_64, powerpc64be, powerpc64le and aarch64. Does this >look OK for trunk? >@@ -50,6 +51,38 @@ namespace detail > void > test01() > { >+ // Verifies correctness of the hexadecimal form [BEGIN,END) for VALUE by >+ // round-tripping it through from_chars (if available). >+ auto verify_via_from_chars = [] (char *begin, char *end, long double value) { >+#if __cpp_lib_to_chars >= 201611L || _GLIBCXX_HAVE_USELOCALE This is currently going to fail, because we don't actually define __cpp_lib_to_chars yet (we should fix that!) Is checking the feature test macro here useful? We know that floating-point from_chars was committed before to_chars, so if this test is running, we should have from_chars (modulo uselocale being available, so that check is right). Is this to make the test usable for other C++ std::lib implementations? >+ long double roundtrip; >+ auto result = from_chars(begin, end, roundtrip, chars_format::hex); >+ VERIFY( result.ec == errc{} ); >+ VERIFY( result.ptr == end ); >+ VERIFY( roundtrip == value ); >+#endif ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] libstdc++: Robustify long double std::to_chars testcase [PR98384] 2021-02-24 16:44 ` Jonathan Wakely @ 2021-02-24 17:04 ` Patrick Palka 2021-02-24 17:08 ` Jonathan Wakely 0 siblings, 1 reply; 7+ messages in thread From: Patrick Palka @ 2021-02-24 17:04 UTC (permalink / raw) To: Jonathan Wakely; +Cc: Patrick Palka, libstdc++, gcc-patches On Wed, 24 Feb 2021, Jonathan Wakely wrote: > On 23/02/21 11:30 -0500, Patrick Palka via Libstdc++ wrote: > > On Mon, 22 Feb 2021, Patrick Palka wrote: > > > > > This makes the hexadecimal section of the long double std::to_chars > > > testcase more robust by avoiding false-negative FAILs due to printf > > > using a different leading hex digit than us, and by additionally > > > verifying the correctness of the hexadecimal form via round-tripping > > > through std::from_chars. > > > > > > Tested on x86, x86_64, powerpc64be, powerpc64le and aarch64. Does this > > > look OK for trunk? > > > > The commit message could explain the issue better, so here's v2 with a > > more detailed commit message. > > > > -- >8 -- > > > > Subject: [PATCH] libstdc++: Robustify long double std::to_chars testcase > > [PR98384] > > > > The long double std::to_chars testcase currently verifies the > > correctness of its output by comparing it to that of printf, so if > > there's a mismatch between to_chars and printf, the test FAILs. This > > works well for the scientific, fixed and general formatting modes, > > because the corresponding printf conversion specifiers (%e, %f and %g) > > are rigidly specified. > > > > But this doesn't work so well for the hex formatting mode because the > > corresponding printf conversion specifier %a is more flexibly specified. > > For instance, the hexadecimal forms 0x1p+0, 0x2p-1, 0x4p-2 and 0x8p-3 > > are all equivalent and valid outputs of the %a specifier for the number > > 1. The apparent freedom here is the choice of leading hex digit -- the > > standard just requires that the leading hex digit is nonzero for > > normalized numbers. > > > > Currently, our hexadecimal formatting implementation uses 0/1/2 as the > > leading hex digit for floating point types that have an implicit leading > > mantissa bit which in practice means all supported floating point types > > except x86 long double. The latter type has a 64 bit mantissa with an > > explicit leading mantissa bit, and for this type our implementation uses > > the most significant four bits of the mantissa as leading hex digit. > > This seems to be consistent with most printf implementations, but not > > all, as PR98384 illustrates. > > > > In order to avoid false-positive FAILs due to arbitrary disagreement > > between to_chars and printf about the choice of leading hex digit, this > > patch makes the testcase's verification via printf conditional on the > > leading hex digits first agreeing. An additional verification step is > > also added: round-tripping the output of to_chars through from_chars > > should yield the original value. > > > > Tested on x86, x86_64, powerpc64be, powerpc64le and aarch64. Does this > > look OK for trunk? > > > @@ -50,6 +51,38 @@ namespace detail > > void > > test01() > > { > > + // Verifies correctness of the hexadecimal form [BEGIN,END) for VALUE by > > + // round-tripping it through from_chars (if available). > > + auto verify_via_from_chars = [] (char *begin, char *end, long double > > value) { > > +#if __cpp_lib_to_chars >= 201611L || _GLIBCXX_HAVE_USELOCALE > > This is currently going to fail, because we don't actually define > __cpp_lib_to_chars yet (we should fix that!) > > Is checking the feature test macro here useful? We know that > floating-point from_chars was committed before to_chars, so if this > test is running, we should have from_chars (modulo uselocale being > available, so that check is right). Is this to make the test usable > for other C++ std::lib implementations? This preprocessor check is copied from from_chars/{5,6}.cc, which I figured should be appropriate to use here as well. I figured we'd want to adjust each of these checks after we define __cpp_lib_to_chars appropriately anyway (e.g. if __cpp_lib_to_chars is conditioned on uselocale being available, then the three tests should be changed just look at __cpp_lib_to_chars, IIUC). > > > + long double roundtrip; > > + auto result = from_chars(begin, end, roundtrip, chars_format::hex); > > + VERIFY( result.ec == errc{} ); > > + VERIFY( result.ptr == end ); > > + VERIFY( roundtrip == value ); > > +#endif > > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] libstdc++: Robustify long double std::to_chars testcase [PR98384] 2021-02-24 17:04 ` Patrick Palka @ 2021-02-24 17:08 ` Jonathan Wakely 0 siblings, 0 replies; 7+ messages in thread From: Jonathan Wakely @ 2021-02-24 17:08 UTC (permalink / raw) To: Patrick Palka; +Cc: libstdc++, gcc-patches On 24/02/21 12:04 -0500, Patrick Palka via Libstdc++ wrote: >On Wed, 24 Feb 2021, Jonathan Wakely wrote: > >> On 23/02/21 11:30 -0500, Patrick Palka via Libstdc++ wrote: >> > On Mon, 22 Feb 2021, Patrick Palka wrote: >> > >> > > This makes the hexadecimal section of the long double std::to_chars >> > > testcase more robust by avoiding false-negative FAILs due to printf >> > > using a different leading hex digit than us, and by additionally >> > > verifying the correctness of the hexadecimal form via round-tripping >> > > through std::from_chars. >> > > >> > > Tested on x86, x86_64, powerpc64be, powerpc64le and aarch64. Does this >> > > look OK for trunk? >> > >> > The commit message could explain the issue better, so here's v2 with a >> > more detailed commit message. >> > >> > -- >8 -- >> > >> > Subject: [PATCH] libstdc++: Robustify long double std::to_chars testcase >> > [PR98384] >> > >> > The long double std::to_chars testcase currently verifies the >> > correctness of its output by comparing it to that of printf, so if >> > there's a mismatch between to_chars and printf, the test FAILs. This >> > works well for the scientific, fixed and general formatting modes, >> > because the corresponding printf conversion specifiers (%e, %f and %g) >> > are rigidly specified. >> > >> > But this doesn't work so well for the hex formatting mode because the >> > corresponding printf conversion specifier %a is more flexibly specified. >> > For instance, the hexadecimal forms 0x1p+0, 0x2p-1, 0x4p-2 and 0x8p-3 >> > are all equivalent and valid outputs of the %a specifier for the number >> > 1. The apparent freedom here is the choice of leading hex digit -- the >> > standard just requires that the leading hex digit is nonzero for >> > normalized numbers. >> > >> > Currently, our hexadecimal formatting implementation uses 0/1/2 as the >> > leading hex digit for floating point types that have an implicit leading >> > mantissa bit which in practice means all supported floating point types >> > except x86 long double. The latter type has a 64 bit mantissa with an >> > explicit leading mantissa bit, and for this type our implementation uses >> > the most significant four bits of the mantissa as leading hex digit. >> > This seems to be consistent with most printf implementations, but not >> > all, as PR98384 illustrates. >> > >> > In order to avoid false-positive FAILs due to arbitrary disagreement >> > between to_chars and printf about the choice of leading hex digit, this >> > patch makes the testcase's verification via printf conditional on the >> > leading hex digits first agreeing. An additional verification step is >> > also added: round-tripping the output of to_chars through from_chars >> > should yield the original value. >> > >> > Tested on x86, x86_64, powerpc64be, powerpc64le and aarch64. Does this >> > look OK for trunk? >> >> > @@ -50,6 +51,38 @@ namespace detail >> > void >> > test01() >> > { >> > + // Verifies correctness of the hexadecimal form [BEGIN,END) for VALUE by >> > + // round-tripping it through from_chars (if available). >> > + auto verify_via_from_chars = [] (char *begin, char *end, long double >> > value) { >> > +#if __cpp_lib_to_chars >= 201611L || _GLIBCXX_HAVE_USELOCALE >> >> This is currently going to fail, because we don't actually define >> __cpp_lib_to_chars yet (we should fix that!) >> >> Is checking the feature test macro here useful? We know that >> floating-point from_chars was committed before to_chars, so if this >> test is running, we should have from_chars (modulo uselocale being >> available, so that check is right). Is this to make the test usable >> for other C++ std::lib implementations? > >This preprocessor check is copied from from_chars/{5,6}.cc, which I I was going to say "which idiot wrote that then?" and then I realised that the check is fine and I just misread the || as &&. Doh. >figured should be appropriate to use here as well. I figured we'd >want to adjust each of these checks after we define __cpp_lib_to_chars >appropriately anyway (e.g. if __cpp_lib_to_chars is conditioned on >uselocale being available, then the three tests should be changed just >look at __cpp_lib_to_chars, IIUC). Agreed. The patch is fine for trunk, sorry for the noise. Thanks. ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2021-02-24 17:08 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-02-22 21:55 [PATCH 1/2] libstdc++: Robustify long double std::to_chars testcase [PR98384] Patrick Palka 2021-02-22 21:55 ` [PATCH 2/2] libstdc++: Fix endianness issue with IBM long double [PR98384] Patrick Palka 2021-02-22 22:22 ` Jonathan Wakely 2021-02-23 16:30 ` [PATCH 1/2] libstdc++: Robustify long double std::to_chars testcase [PR98384] Patrick Palka 2021-02-24 16:44 ` Jonathan Wakely 2021-02-24 17:04 ` Patrick Palka 2021-02-24 17:08 ` Jonathan Wakely
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).